Set correct AssetTitle and AssetParentId #42485
Conversation
|
sorry for the mistake,. must remember not ton post if I have not slept in 24hrs |
|
@sanderpotjer Sorry to say, but your PR seems incomplete to me. For new installations, the assets of tasks are set here e.g. for MySQL (same of course for PostgreSQL in the corresponding file): This would need to be changed, too, so they are right from the beginning on for new installations, and that would require to rework lft and rgt values of other assets, too, when making them a child of asset 90. Are you familiar with nested tables so you know how to change the lft and rgt values? You could find some information on that here: https://en.wikipedia.org/wiki/Nested_set_model If you want, try to make the necessary changes with this PR, and I can check and if necessary provide corrections. If you think it's too complicated let me know, then I can try to propose the necessary changes. But there is another question to be decided: Is moving the task assets to below the scheduler component's asset considered a bug fix? Then it should be made in 4.0-dev and later be merged up into 5.0-dev so it goes into the next patch releases of 4.4 and 5.0. Or is it some kind of a new feature? Then it should be made for the 5.0-dev branch so it goes out with 5.1.0. It's also worth a question if we should migrate any task's assets on update. I think we should not do it as it can't really be easily done with an SQL update script due to the nested table thing mentioned above, so it would require a migration routine in script.php, but that is over-engineered I think, as the assets will be fixed when saving them in the administrator like described in the testing instructions. |
The following presentation by P. Martin may be an addition, link https://www.youtube.com/watch?v=YCs6YD_H14c&t=783s |
@sandewt I am familiar with nested tables so won't need that video. @sanderpotjer Another thing: As it turns out, this PR is good as it is, but it should be made for 4.4-dev as it is a bug fix. It will then later be merged up into 5.0-dev by maintainers. In 4.4-dev we do not have any task assets yet for new installations, so the PR is good as it is. In 5.0-dev it would need a separate follow-up PR when this one has been merged up, which will handle the base.sql changes. I can do that later when it is the time. |
sanderpotjer
requested review from
HLeithner,
Hackwar,
bembelimen,
chmst,
laoneo,
rdeutz and
zero-24
as code owners
|
@richard67 thanks for the feedback! I'm familiar with nested tables, actually have a tool that generates the perfect #__assets table values. If tried to correct the installation/sample data multiple times in the past, but of course outdated by the time someone looks at it. So also the reason I did not include changes on that area in this PR. This is a bug fix of code that asset-wise should not have been merged in the first place. So glad you changed mind that the installation sql changes are not needed for this PR 😄 Created a new PR for 4.4-dev: #42493 |
5 participants
Pull Request for Issue #42484.
Summary of Changes
Currently the asset title and parent asset id are not set for the Scheduled Tasks, so ending up incorrectly in the database. This PR sets the correct values for both
Testing Instructions
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Link to documentations
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed