[#1561] - Fix #31339 (Tags backwards-compatibility issue) through #31488 (a clean observer implmentation in JTable)
- Closed
- 21 Jul 2013
- Medium
- Build: master
- # 1561
- Diff
- beat:observerpattern
- Foreign ID 31339
- Success default The Travis CI build passed Details
User tests: Successful: Unsuccessful:
This pull request is an alternative implementation to the fix proposed in pull request 1452 ( #1452 which was refered in #31339).
One line that shows the big advantage of this proposal easily understandable:
JObserverMapper::addObserverClassToClass('JTableObserverTags', 'JTableContent', array('typeAlias' => 'com_content.article'));is all what is needed to add the Tags feature to a JTableContent. Neither JTableContent nor JTable have after this PR any tags-specific code. This line is outside those 2 classes (here in cms.php), and such lines can be used to add tags to any class!
Detailed explanation of its benefits here for what it fixes:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=31339
and here for its general advantages:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=31488
Hi Michael,
Yes, we can of course put those calls elsewhere than in cms.php . A proposal for the tags as they are now ?
Actually I like your good feedback, and indeed it can be useful to remove observers mapping rules, I have pushed a small change so that this JObserverMapper::addObserverClassToClass that allows with false for params to remove entries. That way a system plugin could remove existing core or extensions mappings, independantly of where the mapping is made. Does that address your concern ?
EDIT:
To further reply: Extensions can use that function anywhere to add observers's mappers (as long as it's before observable objects that need to be observed are created). E.g. in system plugins is fine, but it can be elsewhere.
Otherwise, there is also the method attachObserver() defined in ObservableInterface where an ObserverInterface object can add itself as observer anytime, after the Observable object is created.
What happens to extensions that have already implemented tags? Are you positive that they still work exactly as before with no changes paralleling the changes you have made in the core extensions? What if they have non standard implementations?
Hi Elin,
Thanks for your very valid question. As you know, I'm very cautious with backwards compatibility. :-)
I am certain that this pull request is not changing the current standard tags API:
- There are no changes to the API of tags. All public variables and public methods are unchanged.
- All significant lines of code that were in JTableContent (and other JTables that support tags) is still executed at same time in the load, store and delete methods, just it's not located anymore in JTableContent (and other JTables) anymore, but still executed at same place (thanks to generic Observer updates in JTable which replace the tags-specific code repeated in all JTableContent and others), simply it's in the JTableObserverTags class instead of hardcoded into the JTable class.
- JTableSomethings that use the current hardcoding method without observers are not affected, as they are not observed by default, not being observers-mapped.
I don't know what you mean by "non standard implementations", but I would really suggest to not bother about people who do not use the standard API or missuse or workaround its intended use. In any case, as I didn't change significant code or variables, I honestly don't see what kind of "non standard" use would be broken.
That said, the whole implementation of tags in extensions could be made much simpler with the observer pattern introduced by this pull request (e.g. same as done with JTable, we could add implements JObservableInterface to JModel and move out any tags-specifc and other specific code from JModel). But as I didn't want to change anything to the standard (and non-standard) API of tags, I didn't refactor any other part of tags except of the JTables part. Other changes include fixes on PHPdocs, replacement of non-standard access to protected variables by standard accesses, and bug fixes.
Another example of JObservableInterface potential use: Looking tonight at the copy-pasted debug-code which is in the various database driver classes, I see that if those database drivers were observable, we could simply observe them from the debug plugin, and move the debugging-specific code to where it belongs (in the debug plugin). It just makes things much cleaner, reusable and self-contained. :-)
Let me add that, as Michael states in the tracker http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=31339 , he just had to correct the code for the metadata field storing (unrelated to this PR) for his extension to work as it did in 3.1.1.
I guess any other 3pd implementing tags would anyway have to do the same.
The issue actually isn't related to Beat's patch, but rather the change to not store tag data in the metadata column. To get my own extension to work with tags for 3.1.0 through master, I made this commit - BabDev/Podcast-Manager@8f2711b
Thanks both of you :-)
Btw I would strongly suggest to move the JObserverMapper stuff into the extensions so 3rd party developers like me can find a reference. cms.php really is a bad place.
I'm still not sure where it's supposed to go. The table itself seems to be the wrong place?
@beat I tried to move the ObserverMapper calls from the cms.php to the tables, but it fails early since JTable doesn't find JObservableInterface. The autoloading fails here because the mapper no longer is loaded before the table in the cms.php.
Can you make your classes in separate files (like the standard is) and is there a reason why the JObserverMapper need to be decoupled from the table? Imho they should be coupled.
@Bakual As commented on your pull request:
This is not a good idea, as it adds a direct class dependency, and does not allow the independency of classes. The dependency has been done that way, so that there are NO dependencies. E.g. JTableSomething does not have to know which Tags are implemented.
Please study phpdoc blocks for the proper use of independant injection of Observers.
You can of course do how you proposed for your own extensions, but that removes the beautiful complete independence of injected observers. Please search web for SOLID and DRY as well as PHP Patterns.
If you need to add an observer in your extension, you can do that in your controller, a system-plugin or anywhere you see fit, just adding a line there similar to those for the CMS (which are imho belonging to cms.php), No need to edit CMS.php. Additionally, if your extension implements other tags classes, you can call same function to unregister the core observers (not recommended unless you know what you are doing).
@beat -
First, thank you for this work. It is fabulous - what I would hope for.
I expect JObservableInterface to become a dependency. It meets the SOLID test just fine. (Probably could have even simply used the existing Event Content Prepare but this is the new direction, better approach.)
Adding a dependency isn't bad, in and of itself, or, we wouldn't have separation of concern. Gotta bring things together - what an interface/event approach enabled was removing dependencies on specific tag implementations, and paved the way to do the same with ACL,hit counts, and so on.
I agree with @Bakual that core should provide a blueprint for extensions as to what developers must do to implement the same type of process. Since it's not possible to use the cms.php file for extensions, it really shouldn't be used by core (IMHO).
I studied the patch, read the docblocks, but the documentation is pretty generic in that it explains how to add this same type of capability. What I couldn't find (and very possibly just missed) was instruction on where this line should go:
JObserverMapper::addObserverClassToClass('JTableObserverTags', 'JTableContent', array('typeAlias' => 'com_content.article'));
Personally, I prefer to see that line moved to the core extension to document where it should be for extensions. Again, I see no concern at all, for adding a dependency to your new approach as it accomplishes exactly what SOLID calls for.
Anyway, my thanks and 2 cents. Great work.
The JObserverMapper::addObserverClassToClass() call can be placed anywhere (outside the observed class) depending on your extension. E.g. In a component controller, or in a system plugin that is part of your extension.
The nice thing with JObserverMapper is that it doesn't auto-load the JObserverInterface-implementing class until a JOBserverableInterface-implementing class that has been mapped to be observed is created. Thus it is very CPU and disk-efficient.
We should definitely have a good page for extension developers on the proper and clean way to implement tags in their components. There is still some work to be done to make the Tags implementation clean on all fronts (e.g. Model, Form), possibly using the same Observer approach for JModel to map the tags model to any JModelSomething that needs it. Time was lacking to clean-up/aka reimplement properly Tags the way they should have been from start (Nicholas and me have been proposing to do that for 3.1, but it was deemed too late).
I agree that Tags implementation guidance should be given to developers, but with their current API for the rest of tags, it's more work to write documentation, than to re-implement them elegantly and write a much easier documeentation.
Having the call to JObserverMapper::addObserverClassToClass() in cms.php is just fine for the CMS, but it is certainly not the right place for installable extensions' own JTables.
A good place for extensions would be in the constructors of the instanciator(s) class(es) of their JTables, if they have such instanciators. Otherwise in their main controller constructor.
I'm not considering an optional Interface to be a dependancy ;-) but using a hard-coded classname, yes I am considering it as a dependancy.
garyamort
-
One question. Do the calls for JObserverMapper::addObserverClassToClass()
need to be in the libraries/cms.php file? My thought would be to put them
in the extension's entry files unless there's an argument against that.
One reason being that extensions can't modify this file, so we should do
in core as we would advise extension developers.
On Thursday, July 18, 2013, beat wrote: