Success

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
29 Jul 2013

Moving the calls to create the observer from the cms.php file to the proper tables.

This is done so the code is better understandable by 3rd party extension developers trying to figure out how tags work. Also there is no need to decouple these calls, as the data is tightly coupled anyway. The observers need to be there in this case or we may loose data.

I also had to move the oberserver interfaces and classes to their own files so the autoloader will find them. The problem was that once I removed the JObserverMapper calls from cms.php, the JTable class would not find the JObservableInterface anymore.

Tracker:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=31603

avatar Bakual Bakual - open - 29 Jul 2013
avatar beat
beat - comment - 1 Aug 2013

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.

avatar AmyStephen
AmyStephen - comment - 1 Aug 2013

left a message on @beat 's PR regarding the dependency.

@Bakual - regardless, Bakual@4c9bf58 is a good idea. I believe the framework has a standard of one class per file. Doesn't hurt one way or the other, but I think it's an industry "best practice" (today) anyway.

avatar Bakual
Bakual - comment - 1 Aug 2013

Why do the class need to be independent? Don't get me wrong: It's good that they can be independent in that a plugin can put on observer on a class without hacking the class itself. But here we are talking about tags which are an integral part of the core components. If the table is instantiated by any other extension, the observer should be there to process the tags. Otherwise there may be loss of data. The table is the best place to ensure that the observer is there.
Especially for scripts which run outside of the CMS (without loading cms.php), the tag processing fails and is actually a B/C break.

avatar AmyStephen
AmyStephen - comment - 1 Aug 2013

Rather than this:

JTableObserverTags::createObserver($this, array('typeAlias' => 'com_newsfeeds.newsfeed'));

Would it not be more flexible like this:

JTableObserver::createObserver($this, array('typeAlias' => 'com_newsfeeds.newsfeed'));

Wondering.

avatar beat
beat - comment - 1 Aug 2013

@Bakual : Even if you instantiate a class extending JTableContent, the rule would still apply to the child. So there is no loss of data. Even if things are "core", they can still be implemented cleanly separated. ;-)

avatar AmyStephen
AmyStephen - comment - 1 Aug 2013

I think something might be a little off - http://en.wikipedia.org/wiki/Observer_pattern

The subject (ex. newsfeed) should not be aware of the concrete observer (ex. JTableObserverTags).

The goal is to uncouple these but the way it's implemented, there is another layer, yes, but it's still as coupled as it was before. Could be wrong, but I don't think so.

avatar Bakual
Bakual - comment - 1 Aug 2013

Currently if you load the table from CLI without including cms.php, tag saving doesn't work. Which basically is a B/C break. There are such scripts which automatically create articles from different sources. That's why I think it should go into the table. Anyway, the cms.php is the wrong place.

avatar beat
beat - comment - 1 Aug 2013

@Bakual if cms.php is the wrong place, we can move the mapping rules to another place. Any suggestion ? I did ask if there was a better place for those CMS-related items. cms.php seemed fine as it had other bindings. Now if CLI doesn't load cms.php, but loads another CMS-related file that is also loaded in the web-joomla-app i'm fine. The bindings should definitely NOT go into the JTableXYZ classes themselves.

avatar Bakual
Bakual - comment - 1 Aug 2013

I think it should for sure go somewhere into the extension. It makes also no sense to map everything if stuff like weblinks or newsfeed isn't even used on the site. So if it's in the extension it's only loaded when needed.
I still think the table is the best place :)

avatar beat
beat - comment - 1 Aug 2013

@Bakual Table is neither the right nor the best place, That one is as sure as written in stone, I am firmly opposed to the pull request as proposed.

It's either from where you use the corresponding table, if you do a dynamic on-the-fly binding without rules. Or it has to go into a file that is loaded by the CMS (executed from web or CLI doesn't matter). Maybe cms.php should be loaded from the CLI too, or another new file added for Joomla-CMS core observers mapping rules.

B/C for tags has been broken by a previous patch, and that was a wanted/accepted B/C break as I understood. So it'a a good time to stop mixing features and to start with clean API's and implementations.

avatar Bakual
Bakual - comment - 1 Aug 2013

The previous B/C break is fixed now...
As already said, the core extensions also serve as blueprints for 3rd party developers. So the observer call needs to be in the extension itself or developer will not spot it. Put it to the spot recommended for 3rd party developers wanting to support JTags in their extensions. Which place would you suggest for other extensions if not the table? This would be the place it belongs in the core extensions as well.
Also I used the code suggested on the comments of the content table itself. So it can't be that wrong :)

avatar AmyStephen
AmyStephen - comment - 1 Aug 2013

Sorry, did more studying on the pattern and realized I was mixing in the Events; had it backwards.

If you do this -- and add the alias as a property of the JTable subclass, instead of passing it in):

JObserverMapper::addObserverClassToClass('JTableObserverTags', 'JTable');

Then all an extension would have to do to implement tags is add their XML for the administrator and for menu items:

    <field
        id="show_tags"
        name="show_tags"
        type="radio"
        class="btn-group"
        default="1"
        label="COM_CONTENT_FIELD_SHOW_TAGS_LABEL"
        description="COM_CONTENT_FIELD_SHOW_TAGS_DESC">
        <option value="0">JHIDE</option>
        <option value="1">JSHOW</option>
    </field>

I guess I am confused why you are so against the JTable dependency. JTable is already very dependent on the observer classes. In the Constructor, attachObserver, getObserverofClass

More importantly, @beat, I think you might be missing something pretty dramatic. If you put it down at the Table level, Joomla could offer version control, logging, backups of changed data, email notifications, replication, all those generic data operations like tags, one at a time -- and each new service could be (should be) 100% independent from the component. It would be awesome if components did not have to register or know anything about it.

