Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.3] Do not track rows in #__ucm_content as assets #39165

Merged
merged 3 commits into from
Dec 30, 2022

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Nov 6, 2022

Pull Request for Issue #37153.

Summary of Changes

The UCM system currently spams the assets table with useless entries. Each row in the ucm_content table gets a corresponding row in the assets table which is a direct child of the root node and doesn't have any children itself. Their content always is empty, since the assets data is not copied over from the original content and also is never used anywhere. This PR stops the ucm_content table class from tracking these rows as assets and deletes the existing records in the assets table.

The delete query will not break the tree, since all these entries are leaf nodes, so it will not leave behind any orphaned children. The only inconsistencies by this are holes in the lft and rgt values, however the nested sets model is resilient against these holes and thus this shouldn't create an issue. To close these holes in these values, we would have to load the assets table class and call a $table->rebuild() on this, but since this is a time-consuming task and especially for large sites will most likely generate timeouts, I'm leaving this out here.

Doing stuff like such a rebuild would be something for a maintenance component in the future.

Testing Instructions

  1. Create an article and save it
  2. Go to #__assets and see the new entry for your article. See directly below another new entry with a name and title similar to #__ucm_content.42 and the rules column having the value {}.
  3. Apply patch and create another article.
  4. See that there is no new #__ucm_content.69 entry in the assets table accompanying your articles asset entry.
  5. Execute the included query to clear your assets table from all the dummy entries.
  6. See that everything works as before.

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

@ReLater
Copy link
Contributor

ReLater commented Dec 22, 2022

I have tested this item ✅ successfully on 0c3c734

For me the testing instructions (before patch part) only worked after adding a tag to the new articles. Otherwise no new entry in #__ucm_content and related entry in #__assets are created and it is impossible to compare the testing results before/after patch.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39165.

@Hackwar
Copy link
Member Author

Hackwar commented Dec 29, 2022

Thank you @ReLater

@chmst
Copy link
Contributor

chmst commented Dec 29, 2022

I have tested this item ✅ successfully on 0c3c734

Thanks for the hint @ReLater


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39165.

@chmst
Copy link
Contributor

chmst commented Dec 29, 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39165.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 29, 2022
@joomdonation
Copy link
Contributor

I was about to report my test result this PR, too. Maybe It's worth to mention that before this PR , the asset_id in #__ucm_content table stores asset ID of the UCM record. After this PR, asset_id stores asset ID of the actual item (for example, actual article)

So there is changed with how data is stored. However, I think this change is fine as it is. It makes more sense to store asset ID of the actual item for permission checking if needed than store asset ID of UCM record which is useless. It also helps reduce number of records store in #__assets table, so I would report my test as success, too.

@alikon
Copy link
Contributor

alikon commented Dec 29, 2022

imho it's not good to execute DELETE FROM "#__assets" WHERE "name" LIKE '#__ucm_content.%'; and leave holes in the nested set

@Hackwar
Copy link
Member Author

Hackwar commented Dec 30, 2022

Since we aren't doing any operations on that table which require the lft and rgt values to not have holes, I wouldn't really care about this here. In the future I'm planning a component to do maintenance and fix stuff like this. Hopefully for 4.4.

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Dec 30, 2022
@obuisard obuisard merged commit acc959a into joomla:4.3-dev Dec 30, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 30, 2022
@obuisard
Copy link
Contributor

Thank you Hannes @Hackwar !

@Hackwar Hackwar deleted the 4.3-assets branch April 17, 2023 07:37
@sanderpotjer
Copy link
Member

I do agree that tracking UCM content does not makes any sense, but this change resulting in unwanted behavior as ucm content is still added via tags for example: #43669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants