-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fixed event dispatching inconsistencies leading to incomplete API #4308
Conversation
This looks great, thanks for that work! |
What @Bakual said. Something I've been meaning to do for ages! Thank you very much! |
@@ -173,12 +176,17 @@ public function save($data) | |||
throw new RuntimeException($table->getError()); | |||
} | |||
|
|||
// Store the data. | |||
if (!$table->store()) | |||
$result = $dispatcher->trigger('onExtensionBeforeSave', array($context, &$table, false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't read the whole changelog yet, but shouldn't we use the events_map array here?
@Bakual, @wilsonge You are welcome!. @Hackwar Hi!, thank you for taking the time to review the proposed changes. The events_map array is used to map events to plugin groups, and this so that the model knows which plugin group to load before triggering these events. Let's remember that in Joomla!, events are tied to plugin groups, and that in a given model ... we may need to load different plugin groups for different actions. The events_map array allows for this. If you check JModelAdmin, you will see that all of the events are overridable in the constructor, and this was already the case before before the PR. This PR aims to have a minimal impact on the current existing codebase while providing solutions to the problems mentioned above. There are some models where the events on the trigger calls are hardcoded, and this because those models are not extending from JModelAdmin. Most of them extend from JModelForm. These are not re-usable models but implementations, so no need to make those overridable unless the extra flexibility might get handy. ConfigModelComponent and MenusModelMenu are examples. Does this help?. |
@amazeika Yes, you are right. I read the code wrong initially. I just stumbled over this, since I want us to have several more events and make those configurable. In any case, its fine the way it is. |
Hi @Hackwar!. Okay, good. Thank you again for taking the time to check this out. |
Looks like there are some potential merge conflicts. @amazeika could you update to the latest staging? This could be tested during PBF by some developers. |
@nicksavov Hi!, I've just rebased the PR changes on top of the latest staging. However, Travis isn't liking this anymore. Something to do with your unit checks?. |
Hi yes, It's our (@joomla 's) fault the tests are failing here. If you merge in/rebase on staging again should be fixed :) sorry! |
This allows for events to be mapped to plugin groups independently.
Plugins are not loaded before the onUserAuthorisationFailure is triggered.
@wilsonge Thank you George!. Not a problem. Let's see how it goes now :). |
Testing the specified Models (Template Style, Module, Menu) firing inherited triggers with patch. Menu AddBeforeonContentBeforeDelete triggered in com_menus.item context AfteronContentPrepareData triggered in com_menus.item context Template DeleteBeforeNothing triggered AfteronContentBeforeDelete triggered in com_templates.style context Module DeleteBeforeNothing triggered AfteronExtensionBeforeDelete triggered in com_modules.module context Also tested Content, WebLinks, Users, Groups with same triggers being fired before and after patch applied (appears to be correct triggers being fired in both situations). @amazeika please let me know if there are any other inherited triggers that require testing. |
Tried to test this, got as far as building the extension but couldn't figure out what I was meant to do thereafter. Thought the file attached might speed the process up for some who might not have or know how to using phing/composer. Ruth |
@test OK
Content - CreationonContentPrepareForm has been triggered Content - RetrievalonContentPrepareData triggered in com_content.article context Content - ModificationonContentPrepareForm has been triggered Content - RemovalonContentStateChange triggered in com_content.article context
onContentBeforeDelete triggered in com_content.article context Extension - CreationonExtensionAfterInstall has been triggered Extension - RetrievalonContentPrepareData triggered in com_modules.module context Extension - ModificationonContentPrepareForm has been triggered Extension - RemovalonContentStateChange triggered in com_modules.module context
onExtensionBeforeDelete triggered in com_modules.module context User - CreationonUserBeforeSave has been triggered User - RetrievalonContentPrepareData triggered in com_users.user context User - ModificationonUserBeforeSave has been triggered User - RemovalonUserBeforeDelete has been triggered |
@haydenyoung Thanks for testing. Your test results look good. Please also make sure that events on component config save actions are being triggered. This is something new. Template style duplication (duplicate action) is something that wasn't triggering events. Now save events should trigger when duplicating styles. Menus save and delete actions were not triggering events either. This should be tested as well. @RCheesley These actions were not triggering events: Menu item: add, edit This is a good start for testing purposes. The PR includes changes deep in the base admin model so ideally each component making use of it should be tested. Events triggered by all the models that got changed in this PR must be tested. Thank you for providing a catcher package. @ALL Thank you all for taking some time to test this :). |
Here are my test results: ContentContent CreationonContentPrepareForm has been triggered Content RetrievalonContentPrepareData triggered in com_content.article context Content modificationonContentPrepareForm has been triggered Content RemovalonContentStateChange triggered in com_content.article context Emptying TrashonContentStateChange triggered in com_content.article context At this point I will assume that the rest of the already passed tests will still pass, and instead test the missing areas requested by @amazeika Component ConfigSaving configonContentPrepareForm has been triggered Template styleTemplate style duplicationonContentBeforeSave triggered in com_templates.style context Menu itemMenu item creationonContentPrepareData triggered in com_menus.item context Menu item updateonContentPrepareData triggered in com_menus.item context Menu item deletiononContentStateChange triggered in com_menus.item context Emptying menu item trashonContentBeforeDelete triggered in com_menus.item context MenuMenu saveonContentPrepareData triggered in com_menus.menu context Menu edit saveonContentPrepareData triggered in com_menus.menu context Menu deleteonContentBeforeDelete triggered in com_menus.menu context ModuleDeleteonContentStateChange triggered in com_modules.module context Empty trashonExtensionBeforeDelete triggered in com_modules.module context DONE! |
Great testing! @amazeika, looks like there are some merge conflicts with the latest staging. Could you update the PR again? Thanks in advance! |
Another long long test result: MenusMenu Browse:Before:
After:
Menu ReadBefore:
After:
Menu CreateBefore:
After:
Menu UpdateBefore:
After:
Menu DeleteBefore:
After:
Menu-ItemsMenu-Item Browse:Before:
After:
Menu-Item ReadBefore:
Menu-Item CreateBefore:
After:
Menu-Item UpdateBefore:
After:
Menu-Item DeleteBefore:
After:
Template StylesTemplate Style Browse:Before:
After:
Template Style ReadBefore:
After:
Template Style CreateBefore:
After:
Template Style UpdateBefore:
After:
Template Style DeleteBefore:
After:
ModulesModule Browse:Before:
After:
Module ReadBefore:
After:
Module CreateBefore:
After:
Module UpdateBefore:
After:
Module DeleteBefore:
After:
Articles / ContentContent BrowseBefore:
After:
Content ReadBefore:
After:
Content CreateBefore:
After:
Content UpdateBefore:
After:
Content DeleteBefore:
After:
UsersUser BrowseBefore:
After:
User ReadBefore:
After:
User CreateBefore:
After:
User UpdateBefore:
After:
User DeleteBefore:
After:
|
@aDaneInSpain and I have been testing this all night. Code is good already. However testing is a must. @anibalsanchez tests are also good. I think this is just ready for commit .... |
thanks for testing guys setting RTC |
Merged, thanks! The conflict was a small codestyle issue. Resolved that myself. |
Congrats guys! :) |
This broke groups management. Please check #5152 |
[fix] Broken groups management by #4308
Another issue caused by this PR. Plugins broken. Please check #5202 |
The typo resulted on malformed contexts on save action events.
Fixed typo introduced on PR #4308
@phproberto Thank you for noticing the issues exposed above. The first problem was related to a bad merge, either while rebasing to resolve conflicts or when the PR got merged. I was able to confirm this on a local clone. The double else statement wasn't there. In any case thank you for the fix. Regarding the second issue, we've just submitted a PR for fixing the TYPO that got mistakenly introduced. This wasn't intentional. Thank you again for reporting it. |
@amazeika ThankYOU for submitting this in the first place. Every major PR like this has small bugs or something that creep in but you went through and fixed everything :) So thanks :) |
@wilsonge Thank you George ... I appreciate your comment :). |
staging # By Michael Babker (143) and others # Via infograf768 (131) and others * 'staging' of https://github.com/joomla/joomla-cms: (1128 commits) Fixed typo introduced on PR joomla#4308 Fixed missing space Correcting comments [fix joomla#5173] JRouterInstallation::build() strict standards trim the author and created_by_alias before saving Remove all \r\n Update en-GB.plg_system_debug.ini JRouterInstallation::parse() made compatible with the interface (Fix joomla#5173) Clean up non-printable characters. Fixed creationDate Fixes Author and Copyright info for Editor - None Routing: Implementing an interface for component router rules (Fix joomla#4849) Routing: Adding application and menu objects to component routers (Fix joomla#4848) Fixing JGithub repository contents api (Fix joomla#5067) [bug] Protostar template doesn't work with component links in SEF. Fixes joomla#5085 Fix undefined variable in Article Model ... use the right variable, dummy. fix the codemirror display bug, happens when codemirror is initialized in a hidden element. Use the created_by_alias in the author meta tag. This is consistent with how author name is show elsewhere. [fix] Broken groups management by joomla#4308 ... Conflicts: installation/sql/sqlazure/sample_blog.sql installation/sql/sqlazure/sample_testing.sql
Issue joomlatools#3 |
Reason
Incomplete API. We spotted inconsistencies on some component models which are not properly triggering events as expected. This results in an incomplete and inconsistent API for developers. For example in our case we would like to be able to keep track of extension events, which is currently not possible.
Problem
Some model’s methods are being overridden without calling their parent method. By not calling the parent the event dispatching process is being bypassed.
Example
MenusModelItem::save does not trigger the before/after save events because it does not call its parent.
Patch
This patch includes the following fixes and enhancements:
The API
Extreme care has been taken in order to provide full backwards compatibility, meaning that no API change has been introduced in this patch. However, we would like to report some anomalies that are a direct consequence of the previous inheritance approach being used.
Delete and state change actions from PluginsModelPlugin are inherited from JModelAdmin. This means that until now, the events being triggered for these actions are onContent(Before/After)Delete and onContentChangeState respectively.
On the other hand, onExtension(Before/After)Save events are triggered on PluginsModelPlugin::save calls. This introduces inconsistencies in the API.
One possible approach to fix this would be to use extension events for the delete and change state actions, i.e. onExtension(Before/After)Delete and onExtensionChangeState respectively.
However, and in order to keep things clean while ensuring backward compatibility, we decided not to introduce these changes, but to report them.
This same behavior can be observed on ModulesModelModule and LanguagesLanguage.
onExtension(After/Before)Delete events have been introduced on ModulesModelModule::delete and TemplatesModelStyle::delete calls. Since no events were previously triggered on these calls, triggering these new events does not introduce an API change which might lead to backward compatibility problems.
Tests
In order to properly test this patch, we have developed a small package for catching events. We have called it Catcher.
The package includes a library and set of configurable plugins. These plugins catch events and reports them (optionally with event data) to the UI using the Joomla! messaging queue.
The Catcher package may be found in the following repo:
https://github.com/nooku/joomla-catcher
The package may be installed in using composer or by building a package.
Composer install
This is the easiest way to install the Catcher. The first step is to download composer on your Joomla! site root path. Then create a new composer.json file (also in the root directory of the site) with the following content:
The next and last step is to run the following command from your terminal while located in the site root directory:
Package install
For convenience, a phing script for building the package is also available under the /scripts/build folder of the repo.
After the package is installed, make sure to enable the plugins and configure them to catch and report events.
Thanks in advance for considering this patch.
The Timble team.
http://www.timble.net