Moved the new JObserverMapper for ContentHistory as well out of cms.php to the tables.

Please review the Observer Pattern in any serious pattern computer-science book or lesson:

The link between the Subject (Observed) and the Observer must be done outside both classes for this pattern. That's exactly the interest of it: Both are not aware what is being observed and by whom!!!

I'm not opposed to move those CMS-specific lines of code outside of cms.php into another file that does only this or in a better place, but without adding files, I have not found a better place.

Thus this pull request should be rejected, as it ruins the whole interest of the Observer pattern, Sorry.

Speaking purely as an extension developer here, as long as the CMS ships with loading the observers on EVERY page load in a single file, I'll never implement the code. I don't know where I would implement it otherwise as the observers only need to be active in certain events, namely those with interfaces to JTable from my understanding. Based on the current model, I'd have to either hack the centrally located file to add my data (big no-no) or add a system plugin that adds the observers (subject to user turning them off).

A solution needs to be sought for this. The CMS needs to be a model for extension developers; this current practice is not a suitable model. And suggesting every time that we try to move this code that the relevant patch be rejected is not helping matters.

@Beat I surely understand where you are coming from and understand the basics and advantages of this pattern.

However in our case, we don't need the benefits of the decoupling at all. Or are there benefits of the decoupling I don't see yet? Then please explain them.

I would be happy to put the observer mapping into the controller or any other file of the extension, but unfortunately there is no single place where it could be placed. Especially given that the tables are directly used by other extensions as well. The observer mapping would have to be done in each and every extension and class/function which directly accesses the table, not something that sounds like good programming style. The only place which always gets executed for sure if a table is loaded is its constructor. Even the current cms.php is not save as it's not necessary loaded from CLI scripts.
That's my main point and imho this weights much more than a theoretical concept how the observer pattern should (not must!) be included.

Also it doesn't help to create a new file to put all this observer mappings, as this file would as well be run on each pageload and load the mappings for each extension. Which is my second point: The mapping is only used in case of editing. It's never used just for displaying something, the table isn't involved here as well. Currently we are going to execute code for nothing, just to please the concept of observer patterns. That's also not good coding style imho.

I'm open for suggestions for a better place where to put it, but unfortunately I don't see one. Neither for core extensions nor for 3rd party extensions.

Since the current place is worse than not following a concept, I'm still for making that change :-)

@mbabker agreed (remember I'm also developing extensions, so pretty much on both sides like you).
As a system plugin seems not to be the proper solution for you, and that the Controller seems to not be a good solution for @Bakual , I will propose a new FR with a clean solution suitable for extensions (other than a system plugin as those may not be suitable always... In my extensions which require a plugin, I'm checking for them to be published and display a red warning on top of all correspoinding admin pages) for that as soon as I'm done with the 3.2 JUX part and before 3.2 RC :-)

@Bakual Understand from where you're comming from, but let's do something that is better than cms.php without ruining one of the big advantage of Observers (maybe even use them for that!) ;-)

@beat Just wondering: What do you see as the big advantage of Observers in the case of JTags and ContentHistory which would be gone when the mapping would be moved to the tables? Or rephrased: What is the benefit from decoupling in those two cases?

Maybe we can also use this PR to get this issue solved and if Beat proposes a better solution someday, we can still merge his approach.

I just want to get that stuff out of cms.php before even more is added there and so extension developers can start finding it.

I'm with @Bakual , let's solve this. To the extent that these are really just hooks the concept is fine (as we know I originally implemented tags with a post save hook and functionally all we really ended up doing is now adding a post store() hook). This is easy to solve so let's make it simple ,flexible (i.e. I can add options by overriding the constructor) and usable for everyone and only called when needed. For example adding tagging or versioning to a core table that is not using tags in the core (such as modules or users to name two) should be as simple as overriding the constructor. That's part of the whole point of how tagging and versioning are managed centrally.
We need to deal with the fact that JTable::store() and other methods (delete, copy) are called from many contexts and those contexts change how the tags and other apis should behave... this is one reason we moved to having tags handling in the table, to provide consistency whenever there was a store() or delete() or delete a tag or tag/untag item from any source. Right now we unfortunately are not differentiating where a store() comes from (batch, saveorder, edit, something else) but I think by having this in the constructor we can have flexibility in passing in context information when appropriate.
The whole point of not saving in metadata was to make an api that was usable for extension developers and people working with the core in complex ways.

