[#6677] - [fix] store JModelList filters as objects in session without relaying in Registry
- Closed
- 16 May 2015
- Medium
- Build: staging
- # 6677
- Diff
- phproberto:stools-objectvsarray
- Failure The Travis CI build failed Details
User tests: Successful: Unsuccessful:
This issue tries to fix issue #6544 after updating Registry package to latest framework version.
Replaces #6548
A bit of history
When I implemented searchtools (yes you can blame me 
) I stored filters in session as array and relayed in Registry to cast them to objects. After updating to the latest Registry package my modification was gone and searchtools started to experience conflicts between filters sent as objects (the old way) and filters sent as arrays (the new way).
Having reviewed and discussed about it I now see that I did it wrong forcing Registry to cast data to objects. The solution is to cast data in JModelList directly. But can we now do that without causing B/C issues with extensions already using searchtools? This what this pull request tries to test. Let's see if the best fix is still possible. If not we will need a B plan.
Backward compatibility issues
As you can see in the changes the best behavior parent::populateState() is called now on top of any model extending JModelList that way automatic filters can be overriden by custom filters. In the old implementation the parent::populateState was done the last thing to allow that any filter could initialize the object in session before any filter is stored. That was mainly to support old filtering systems (like the ones that Hathor uses).
After searchtools was implemented extension developers should decide if you they going to use searchtools or not and based in that setup their model populateState() method.
The issue may now be that third part extensions have to move the parent::populateState() method inside their model populateState(). So we need third part extensions using searchtools to HEAVILY test this.
Why is this the best fix possible?
Because it now ensures that all the filters are stored in session in the same way (objects). Filters can be accessed & modified in the same way than before Searchtools was implemented (using objects). The idea is that JModelList automatically detects the filters allowing you to override them if needed.
How to test it
Go to any of the current managements using Searchtools and ensure that filters work normally. This includes:
- Creating more than 5 items per management to be able to test pagination
- Set any filter and then test pagination ensuring that filters are kept and pagination works normally.
- Reset filters should restore all the values and should redirect you to page 1 of search results.
- Enable Hathor template and test that all the filters work there too. Thos filters are not using searchtools. Before you had to logout / login to get the filters working but now it should work correctly.
Managements to test:
- com_banners > Banner list
- com_banners > Client list
- com_content > Category list
- com_content > Article list
- com_content > Featured articles list
- com_menus > Item list
Test by third part extensions
- Install your extension in a joomla that includes this patch and test that your filters work normally.
joomla-cms-bot
- | Labels |
Added:
?
|
||
Couldn't detect any issues with my own component which uses Search Tools. But then, I hadn't any issues without that patch as well. So not sure if that counts as a successfull test 
However Travis isn't happy as several unit tests fail.
zero-24
- | Category | ⇒ | Components Libraries |
@test Success with following test
- Apply patch #6682 It integrates searchbar filters in Module Manager.
Go to Modules Manager. Set filter State to Published. Test pagination. Doesn't work.
Change file/method
/administrator/components/com_modules/models/modules.php::populateState()
and move line
parent::populateState('position', 'asc');
to top like this
protected function populateState($ordering = null, $direction = null)
{
// List state information.
parent::populateState('position', 'asc');
- Test pagination again. Works.
- Apply this patch 6677 here
Test pagination again. Works.
Move line
parent::populateState('position', 'asc');
back to bottom.
- Test pagination again. Fails,
Thanks for testing. I'll review travis errors today.
I should have written
you forgot com_ menus items, i.e. change also:
administrator/components/com_menus/models/items.php
where moving parent::populateState('a.lft', 'asc'); to the top of the method works fine.
Thanks @infograf768. Now it's fixed
About tests I cannot get the errors locally so I think I need some help with that :(
The main issue is that now filters & list options from searchtools are not using $app->getUserStateFromRequest() but JModelList::getFilterStateFromRequest() so I think the tests need to be changed to test that.
This is what I get when running unit tests locally:
$ phpunit --testsuite libraries-legacy
PHPUnit 4.6.1 by Sebastian Bergmann and contributors.
Configuration read from /home/roberto/www/jcms3x/phpunit.xml
The Xdebug extension is not loaded. No code coverage will be generated.
....S.....................................SSSSSSSSSSSSSSSSSSSSS 63 / 140 ( 45%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS....SSS.. 126 / 140 ( 90%)
..............
Time: 268 ms, Memory: 23.50Mb
OK, but incomplete, skipped, or risky tests!
Tests: 140, Assertions: 146, Skipped: 79.
Got the errors locally. I was missing the sqlite package (php5-sqlite) and apparently that was preventing tests to work as expected.
I have added new unit tests for the new getFilterStateFromRequest(). Next: Fix old tests
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-05-16 12:25:34 |
| Closed_By | ⇒ | phproberto |
You forgot com_menus.
For the others, looks like working here.
Did not test any 3rd party extension.