I love this concept. As a developer it unties my hands in many cases. I'd rather see this being implemented in the base model class (like Koowa does – honestly, I liked the concept so much that I've been using it in my own code) but since Joomla!'s code involves a lot of copying/pasting instead of extending what you did is the best solution with backwards compatibility in mind. Thank you!

Awesome! Thanks for submitting this Joomlatools Team!

Great! Thanks for reviewing the code, Nicholas!

We'll try to get some testers this during PBF this Friday, if someone doesn't test before then.

Cheers,
Nick

Update : Patch will generate merge conflicts due to changes made in
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=29769

We are happy to change the patch to acommodate for this but we propose to consider reverting the changes as our patch solves the same problem without the introduction of two new events (onConfigurationBeforeSave, onConfigurationAfterSave) The patch uses onExtensionBefore/AfterSave instead.

The Joomla event system allows to pass context, which reduces the need to create separate events. We have taken alot of care when creating this patch to make it fully BC and not introduce any extra event types. This approach results in more flexibility, more consistency and less documentation.

I agree with Johan that the most generic plugin events are more desirable than onConfigurationBeforeSave/...AfterSave. I believe the other patch's implementation was the result of bad advice from people who really ought to have known better. Actually, that's a certainty, not belief, as I happen to have discussed this patch with its author a few weeks ago. The proposed implementation by JoomlaTools is very elegant, as it provides the context (component and view), a reference to the table being saved and whether this is a new or an existing record. That's a universal, elegant solution.

I can see two ways to resolve this:
1. The other patch must be rolled back before a new Joomla! 2.5 release is made and the proposed implementation is applied instead.
2. The other patch stays as it is and the proposed implementation is only included in Joomla! 3.

I would rather see the first one being implemented. Developers working on Joomla! 2.5 will greatly benefit from this added functionality. Nick, is it possible to revert the other patch?

Yes, it is, but it's preferable to have this pull request revert it at the same time. Johan and his team are going to do that soon.

Perfect!

Hello guys,

I've just merged from remote and re-applied our solution for the com_config feature on top of it. This will effectively revert the current solution (http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=29769) while leaving ours in case of a pull. I'm also attaching a new patch in the tracker.

Cheers,

Arunas

Additional comments on how to test.

Unit tests

The patch doesn't implement a new unit of code against which can be tested. Instead it implements an existing API in a consistent way across all core Joomla components.

The existing unit test for the Joomla event system will apply: https://github.com/joomla/joomla-cms/tree/2.5.x/tests/unit/suite/libraries/joomla/event.

Test Instructions

The code doesn't have any impact on existing functionality. It adds to it. To test it, a plugin can be written that intercepts all the onExtensionX, and onContentX events. After the patch is applied all core extensions will fire these events correctly and consistently.

Guys - couple of concerns.

The idea of not creating additional events makes sense, agree with that. But, the danger using existing events in new locations is that existing plugins that are already installed could fire if you add an existing event at an existing location. Essentially, that changes the behavior and creates an API change, backported to 2.5, that's an API change in stable release.

I suspect that's the reason @eddieajau suggested new names for events? In adding names, you can't possibly impact any existing behavior by already installed plugins.

Agree that 'context' could be used to prevent the existing plugins from firing -- but if plugins were written given the existing implementation, then there would have been no need to check for this use because it wasn't possible. Again, this really is an API change.

Will it create problems? I have no idea, but there are good reasons to use new labels. Would have been better to fully implement the API back in 1.6, but it is what it is now. Best to get Andrew's quick review -- see if these new ideas seem appropriate to him or if there was more behind his suggestion than the short comment might suggest.

Didn't look at the rest of the patch, I am only talking about the event names.

Amy, onContentBeforeSave/onContentAfterSave is already used not only in com_media but also in all core and third party extensions which need to let Finder know of an item addition/modification. I'm pretty sure @eddieajau is fully aware of this. If a developer is handling this event in his plugin without checking the context then he has to fix his code as it's already incompatible with Joomla! 2.5 or later. The JoomlaTools guys are relying on these pre-existing events without altering their meaning or creating backwards incompatibility issues.

