[#4308] - Fixed event dispatching inconsistencies leading to incomplete API
- Closed
- 8 Nov 2014
- Medium
- Build: staging
- # 4308
- Diff
- nooku:staging
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
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:
- It aims to solve the issue by properly taking care of the dispatching process on model actions.
- It also improves JModelAdmin a bit by reducing the amount of required code in some cases, e.g. extending JModelAdmin and wanting to trigger events on plugins other than content.
- Additionally event support was also added to com_config component save actions.
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:
{
"require": {
"nooku/pkg_catcher": "1.*"
}
}
The next and last step is to run the following command from your terminal while located in the site root directory:
php composer.phar install
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
jissues-bot
- | Labels |
Added:
?
|
||
| Category | ⇒ | Libraries |
@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?.
| Labels |
Added:
?
|
||
@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?.
Testing the specified Models (Template Style, Module, Menu) firing inherited triggers with patch.
Menu Add
Before
onContentBeforeDelete triggered in com_menus.item context
onContentAfterDelete triggered in com_menus.item context
onContentPrepareForm has been triggered
After
onContentPrepareData triggered in com_menus.item context
onContentPrepareForm has been triggered
onContentBeforeSave triggered in com_menus.item context
The item is NEW
onContentAfterSave triggered in com_menus.item context
The item is NEW
onContentPrepareForm has been triggered
Template Delete
Before
Nothing triggered
After
onContentBeforeDelete triggered in com_templates.style context
onContentAfterDelete triggered in com_templates.style context
Module Delete
Before
Nothing triggered
After
onExtensionBeforeDelete triggered in com_modules.module context
onExtensionAfterDelete 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.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4308.
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
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4308.
@test OK
- Installed Ruth's Catcher (thank you!) and enabled CRUD events for Content, Extension, and User
Content - Creation
onContentPrepareForm has been triggered
onContentBeforeSave triggered in com_content.article context
The item is NEW
onContentPrepareData triggered in com_content.article context
onContentPrepareForm has been triggered
Content - Retrieval
onContentPrepareData triggered in com_content.article context
onContentPrepareForm has been triggered
Content - Modification
onContentPrepareForm has been triggered
onContentBeforeSave triggered in com_content_article context
The item is NOT new
onContentAfterSave triggered in com_content_article context
The item is NOT new
onContentPrepareForm has been triggered
Content - Removal
onContentStateChange triggered in com_content.article context
onContentPrepareForm has been triggered
- Empty Trash
onContentBeforeDelete triggered in com_content.article context
onContentAfterDelete triggered in com_content.article context
onContentPrepareForm has been triggered
Extension - Creation
onExtensionAfterInstall has been triggered
Extension - Retrieval
onContentPrepareData triggered in com_modules.module context
onContentPrepareForm has been triggered
Extension - Modification
onContentPrepareForm has been triggered
onExtensionBeforeSave triggered in com_modules.module context
The item is NOT new
onExtensionAfterSave triggered in com_modules.module context
The item is NOT new
Extension - Removal
onContentStateChange triggered in com_modules.module context
- Empty Trash
onExtensionBeforeDelete triggered in com_modules.module context
onExtensionAfterDelete triggered in com_modules.module context
User - Creation
onUserBeforeSave has been triggered
The item is NEW
onUserAfterSave has been triggered
The item is NEW
onContentPrepareForm has been triggered
User - Retrieval
onContentPrepareData triggered in com_users.user context
onContentPrepareData triggered in com_users.profile context
onContentPrepareForm has been triggered
onContentPrepareData triggered in com_users.user context
User - Modification
onUserBeforeSave has been triggered
The item is NOT new
onUserAfterSave has been triggered
The item is NOT new
onContentPrepareForm has been triggered
User - Removal
onUserBeforeDelete has been triggered
onUserAfterDelete has been triggered
onContentPrepareForm has been triggered
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4308.
@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
Menu: add, edit, delete
Module: delete
Template style: add (duplicate), delete
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:
Content Creation
onContentPrepareForm has been triggered
onContentBeforeSave triggered in com_content_article context
The item is NEW
onContentAfterSave triggered in com_content_article context
The item is NEW
onContentPrepareForm has been triggered
Content Retrieval
onContentPrepareData triggered in com_content.article context
onContentPrepareForm has been triggered
Content modification
onContentPrepareForm has been triggered
onContentBeforeSave triggered in com_content_article context
The item is NOT new
onContentAfterSave triggered in com_content_article context
The item is NOT new
onContentPrepareData triggered in com_content.article context
onContentPrepareForm has been triggered
Content Removal
onContentStateChange triggered in com_content.article context
onContentPrepareForm has been triggered
.... I am testing more just don't want to loose progress
Another long long test result:
Menus
Menu Browse:
Before:
- No event After:
- No event ## Menu Read Before:
- onContentPrepareForm triggered in com_menus.menu After:
- onContentPrepareData triggered in com_menus.menu
- onContentPrepareForm triggered in com_menus.menu ## Menu Create Before:
- No event After:
- onContentBeforeSave triggered in com_menus.menu
- onContentAfterSave triggered in com_menus.menu ## Menu Update Before:
- No event After:
- onContentBeforeSave triggered in com_menus.menu
- onContentAfterSave triggered in com_menus.menu ## Menu Delete Before:
- No event After:
- onContentBeforeDelete triggered in com_menus.menu
- onContentAfterDelete triggered in com_menus.menu
Menu-Items
Menu-Item Browse:
Before:
- onContentPrepareForm triggered in com_menus.items.filter After:
- onContentPrepareForm triggered in com_menus.items.filter ## Menu-Item Read Before:
- onContentPrepareForm triggered in com_menus.item
- onContentPrepareForm triggered in com_menus.items.filter ## Menu-Item Create Before:
- No event After:
- onContentBeforeSave triggered in com_menus.item ## Menu-Item Update Before:
- No event After:
- onContentBeforeSave triggered in com_menus.item ## Menu-Item Delete Before:
- onContentChangeState triggered in com_menus.item After:
- onContentChangeState triggered in com_menus.item
Template Styles
Template Style Browse:
Before:
- No event After:
- No event ## Template Style Read Before:
- onContentPrepareForm triggered in com_templates.style After:
- onContentPrepareForm triggered in com_templates.style
- onContentPrepareData triggered in com_templates.style ## Template Style Create Before:
- onExtensionBeforeSave triggered in com_templates.style
- onExtensionAfterSave triggered in com_templates.style After:
- onExtensionBeforeSave triggered in com_templates.style
- onExtensionAfterSave triggered in com_templates.style ## Template Style Update Before:
- onExtensionBeforeSave triggered in com_templates.style
- onExtensionAfterSave triggered in com_templates.style After:
- onExtensionBeforeSave triggered in com_templates.style
- onExtensionAfterSave triggered in com_templates.style ## Template Style Delete Before:
- No event After:
- onContentBeforeDelete triggered in com_templates.style
- onContentAfterDelete triggered in com_templates.style
Modules
Module Browse:
Before:
- No event After:
- No event ## Module Read Before:
- onContentPrepareData triggered in com_modules.module
- onContentPrepareForm triggered in com_modules.module After:
- onContentPrepareData triggered in com_modules.module
- onContentPrepareForm triggered in com_modules.module ## Module Create Before:
- onContentPrepareForm triggered in com_modules.module
- onExtensionBeforeSave triggered in com_modules.module
- onExtensionAfterSave triggered in com_modules.module After:
- onContentPrepareForm triggered in com_modules.module
- onExtensionBeforeSave triggered in com_modules.module
- onExtensionAfterSave triggered in com_modules.module ## Module Update Before:
- onContentPrepareForm triggered in com_modules.module
- onExtensionBeforeSave triggered in com_modules.module
- onExtensionAfterSave triggered in com_modules.module After:
- onContentPrepareForm triggered in com_modules.module
- onExtensionBeforeSave triggered in com_modules.module
- onExtensionAfterSave triggered in com_modules.module ## Module Delete Before:
- onContentChangeState triggered in com_modules.module After:
- onContentChangeState triggered in com_modules.module
Articles / Content
Content Browse
Before:
- onContentPrepareForm triggered in com_content.articles.filter After:
- onContentPrepareForm triggered in com_content.articles.filter ## Content Read Before:
- onContentPrepareForm triggered in com_content.article
- onContentPrepareData triggered in com_content.article After:
- onContentPrepareForm triggered in com_content.article
- onContentPrepareData triggered in com_content.article ## Content Create Before:
- onContentBeforeSave triggered in com_content.article
- onContentAfterSave triggered in com_content.article
- onContentPrepare triggered in com_finder.indexer
- onContentPrepare triggered in com_finder.indexer After:
- onContentBeforeSave triggered in com_content.article
- onContentAfterSave triggered in com_content.article
- onContentPrepare triggered in com_finder.indexer
- onContentPrepare triggered in com_finder.indexer ## Content Update Before:
- onContentBeforeSave triggered in com_content.article
- onContentAfterSave triggered in com_content.article After:
- onContentBeforeSave triggered in com_content.article
- onContentAfterSave triggered in com_content.article ## Content Delete Before:
- onContentChangeState triggered in com_content.article
- onContentPrepare triggered in com_finder.indexer After:
- onContentChangeState triggered in com_content.article
- onContentPrepare triggered in com_finder.indexer
Users
User Browse
Before:
- onContentPrepareForm triggered in com_users.users.default.filter After:
- onContentPrepareForm triggered in com_users.users.default.filter ## User Read Before:
- onContentPrepareData triggered in com_users.profile
- onContentPrepareForm triggered in com_users.user
- onContentPrepareData triggered in com_users.user After:
- onContentPrepareData triggered in com_users.profile
- onContentPrepareForm triggered in com_users.user
- onContentPrepareData triggered in com_users.user ## User Create Before:
- onUserBeforeSave triggered in onUserBeforeSave
- onUserAfterSave triggered in onUserAfterSave After:
- onUserBeforeSave triggered in onUserBeforeSave
- onUserAfterSave triggered in onUserAfterSave ## User Update Before:
- onUserBeforeSave triggered in onUserBeforeSave
- onUserAfterSave triggered in onUserAfterSave After:
- onUserBeforeSave triggered in onUserBeforeSave
- onUserAfterSave triggered in onUserAfterSave ## User Delete Before:
- onUserBeforeDelete triggered in onUserBeforeDelete
- onUserAfterDelete triggered in onUserAfterDelete After:
- onUserBeforeDelete triggered in onUserBeforeDelete
- onUserAfterDelete triggered in onUserAfterDelete
@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
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4308.
| Status | Pending | ⇒ | Ready to Commit |
| Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-11-08 12:31:26 |
Merged, thanks!
The conflict was a small codestyle issue. Resolved that myself.
Congrats guys! :)
@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.
resolve joomlatools/joomla-fork#2
This looks great, thanks for that work!
Let's get that tested