[#7858] - Fix a problem with populateState
- Fixed in Code Base
- 25 Aug 2016
- Medium
- Build: staging
- Easy Test
- # 7858
- Diff
- okonomiyaki3000:improve-populate-state
- Success JTracker/HumanTestResults Human Test Results: 3 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
These classes all ignore any values passed to populateState() and, instead, use hard-coded ones. Of course this is normally never an issue because this function only ever gets called by JModelLegacy::setState() and it doesn't pass any values anyway. So what is even the point of this function having any parameters at all? (Well the parent class has them so we have to have them.) Anyway, they're not very useful. Let's say you wanted to make a subclass of one of these models (there are totally legit reasons why a person would do this) and override populateState() with your own function which then calls parent::populateState() and would like to pass in some arguments. Well, too bad, those arguments will fall on deaf ears.
So what I'm doing here is just using the hard-coded values (that normally get passed to the parent function) as the default values for the parameters in all these functions. Then, just pass those on to the parent. So, if one of these classes gets subclassed, the sub can call parent::populateState() and pass in whatever it wants and get the result it expects.
I reckon this will be difficult to test. And also not difficult to test. First of all, these classes mostly just generate lists that get displayed on various admin pages. If all of those lists are still working, nothing has been broken. On the other hand, to test as a development feature, you'll need to create subclasses of each of these classes, override the populateState() function, and pass your own values to the parent. I don't recommend anyone waste time doing either of these things. Quite a few files are affected but the change is the same in all of them and quite small. I recommend that, instead of extensive testing, just review this code and decide whether or not these changes make sense.
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
They are not meant to be extended by 3rd party code.
Then they should be marked final but I hope that does not happen. There's nothing lazy about making a subclass. It's just a normal thing that you do when appropriate. Benefits include avoiding duplicate code, automatically getting bugfixes and other improvements when the parent class gets updated, and keeping up with changes to other code and data sources that the parent class accesses. There is a risk of something breaking when the underlying classes change but there is always a risk of that or something similar no matter what you do. Naturally, if you write code that relies on code written by someone else, you need to test it whenever there's an update. There's also the possibility that the underlying classes could disappear entirely but if things change to that degree, you were probably going to have to rewrite some stuff anyway.
These classes are used in pretty much every administrator list view. That would include:
/administrator/index.php?option=com_banners&view=banners
/administrator/index.php?option=com_banners&view=clients
/administrator/index.php?option=com_banners&view=tracks
/administrator/index.php?option=com_cache&view=cache
/administrator/index.php?option=com_banners&view=tracks
/administrator/index.php?option=com_categories&view=categories
/administrator/index.php?option=com_banners&view=tracks
/administrator/index.php?option=com_checkin&view=checkin
/administrator/index.php?option=com_contact&view=contacts
/administrator/index.php?option=com_content&view=articles
/administrator/index.php?option=com_finder&view=filters
/administrator/index.php?option=com_finder&view=index
/administrator/index.php?option=com_finder&view=maps
/administrator/index.php?option=com_installer&view=database
/administrator/index.php?option=com_installer&view=discover
/administrator/index.php?option=com_installer&view=manage
/administrator/index.php?option=com_installer&view=update
/administrator/index.php?option=com_installer&view=updatesites
/administrator/index.php?option=com_languages&view=installed
/administrator/index.php?option=com_languages&view=languages
/administrator/index.php?option=com_languages&view=overrides
/administrator/index.php?option=com_menus&view=items
/administrator/index.php?option=com_menus&view=menus
/administrator/index.php?option=com_messages&view=messages
/administrator/index.php?option=com_modules&view=modules
/administrator/index.php?option=com_newsfeeds&view=newsfeeds
/administrator/index.php?option=com_plugins&view=plugins
/administrator/index.php?option=com_redirect&view=links
/administrator/index.php?option=com_search&view=searches
/administrator/index.php?option=com_tags&view=tags
/administrator/index.php?option=com_templates&view=styles
/administrator/index.php?option=com_templates&view=templates
/administrator/index.php?option=com_users&view=groups
/administrator/index.php?option=com_users&view=levels
/administrator/index.php?option=com_users&view=notes
The debuggroup and debuguser views require a group or user id in the query string so it's best if you access them from links in the administrator.
The contenthistory component also has a view that should be affected but you can't access it by a url directly. You need to click the 'Versions' button in the article view.
The modules component has a modal 'positions' view that shows a list of positions. I'm not sure where this view is used. Presumably, there's a button or link somewhere that will cause it to appear.
There is a risk of something breaking when the underlying classes change but there is always a risk of that or something similar no matter what you do.
The main thing is that our backward compatibility promise is only valid for the library folder. Any class in extensions can be changed at any time. Like rewritten to a new MVC or even removed completely. It could also be that the whole extension isn't present because we decoupled it.
That's why it's very bad practice to extend from those classes. Just don't do it.
I'm gonna do it. Damn the consequences!!
| Category | ⇒ | Administration |
| Easy | No | ⇒ | Yes |
I did not notice any kind of issues with this patch applyed on the views above. One more tester for RTC . And thanks @okonomiyaki3000 that change makes sense.
Can you update this branch to latest staging. I've reviewed the code and it is good to go. 
However, does this trigger STRICT errors?
This PR has received new commits.
CC: @zero-24
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7858.
This PR has received new commits.
CC: @zero-24
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7858.
This PR has received new commits.
CC: @zero-24
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7858.
I have rebased this with the latest staging. There should be no conflict.
I even used this cool mergetest alias from stackoverflow and everything looked good:
http://stackoverflow.com/questions/501407/is-there-a-git-merge-dry-run-option/23148424#23148424
I have tested this item
I opened all the links and navigated manually to the sites also.
The following links had an error where the first menubar and the second occupied the same space
/administrator/index.php?option=com_menus&view=items
/administrator/index.php?option=com_menus&view=menus
I could´t reproduce this error, so maybe it was just a hiccup.
The code looks ok also.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7858.
I have tested this item
Applied patch, opened all links, checked for errors. Seems fine. Code review seems fine, too @icampus
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7858.
| Status | Pending | ⇒ | Needs Review |
Setting to Needs Review based on the comments of @bakual so the CMS maintainers can make a decision
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7858.
I have tested this item
The test workes @icampus
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7858.
This change is good to go for me. It doesn't affect current functionality and we already have classes in core doing exactly the same thing. So if @okonomiyaki3000 can fix the merge conflict we can set it to RTC. Thanks.
| Status | Needs Review | ⇒ | Information Required |
| Category | Administration | ⇒ | Administration Components |
| Status | Information Required | ⇒ | Ready to Commit |
| Labels |
Added:
?
|
||
rdeutz
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-25 20:09:14 |
| Closed_By | ⇒ | rdeutz |
| Labels |
Removed:
?
|
||
I disagree strongly with this reasoning. You never ever should extend classes from other extensions. That's just lazy coding and will bring you in trouble if we ever change those classes. They are not meant to be extended by 3rd party code.
I haven't an issue with the code itself. That looks good and should work.
Anyway can you provide a list of affected views so testers can make sure nothing broke?