For what is worth, yes, it may not be backwards compatible with Joomla! 1.6/1.7 only plugins, but let's face it, these J! versions are long dead. This solution is fully compatible with existing Joomla! 2.5 / 3.0 plugins. Do you think of any use case where BC might be broken?

LTS stable means developers shouldn't have to change their code to update to a bugfix release, that's not how semantic versioning nor the Joomla Release Cycle works. Implementing the hooks with the new names means there isn't a potential API change in the LTS bugfix release.

That said, I'm for consistency, so I do like the proposed patch here. If it can be implemented and guaranteed not to break things, then go for it. But as I said, LTS bugfixes shouldn't break existing code.

Regardless, I'm all for doing this right, with this patch, in 3.x.

So Nic, if what you say is true (no reason to doubt it's not) that it doesn't break 2.5.x code, then I'd say use this patch.

@nikosdion Speaking about Extension save events, instead of Configuration save events, not Content.

From #757 (comment) and your response #757 (comment)

Right now, the Extension events do not fire in this location. If new labels are used, as Andrew suggested, (like Configuration), then behavior will not change (no API change, just addition).

But if an existing event label is used in a new location, then the existing behavior (plugins do not fire there for Extension save events) would change (plugins do fire there for Extension save events), and that is an API change.

While it's true context is available to restrict plugins from firing, this condition could not happen before, therefore, developers would have never known to restrict their plugins in this case.

My comment on 1.6 referred only to the original posters comment about the API not being fully implemented. Had it been implemented in 1.6, this consistency would have been possible. But, now, gotta be careful with introducing behavior changes, especially in 2.5 which should be stable. Don't want plugins firing off that were not doing so before and are not intended to do so.

I'm guessing that's why ( ? ), Andrew suggested new labels. Make more sense?

In Joomla! 2.5, onExtensionAfterSave is used in the following places (I did a quick grep and examined the files):

administrator/components/com_languages/models/language.php
administrator/components/com_modules/models/module.php
administrator/components/com_templates/models/source.php
administrator/components/com_templates/models/style.php

In all cases there is a context passed on in the format com_whatever.viewname, with the following two arguments being &$table and $isNew. As a result if any developer wishes to call his onExtension{Before|After}Save handling plugin compatible with Joomla! 2.5 he/she MUST be aware of the context. Otherwise the plugin is not implementing the Joomla! API and breaks the site. Ergo this comment of yours, Amy, is inaccurate:

While it's true context is available to restrict plugins from firing, this condition could not happen before, therefore, developers would have never known to restrict their plugins in this case.

No. They would have known because it is already happening in Joomla! 2.5. Yes, Joomla! 1.6 and 1.7 plugins won't work. They needed to be updated for Joomla! 2.5 even before the application of this merge request. But if anybody has a plugin handling these events which actually does work properly on Joomla! 2.5 he's already aware of the context.

Anyway, as I already said, JoomlaTools' solution is following exactly this existing convention. It's not replacing or extending functionality beyond its current scope. It merely adds more contexts which can result in this plugin event being called. But since developers must already be handling the context there is no backwards compatibility issue, at least none that I can see and none that's related to your concerns/reservations.

A generic remark for all of us, if you please allow me. If we honestly want to encourage developers to contribute code to the CMS and the Joomla! project in general we should not shoot down their contribution without solid arguments, based on feeling and/or speculation. Please, let's provide arguments the developers can actually respond to. I've been on both sides of situations like this and, yes, I have sinned (I'm sure Seth and Cristina know first-hand what I mean). Feel free to toss this advice away if it frustrates you / you don't agree.

@nikosdion Understood that core is passing in a context. Not my concern. The problem is that there is no requirement that context must be used within the plugin, and many times it is not. For that reason, it is possible existing plugins could fire that never did before.

I agree with the @amazeika in the issue report -- it would have been good to have fully implemented the API, but such is not the case, so here we are. Going back to 2.5 and adding existing events at locations those events do not exist creates a behavior change that could cause plugins to fire that did not fire before, and that's the basis of an API change, right? That's just a fact.

The only way to prevent that possibility is to create new labels. Am I missing something?

I'm not sure how raising that point would be discouraging, especially to Johan and the team, they know I have nothing but respect for who they are and their abilities and they are well known for sticking to the code and keeping focus on those points. That's the advise we need to remember. Just because we might disagree on a technical issue, it's OK, in fact, it should be considered good - together we are stronger.

Doesn't frustrate me, and I understand (and appreciate) the concern, but we should keep tracker discussions focused.

The problem is that there is no requirement that context must be used within the plugin, and many times it is not.

This is false. Please take a look at the code, Amy. Whenever the aforementioned plugin events are called a context is ALWAYS passed. There is simply no conceivable way that you can ever write a plugin which handles those events without it being aware of the context or at the very least type-check the $table variable. If a plugin works with Joomla! 2.5 as of yesterday it will still work after this patch is applied.

Going back to 2.5 and adding existing events at locations those events do not exist creates a behavior change that could cause plugins to fire that did not fire before, and that's the basis of an API change, right? That's just a fact.

No, Amy, that's your understanding without reading the code. If you please do read Joomla! 2.5's code you will see that you're wrong in your assumption. Whenever these plugin events are called there is already $context being passed. Amy, I am talking based on experience. I am currently working on two plugins (content and system) which will allow the integrated Joomla! extensions updater to update my paid software which requires an extra query parameter to be appended to the download URL. It so happens that I want my plugin to be notified whenever it's being unpublished so that it can remove the update_sites records (if the plugin is unpublished the update URLs will no longer be valid). I am actually using the onExtensionBeforeSave event to do that and I do have to check the $context variable, otherwise I'm breaking the site whenever I add or edit an article. That's not speculation, that's actual code I am running on an actual web server with the current Joomla! version. Yes, I am working against Joomla! 2.5.9, without Johan's patch. Your assumption is wrong, I don't how else to put it.

Amy, I honestly believe that you are missing something. You didn't read Joomla!'s code. Please do. You'll see what I mean. It will only take 5-10 minutes tops.

So, for the third time: is there a specific use case of an existing, valid and functional plugin for Joomla! 2.5 which does not use the context and would break by this patch? If there is, provide the code which proves your point. We can't ask a developer to put his patch on hold for another year just because we have a hunch that it may or may not cause an API change which may or may not break plugins. It's a development (=practical & concrete) issue, not a philosophical (=speculative & abstract) one.

We need to make sure the JCA is signed before we can merge this PR. Not sure if you need to do the corporate version or the individual version. Thanks!

Nick - of course I review the code, you aren't following, it has nothing to
do with the calling controller, model, or the dispatcher. Yes, context is
sent into each plugin. The problem is - not all plugins check the value in
context. This patch adds a dozen or more new event schedules where none
existed before. If existing event names are used, it is possible plugins
would run that did not run before - and it's possible that's not intended.

Mark - don't mean to create trouble, but the rules should be consistent. I
understood small patches like this did not require the agreement be signed.
Or, are you now requiring everyone do so?

Thanks - and I am done. I don't use the CMS. Take my comment for what it's
worth.

On Sat, Mar 16, 2013 at 4:42 PM, Mark Dexter notifications@github.comwrote:

We need to make sure the JCA is signed before we can merge this PR. Not
sure if you need to do the corporate version or the individual version.
Thanks!

—
Reply to this email directly or view it on GitHub#757 (comment)
.

Amy, this particular patch only causes a few specific events to be called. It doesn't add $context to all existing plugin events. Those specific few events already had $context in them and developers really needed to handle it already. The only difference made with this plugin is that these pre-existing plugin events are called from more components. So I still don't follow you as to what the problem is.

Moreover, there's an important thing we overlooked in our discussion. The new events are solely called in the new "extension" plugin type. There are no "extension" type plugins in Joomla! right now so there's nothing to be called accidentally. As for the pre-existing plugins (which are of the "content" type) I see that there is special care taken to call them in the exact same places they are being called by Joomla! 2.5.9. What could happen is an "extension" plugin being called together with "content" plugins, but since the "extension" plugin type is new this isn't a problem as far as I can understand.

It's not a matter of you using the CMS or not. You raised a development issue about API usage. I understand why you are worried, but I can't see how what you fear can possibly happen. Please, can you give us an example of a plugin which will break after this patch is applied?

@nikosdion Not worried. Not interested in arguing with you. Just wanted to point out a concern and since I have done so many times now, I'm out of interest and time to continue =). There really isn't a good answer on these. I don't see a great way to move forward. If it were up to me, I'd probably wait until 3.0 since 2.5's API shouldn't change. You don't agree, I respect that. Sometimes these things are not very clear.

@johanjanssens - @dextercowley is right - there is nothing wrong with getting the agreement signed. In fact, you have all nooku authors sign an agreement before you accept their code, so what's the big deal? In this case given how it's signed the Joomlaworks team it really should be an agreement. Time for everyone to be cooperative. No reason for us not to be. If you want to share code, do like I did, sign the agreement. You are contributing the code - so why wouldn't you sign it? Right?

Would be great if we can have phpcs run over the patch and bring all the modified files up to the current standard.
https://github.com/joomla/coding-standards

It's one of our goals for the CMS for this year and there are a few obvious off-style changes that need to be reverted so it's just better to let phpcs do the talking.
http://developer.joomla.org/news/552-production-goals-for-2013.html (see 2.2)

Thanks in advance.

Thanks all!

Looks like according to http://community.joomla.org/blogs/leadership/1119-rfc-joomla-contributor-agreement.html , the JCA would not be required in this case (unless there is additional code in the codebase of which I'm not aware).

As to code style, we haven't been enforcing it yet, though we should make plans to do that soon. Until then, this can be merged without styling changes (though code style clean up would be preferred) and styling can be corrected after being merged.

Looks like we just need a few more tests (not unit tests but rather tests by individuals) of the actual pull request.

Thanks again everyone!

Kind regards,
Nick

Initially, I thought that too, Nick, but when I looked at the submission
again, it included the company name JoomlaTools, So, wasn't so sure then
since other companies have contributed less (sadly) but have been asked to
sign.

I know nothing and I am sure you talked to Mark and OSM so, just explaining
my comment, it has no authority. =)

On Wed, Mar 20, 2013 at 2:03 PM, Nick Savov notifications@github.comwrote:

Thanks all!

Looks like according to
http://community.joomla.org/blogs/leadership/1119-rfc-joomla-contributor-agreement.html, the JCA would not be required in this case (unless there is additional
code in the codebase).

