-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[6.0] Optimise checks in plg_behaviour_versionable #46360
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
base: 6.0-dev
Are you sure you want to change the base?
Conversation
|
@joomdonation Could you check this one here? Thanks in advance. |
|
@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? |
Further clean up to Versionable plugin
|
Thanks @joomdonation I merged your PR. 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. |
Yes, I missed it in my code. Thanks for adding it. Busy at the moment, so I will test it later today. |
|
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. |
|
I applied the code changes from this PR in an existing J6.0.0 installation. |
|
@MrMusic Please go to this PR in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/46360 and mark your test result by using the blue "Test this" button at the top left corner, selecting your test result and submitting. Posting a comment with a green check mark is not sufficient for the result being properly counted. Thanks in advance. |
|
In the famous words of @brianteeman ;), if you don't match the BEFORE condition, then something is wrong ;) lol Possibly, I did something wrong but I installed Joomla 6 Nightly, installed Weblinks, and when I tried to check versioning after having made a couple of versions, I receive this error message: An error has occurred. Did I do something wrong? ;( |
|
I have tested this item ✅ successfully on 154102f Thanks @Elfangor93 for the fix/PR. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46360. |
|
@MrMusic Please mark your test result as instructed here #46360 (comment) . |
|
@MrMusic As you haven't reacted on my request to mark your test result, I will do that for you. Every PR needs 2 human tests to be accepted, and marking the test result makes sure that the tests are counted and helps maintainers to find these PRs. Not marking the test result causes additional work for maintainers. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46360. |
|
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. |
@richard67 Thank you for taking care of that.
Sorry, but for personal reasons, I am not always able to respond quickly. |
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)
(https://downloads.joomla.org/extensions/weblinks/5-0-0)
Test 2 (Check if installation works)
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