You could even move much of the ACL operations down here by adding data elements which describe the read, write, delete, publish, etc., capabilities at the time of data retrieval, let alone providing for "last defense" checking immediately before the table was modified.

But @Bakual is right that it must be in the Table class -- again, it's the "last defense" position -- in some cases, the only chance, of ensuring data integrity and business rules and validation - etc.

You can of course try to ensure other classes are always loaded but you will never have a better place than JTable.

I think you've exposed a lot of opportunity @beat -- but please try to uncouple it from the component. Big bang for your buck there - and I think you've only got tiny strings to cut to be there. Very encouraging to see.

avatar AmyStephen
AmyStephen - comment - 1 Aug 2013

BTW - the next step @mbabker has already taken with the layouts - won't be long before even that XML and menu/layout customization won't be needed either.

The Joomla Component is almost gone. Long live Resources (content) and plugins and layouts.

avatar Bakual
Bakual - comment - 3 Aug 2013

Moved the Observer stuff to the CMS library as of Elins suggestion in the tracker.

Should I better make an own PR for the file stuff (put the observer classes in their own files and move to cms library) since I think there is less discussion about this?
Or do you think we can get on a consent where to best put the observer calls and get this whole PR merged? :)

avatar beat
beat - comment - 3 Aug 2013

@Bakual I really don't understand what you are trying to achieve here.

I see no more benefits in your last changed PR than in the previous one.

avatar Bakual
Bakual - comment - 3 Aug 2013

@Beat

  • The last two commits were suggested by Elin on the tracker and I think she is right. The Joomla library is basically what was in the platform (maybe new framework). The CMS library is where the stuff related to the CMS should go. I just moved the files there because they belong there. It doesn't change anything in the function at all, it's just a question about where the files should be stored.
  • The part about splitting up the files so each class is in its own file is a no-brainer by itself. It's how the libraries are set up and how the autoloader works. You even suggested that yourself in the original commit I think. For you it worked because you loaded the mapper first and thus loaded all other classes and interfaces as well, but it fails for CLI scripts.
  • I think the only part of this PR where one can have different opinions is the part where the observer call should be made in the CMS. I think I already explained why I want them to be in the table and you explained why you want it decoupled. I leave this up for others to decide.

I don't think you have any objections to the first two parts. Or did I got this wrong? If so maybe it's better to split this PR up into two parts. One dealing with the files and one moving the calls to the table. Then at least the file part can be merged faster and fixes the CLI issues with not finding the interface.

avatar beat
beat - comment - 4 Aug 2013

@Bakual I have now read the tracker item that I wasn't following. I will keep the discussion here, as having it on 2 places does not make sense.

  1. Moving to CMS: No: the Observer is used by JTable (and will most probably be used by other core classes in 3.2), thus it should go where JTable goes. As JTable is in joomla folder (ex-platform) it should be in joomla. If JTable moves to cms, then it should move along. So not agreeing on the move to cms folder. But moving for moving JTables and/or Observers classes files from joomla to cms folder is just changing for the sake of changing without added value. Your time could be better spent (and mine too) in improving Joomla 3.2 in a way which makes a difference to the user.

  2. Splitting interfaces and classes into separate files: OK, Yes, No issues: Makes sense as commented in original source code. Please make a separate pull-request just for this for review and please paste it's URL in here for review and approval :-) .

  3. Observers mapping initialization into JTables: No, do not agree: I am really for following SOLID and DRY. So that part of the pull request should not be accepted.

  4. Fully agree that extension developers need appropriate examples in the Tags documentation. But that is not a code issue. It would be better to add Observers to other parts of Tags API so that it's much easier to implement tags than to revert SOLID improvements here.

avatar Bakual
Bakual - comment - 4 Aug 2013

@beat Keeping the discussion here is fine for me :-)

  1. Actually I don't care much here. I just followed the suggestion of Elin which made sense to me. You have a good point here as well.

  2. Did a new PR just for this as suggested. Hopefully we can get that merged soon so the issue with some scripts can be solved and we get a cleaner library.

  3. I think doing the observer mapping in the cms.php is following neither SOLID nor DRY. The cms.php is only there to initialize the CMS and its library. And it contains some B/C stuff related to renaming classes. It hasn't to do anything which any extensions and especially not with tags. So putting the mapping there is for sure the wrong place. I hope you don't disagree with that :-)
    The question is more where the appropriate place is. I think we can agree that it should go into the extension itself for various reasons. Be it controller, model or table. I still have no other advise than what is written in JTableContent:

/*

  • 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')); */

Since I still don't see the point of why it should be decoupled in the first place, I still favor the table as the appropriate place. Especially since 3rd party extensions have no other way than doing it coupled.

  1. It is a code issue because code should be self describing. Either by putting in docblocks (like you did) or by using code which is self explaination. I think we can also agree that the current documentation about how to support tags in 3rd party extensions is either not existent, outdated or not easy to find. Personally I don't know where it is, I didn't find it and had to search the code for the solution, where I didn't find it neither because the stuff is currently in cms.php. That's also why this matters to me. I'm looking at it as a 3rd party extension developer who wants to support in my extension. The thing is that I supported it from when it was introduced, tested even during beta and fixed some bugs. My implementation broke with 3.1.4 due to a B/C bug. I fixed that bug in Joomla and also changed my extension to support the new way. That took me at least some hours of my free time and I want to do some changes in the cms so other developers will have it easier than me. Which in the end will benefit the user as well I'm sure. In fact I could care less, since my extension now works. But I don't think that's the attitude which will make Joomla a better CMS:
avatar Bakual Bakual - close - 28 Aug 2013
avatar Bakual Bakual - head_ref_deleted - 28 Aug 2013

Add a Comment

Login with GitHub to post a comment