As to code style, we haven't been enforcing it yet, though we should make
plans to do that soon. Until then, this can be merged without changes
(thoough code style clean up would be preferred) and styling can be
corrected after being merged.

Looks like we just need a few more tests (not unit tests but rather tests
by individuals) of the actual pull request.

Thanks again everyone!

Kind regards,
Nick

—
Reply to this email directly or view it on GitHub#757 (comment)
.

Nick, how does that explain why the JCA wouldn't be required? If this is a contribution on behalf of Joomlatools, it 100% is required.

@AmyStephen Thanks for your contribution in this discussion, Amy! :) Give yourself more credit; you know a lot :)

As to your concern, I was just going based off of precedent (I'm not aware of other companies that have been required to for less code), the blog post that I quoted, and the PLT Skype chat (disclosure: for the Skype chat, usually, not everyone provides feedback on a topic). I have not talked to OSM about it nor do I see the need to (based on precedent and that blog post). If you feel that the JCA needs to be discussed further, please open up a discussion in the JCMS mailinglist or ping me privately, as this is not a good place for it. We can always leave a summary here after discussion, if necessary.

@dongilbert Thanks for addressing your concern! Within the blog post it states:

As a compromise, the Joomla! Project requires a JCA from anybody who makes a significant contribution to Joomla! or any other OSM project. "Significant" is, of course, a judgment call. As a guideline, if you have more than 500 lines of code in the codebase, we need a JCA. Additionally, to be granted commit access to our source code repository, we will need a JCA."

I don't see this pull request as being significant nor do I see it has having more than 500 lines of code.

Also, the way the blog post is written, the above requirements apply to General, Corporate, and Minor JCA forms. There are not additional requirements for businesses. For example, as stated in the blog:

If you work for a company (or organization) that has any copyright claim over work you produce, the corporate edition is for you.

If you feel that the JCA needs to be discussed further, please open up a discussion in the JCMS mailinglist or ping me privately, as this is not a good place for it. We can always leave a summary here after discussion, if necessary.

@ all,
We're doing our best to make code contribution a friendly and welcoming process. Please keep that in mind when commenting on people's work that they have labored to submit to the project out of their good intentions. Thanks in advance! If you have general questions about processes, please ask them on the JCMS mailinglist.

Thanks again everyone! Hope you're all having a great day! Enjoy it :)

Andrew added this to the framework repo today, so I was assuming he pulled
the text from an existing ruleset:

"If you are contributing as an employee of a company then we need a JCA
with your company no matter how small the contribution is."

If that's not the case, then I'm sorry for misunderstanding.

On Wed, Mar 20, 2013 at 2:45 PM, Nick Savov notifications@github.comwrote:

@AmyStephen https://github.com/AmyStephen Thanks for your contribution
in this discussion, Amy! :) Give yourself more credit; you know a lot :)

