[#46360] - [6.0] Optimize checks in plg_behaviour_versionable
- Pending
- Medium
- Build: 6.0-dev
- # 46360
- Diff
- Elfangor93:rem-model-plgVersions
User tests: Successful: Unsuccessful:
Pull Request for Issue #46040 .
Summary of Changes
Remove the model check in onTableAfterStore of the Behaviour - Versionable (plg_behaviour_versionable).
This PR needs the PR #46268.
In PR #46268, both saving and deleting are handled by VersionableModelInterface. Therefore, extensions using VersionableModelInterface can update their Table classes to remove implements VersionableTableInterface. Therefore the code removed in this PR is no longer required.
Testing Instructions
Use Joomla! 6
Test 1 (Check if versioning works)
- Install a component which uses versioning. e.g Weblinks component
(https://downloads.joomla.org/extensions/weblinks/5-0-0) - Enable versioning for this component
- Create a version history and delete the version afterwards again.
Test 2 (Check if installation works)
- Install JoomGallery Extension (Available here)
Actual result BEFORE applying this Pull Request
Test 1
Creating and deleting of versions is working correctly.
Test 2
JoomGallery extension installation fails.
Expected result AFTER applying this Pull Request
Test 1
Creating and deleting of versions is working correctly.
Test 2
JoomGallery extension installation successful.
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
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Front End Plugins |
| Labels |
Added:
PR-6.0-dev
|
||
@richard67 Yes, it works. But I would like to take this chance to clean up the method a bit more: Some check should be performed earlier, some check is redundant, the mis-leading comment... @Elfangor93 Could you please check and merge my PR Elfangor93#2 to your branch if it is OK for you?
Thanks @joomdonation I merged your PR.
But I had to add a token to the function strtok() you used as a replacement for explode(). Since php 8.3, strtok() requires a token (string) as the second function parameter.
https://www.php.net/manual/de/function.strtok.php#refsect1-function.strtok-changelog
Thanks @joomdonation I merged your PR. But I had to add a token to the function
strtok()you used as a replacement forexplode(). Since php 8.3,strtok()requires a token (string) as the second function parameter.https://www.php.net/manual/de/function.strtok.php#refsect1-function.strtok-changelog
I was just about to comment in the same way, but @Elfangor93 you were was faster.
But I had to add a token to the function strtok()
Yes, I missed it in my code. Thanks for adding it. Busy at the moment, so I will test it later today.
| Title |
|
||||||
BTW, we do not use this plugin in core anymore. So to test it, we will need to test and make sure version history still work for extensions use this plugin. I guess we can use Weblinks for testing https://downloads.joomla.org/extensions/weblinks/5-0-0
I have tested this item ✅ successfully on 154102f
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46360.
Version history in Weblinks is broken at the moment (due to invalid data in #__content_types database table, happens since the time we namespaced the extension but I no-one knows about the bug), so it could not be used for testing. I made correction to the data manually, tested weblinks version history and it worked
So for other testers, all you need to test is making sure version history still works OK for article and maybe contact still work as expected. A bonus test would be making sure JoomGallery is being installed properly as mentioned in testing instructions, but it is not really required.
@joomdonation Could you check this one here? Thanks in advance.