Agreed. I had trouble explaining to people at JDay UK the other day how they were supposed to do one thing in their extensions and core was doing something completely different. I think we really need to get this fixed before 3.2 ships!

Updated with master, resolved conflict due to four new mapping calls in cms.php and moved those four calls to their respective tables. Those are:

• banners.banner
• banners.client
• tags.tag
• users.note

Beat is right in the sense this should be done outside the respective classes. But in this case they have to go inside the extensions (thinking ahead to JLite). So I definitely am giving the to this!

We can always move the calls again if someone comes up with a better plan.
I don't say the tables are the best place, but it's better than cms.php for sure :-)

For now this is the best I can think of and I don't see any disadvantages yet. Plus nobody proposed a better solution yet.

Actually, I have answered a user question on the subject of Observers in extensions here: #2050 : Maybe that can help in some cases.

Let's solve this properly. Now that 3.2 beta 1 is almost done, I have started designing the missing class ObserversDefinitions for a central storage to get all ObserverMappings at load time quickly, and hope to do a PR for that in time for 3.2.0 beta 3, hopefully beta 2.

Once that is done, the Observer Pattern will simplify many aspects and help to remove many dependancies, opening the road to optional extensions and libraries that Joomla has in its plans.

I'm also in touch with the Joomla UCM workgroup regarding this design, to make sure it matches well the next big step of Joomla :-) .

So please don't merge the solution above, as it would make the move to the new ObserversDefinitions mappers-storage in a backwards-compatible way impossible and add to the Joomla library API a supposition of a hard-link that should not be made. Thanks! Final solution will be ready to be proposed for Joomla 3.2.0 RC stable.

Thanks!

So please don't merge the solution above, as it would make the move to the new ObserversDefinitions mappers-storage in a backwards-compatible way impossible and add to the Joomla library API a supposition of a hard-link that should not be made.

Actually, your future solution will already have to be backward compatible with a mapping done in the table constructor. Because I'm sure extensions already use this as it's currently the best way available for 3rd party extensions (beside providing a system plugin).
In fact the current comment within https://github.com/joomla/joomla-cms/blob/master/libraries/legacy/table/content.php, (written by yourself?) references this place as a possible variant:

    /*
     * This is left here for reference:
     *
     * This would set up the tags observer in $this from here (so not entirely decoupled):
     * JTableObserverTags::createObserver($this, array('typeAlias' => 'com_content.article'));
     *
     * But this makes the relation between content and tags completely external to Content as JTable is observable:
     * So we are doing this only once in libraries/cms.php:
     * JObserverFactory::addObserverClassToClass('JTableObserverTags', 'JTableContent', array('typeAlias' => 'com_content.article'));
     */

So while I sure am looking forward for your proposed solution, I think that it would still make sense to merge this till your solution is ready.
More and more extension developers will search for guidance on how to use Tags and now especially History. They are looking at the first beta to get their extensions ready for release of 3.2 so it will support the new history feature (which is the main feature of 3.2). We should have shown them a possible way months ago already.

A question on your idea for the ObserverDefinition class. You wrote

class ObserversDefinitions for a central storage to get all ObserverMappings at load time quickly,

Does this mean Joomla will load all mappings for each extensions at load time, doesn't matter if they are needed or not? Like today with cms.php? Or will that be a selective loading when needed?
Just asking because we currently have 14 mappings for core alone, and I know my extension will add at least 6 as well. That could add up a lot fast with future features and other installed extensions. So I hope it's not the case and it will be loaded selectively only when needed (eg only when the table is loaded). That would be awesome then, especially when we could use the extension manifest file or a defined file in the extension directory (similar to /helpers/category.php, helpers/association.php, router.php and the like) for the definitions.

Merged upstream master

@beat I want to try and get this solved before 3.3. I'll test this later if you have nothing on the table. because it's unfair on thomas to sit here waiting when there are no other proposals :)

As developped during JAB14 after discussion with @Bakual and @mbabker and mentioned in corresponding commit 2248368#commitcomment-6609826 , here is my alternate PR proposal: #3753