As to your concern, I was just going based off of precedent (I'm not aware
of other companies that have been required to for less code), the blog post
that I quoted, and the PLT Skype chat (disclosure: for the Skype chat,
usually, not everyone provides feedback on a topic). I have not talked to
OSM about it nor do I see the need to (based on precedent and that blog
post). If you feel that the JCA needs to be discussed further, please open
up a discussion in the JCMS mailinglist or ping me privately, as this is
not a good place for it. We can always leave a summary here after
discussion.

@dongilbert https://github.com/dongilbert Thanks for addressing your
concern! Within the blog post it states:

As a compromise, the Joomla! Project requires a JCA from anybody who makes
a significant contribution to >Joomla! or any other OSM project.
"Significant" is, of course, a judgment call. As a guideline, if you have
more >than 500 lines of code in the codebase, we need a JCA. Additionally,
to be granted commit access to our source >code repository, we will need a
JCA."

I don't see this pull request as being significant nor do I see it has
having more than 500 lines of code.

Also, the way the blog post is written, the above requirements apply to
General, Corporate, and Minor JCA forms. There are not additional
requirements for businesses. For example, as stated in the blog:

If you work for a company (or organization) that has any copyright claim
over work you produce, the corporate >edition is for you.

If you feel that the JCA needs to be discussed further, please open up a
discussion in the JCMS mailinglist or ping me privately, as this is not a
good place for it. We can always leave a summary here after discussion.

@ all,
We're doing our best to make code contribution a friendly and welcoming
process. Please keep that in mind when commenting on people's work that
they have labored to submit to the project out of their good intentions.
Thanks in advance! If you have general questions about processes, please
ask them on the JCMS mailinglist.

Thanks again everyone! Hope you're all having a great day! Enjoy it :)

—
Reply to this email directly or view it on GitHub#757 (comment)
.

@dongilbert that's for the Framework (and just plain common sense in my opinion).

I still think you should not make the code style "worse" on existing code (which this PR does), but it's not my call. We used to take great pride, as a project, in how our code looked. I really pine for those days.

On Wed, Mar 20, 2013 at 4:14 PM, Andrew Eddie notifications@github.comwrote:

I really pine for those days.

Got a lot of really good memories of my own from the early days of Joomla!
where I first discovered open source and found my first open source heroes
-- I used to pine for those days, as well. =)

Now, I am looking forward to seeing all this code intermingling again in
the broader open source world. Heading into better times, lots of the same
leaders.

Apologies for off-topic remarks. =)

Just bought a extension where this pull request would make it more useful. Is this going to be implemented in Joomla 3.2? It seems quiet here after teh discussion. I'm not a programmer. But I can test it ;)

@amazeika Can you create a new PR with this changes rebased and targeted against staging?
We can't really merge this into the 2.5.x branch but I would be interested in having this in 3.x.

Closing in favor of #4308