Added FilterPersister to customize the way filters are stored #4331

yann-eugone · 2017-02-16T09:42:38Z

I am targetting this branch, because this feature as a backward compatibility.

Changelog

### Added
- Added some `Sonata\AdminBundle\Filter\Persister\FilterPersisterInterface` to externalise filter persistence to a dedicated class
- Added some `Sonata\AdminBundle\Filter\Persister\SessionFilterPersister` to store filters to session (as done now)

### Changed
- Changed `Sonata\AdminBundle\Admin\AbstractAdmin::getFilterParameters` to use the configured filter persister component

To do

Update the tests
Update the documentation
Add an upgrade note

Subject

I had to persist filters accross sessions, so I decided to persist values to a database.
But as all the code remains in the AbstractAdmin, I had to redefine the whole method in my admin base class.

I did this PR so we can extend easily the way filters are persisted. We can also create more out-of-the-box filter persisters, such as : ORM, ODM, Redis...

OskarStark · 2017-02-16T10:05:21Z

Admin/AbstractAdmin.php

     */
-    protected $persistFilters = false;
+    protected $filterPersister;


this is a BC break

you're right... if we want to do it cleanly, we should remove this property

OskarStark · 2017-02-16T10:07:08Z

If we can stay BC 3.x is fine, otherwise we should deprecate the current logic and do this PR against master

greg0ire · 2017-02-16T10:11:53Z

Admin/AbstractAdmin.php

+    /**
+     * @param FilterPersisterInterface $filterPersister
+     */
+    public function setFilterPersister($filterPersister)


Please use type hinting here, and make it optional so that null can be passed.

greg0ire · 2017-02-16T10:12:11Z

Admin/AbstractAdmin.php

-     * Whether or not to persist the filters in the session.
-     *
-     * @var bool
+     * @var FilterPersisterInterface|bool


FilterPersisterInterface|null

greg0ire · 2017-02-16T10:12:22Z

Admin/AbstractAdmin.php

@@ -766,16 +766,28 @@ public function getFilterParameters()
    {
        $parameters = array();

+        // if filter persister was configured with `true`, use the default persister (session)
+        if ($this->filterPersister === true) {


greg0ire · 2017-02-16T10:19:34Z

Admin/AbstractAdmin.php

@@ -766,16 +766,28 @@ public function getFilterParameters()
    {
        $parameters = array();

+        // if filter persister was configured with `true`, use the default persister (session)
+        if ($this->filterPersister === true) {
+            $this->filterPersister = new SessionFilterPersister($this->request->getSession());


Couldn't this be done with DI, BTW? If no filterPersister is set, inject this default service?

my aim was to avoid deprecation, but yeah i can do it like it, but the boolean mode will have to be removed

Getting rid of the boolean mode is a good thing, b/c then it will be easier to reason on the type of filterPersister, and use native checks like type hinting.

greg0ire · 2017-02-16T10:20:10Z

Admin/AbstractAdmin.php

-                if ($filters == array() && $this->request->query->get('filters') != 'reset') {
-                    $filters = $this->request->getSession()->get($this->getCode().'.filter.parameters', array());
+            // if filter persistence is configured
+            if ($this->filterPersister instanceof FilterPersisterInterface) {


What else can it be?

i'm not a big fan of falsy tests, i rather test type of my object

Not a big fan either, it was an honest question b/c I hadn't seen it could be boolean yet.

greg0ire · 2017-02-16T10:22:00Z

DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php

+        // if persist filters is a boolean use session persister as default
+        // if persist filters is a string, use service reference instead
+        if (is_bool($persistFilters)) {
+            $definition->addMethodCall('setPersistFilters', array($persistFilters));


If true, inject default service, if false, don't do anything.

yann-eugone · 2017-02-16T10:51:43Z

Should I close this PR, and reopen onto the master branch, with service injection ?

greg0ire · 2017-02-16T10:53:49Z

Should I close this PR, and reopen onto the master branch, with service injection ?

You mean it can't be BC?

greg0ire · 2017-02-16T10:56:06Z

Filter/Persister/SessionFilterPersister.php

+     */
+    public function reset($admin)
+    {
+        $this->session->remove($admin.'.filter.parameters');


Please isolate the pattern building in a dedicated method.

greg0ire · 2017-02-16T10:57:14Z

Filter/Persister/FilterPersisterInterface.php

+     * Set persisted filters for given admin.
+     *
+     * @param string $admin   The admin code
+     * @param array  $filters The filters to persist


Please document what the array is supposed to look like (can it be multidimensional? What type are the keys, what are they?)

The structure is the same than the filter values. It depends on what filters you are using.

yann-eugone · 2017-02-16T10:57:50Z

@greg0ire I think so. We need to remove the persistFilters property (it may be confusing), and use service setter injection only.

this is a BC right ?

greg0ire · 2017-02-16T10:59:43Z

Removing a protected property is not BC. It's a BC-break and the property should be deprecated instead.

Can you try making the PR on 3.x? If we can't find a solution, then you'll have to rebase on master, but right now it's hard for me to imagine what would be a BC-break.

yann-eugone · 2017-02-16T11:27:10Z

Ok, I juste pushed a new version.

greg0ire · 2017-02-16T11:28:22Z

Admin/AbstractAdmin.php

+     *
+     * @var FilterPersisterInterface|null
+     */
+    protected $filterPersister;


How about making it private?

When a code for myself, everything is private, but here, other properties are protected, i just tried to code like the rest

Ok, good thinking, but please make it private anyway, we're trying to change things.

greg0ire · 2017-02-16T11:29:31Z

Admin/AbstractAdmin.php

     * @var bool
+     *
+     * @deprecated since 3.x, to be removed in 4.0.


We could use magic to trigger deprecation notices here, but since it's the admin class, which is already big, and other properties could be deprecated, this could become a maintenance nightmare, so I'd say leave it like that.

greg0ire · 2017-02-16T12:50:14Z

Filter/Persister/SessionFilterPersister.php

+     *
+     * @return string The storage key
+     */
+    protected function buildStorageKey($admin)


private please

greg0ire · 2017-02-16T12:50:26Z

Filter/Persister/SessionFilterPersister.php

+ *
+ * @author Yann Eugoné <eugone.yann@gmail.com>
+ */
+class SessionFilterPersister implements FilterPersisterInterface


Make it final please

greg0ire · 2017-02-16T13:35:09Z

Filter/Persister/FilterPersisterInterface.php

+     *                        "_page" => {int page num},
+     *                        "_sort_by" => {string sort property},
+     *                        "_sort_order" => {string sort order (ASC|DESC)},
+     *                        "_per_page" => {int count rows per page}


greg0ire · 2017-02-16T13:36:25Z

Looks BC to me.

jordisala1991 · 2017-02-16T18:54:10Z

DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php

        }

-        $definition->addMethodCall('setPersistFilters', array($persistFilters));
+        // if boolean config value provided, use the default persister : session
+        if (is_bool($persistFilters) && $persistFilters) {


Do we have to check if is_bool($persistFilters)? This property is set from container parameter or from the attributes, and we can cast it previously on the other case.

Maybe we can change from scalarNode to booleanNode on the Bundle Configuration?

Just to have properties that we know they are a concrete type and can't be other things.

this code may also receive value from the tag of an admin service. I think that allowing true to be provided, may be a short but efficient way to configure the persistence.

got it. So if it is a boolean and true, we just change its value to the default persister, otherwise we chech if the value is there set by config (other provider or default) and then we call to setFilterPersister. It is something like that? was confused at first

you got it. configuring persist_filters to true means

i don't care how you persist it, just do it

jordisala1991 · 2017-02-16T18:58:18Z

This is super nice!

We should remember to update other bundles mocking some classes that extends AbstractAdmin, because those test will broke.

greg0ire · 2017-02-17T07:11:54Z

We should remember to update other bundles mocking some classes that extends AbstractAdmin, because those test will broke.

@OskarStark see? Don't mock what you don't own ;)

jordisala1991 · 2017-02-17T07:17:06Z

Agree with that, but when you are extending a class that you dont own. Either you mock it or you dont test your logic. Ideally extend classes is not a good practice in TDD but in some cases like sonataMedia we are not TDDing, but just adding test for the current code. If I have to choose between fragile test or fragile code IMO prefer to fix test all day long.

OskarStark · 2017-02-17T07:29:32Z

@OskarStark see? Don't mock what you don't own ;)

👍

OskarStark · 2017-02-17T07:29:47Z

@yann-eugone can you please fix StyleCi?

yann-eugone · 2017-02-17T08:21:08Z

@OskarStark just pushed the fix, but not sure to understand the problem with AbstractAdmin::$filter property

…(s)_persister

…rfaces

yann-eugone · 2018-03-13T08:12:27Z

@greg0ire @OskarStark @jordisala1991 @core23 just rebased the whole Pull Request for the 2nd time, hope everything will be OK this time

greg0ire · 2018-03-13T18:47:56Z

Thanks a lot for this!

OskarStark · 2018-03-14T06:42:38Z

Thank you very much, great work @yann-eugone 👍

yann-eugone · 2018-03-14T07:46:32Z

Happy to finally find a way to contribute 😄

jordisala1991 · 2018-04-11T10:12:57Z

src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php

-            $persistFilters = (bool) $attributes['persist_filters'];
-        } else {
-            $persistFilters = (bool) $container->getParameter('sonata.admin.configuration.filters.persist');
+            $persistFilters = $attributes['persist_filters'];


Could be located here? @soullivaneuh , before we had a cast to (bool) and now we don't. Maybe the value is getting there but not with the correct type?

well, "true" would also be considered as a true so not sure that is it

"true" !== true

if strict is used 😄

SonataAdminBundle/src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php

Lines 314 to 323 in 0344393

if (isset($attributes['persist_filters'])) {

$persistFilters = $attributes['persist_filters'];

}

$filtersPersister = $container->getParameter('sonata.admin.configuration.filters.persister');

// override default configuration with admin config if set

if (isset($attributes['filter_persister'])) {

$filtersPersister = $attributes['filter_persister'];

}

// configure filters persistence, if configured to

if ($persistFilters) {

and it is not

…-project#4331)

SonataCI added the review required label Feb 16, 2017

OskarStark requested changes Feb 16, 2017

View reviewed changes

OskarStark added the minor label Feb 16, 2017

greg0ire requested changes Feb 16, 2017

View reviewed changes

greg0ire reviewed Feb 16, 2017

View reviewed changes

greg0ire requested changes Feb 16, 2017

View reviewed changes

greg0ire reviewed Feb 16, 2017

View reviewed changes

greg0ire requested changes Feb 16, 2017

View reviewed changes

greg0ire reviewed Feb 16, 2017

View reviewed changes

jordisala1991 reviewed Feb 16, 2017

View reviewed changes

OskarStark added the pending author label Feb 17, 2017

SonataCI removed the pending author label Feb 17, 2017

yann-eugone added 11 commits March 13, 2018 09:08

Add an upgrade note

ee3b824

Fixed naming inconsistency between plural and singular form of filter…

e3c9b9b

…(s)_persister

Feedback about syntax in recipe

5b46356

Added typehinting to all methods in filter persister classes and inte…

88f1154

…rfaces

Code review : parameter naming to be more clear

6731e25

Removed upgrade header

703b980

Fixed unit tests after rebase

d86bdd0

Code style fixes

16e84db

Removed PHP7 typehint

811dca8

Fixed recipe location and syntax

dc214f6

Added phpunit tests for the SessionFilterPersister

0eab2d8

yann-eugone dismissed stale reviews from core23, jordisala1991, and greg0ire via 0eab2d8 March 13, 2018 08:09

SonataCI removed the pending author label Mar 13, 2018

OskarStark approved these changes Mar 13, 2018

View reviewed changes

jordisala1991 approved these changes Mar 13, 2018

View reviewed changes

jordisala1991 requested review from greg0ire, core23 and kunicmarko20 March 13, 2018 08:41

kunicmarko20 approved these changes Mar 13, 2018

View reviewed changes

greg0ire merged commit 2579fc4 into sonata-project:3.x Mar 13, 2018

greg0ire mentioned this pull request Mar 18, 2018

Fix mocks #5033

Merged

kunicmarko20 mentioned this pull request Apr 10, 2018

Filters from list are reset after a "Save and close" action #5069

Closed

jordisala1991 reviewed Apr 11, 2018

View reviewed changes

pestaa pushed a commit to Crosssec/SonataAdminBundle that referenced this pull request Aug 6, 2018

Added FilterPersister to customize the way filters are stored (sonata…

06342c8

…-project#4331)

	if (isset($attributes['persist_filters'])) {
	$persistFilters = $attributes['persist_filters'];
	}
	$filtersPersister = $container->getParameter('sonata.admin.configuration.filters.persister');
	// override default configuration with admin config if set
	if (isset($attributes['filter_persister'])) {
	$filtersPersister = $attributes['filter_persister'];
	}
	// configure filters persistence, if configured to
	if ($persistFilters) {

Added FilterPersister to customize the way filters are stored #4331

Added FilterPersister to customize the way filters are stored #4331

Conversation

yann-eugone commented Feb 16, 2017 • edited by jordisala1991 Loading

Changelog

To do

Subject

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yann-eugone commented Feb 16, 2017

greg0ire commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yann-eugone commented Feb 16, 2017

greg0ire commented Feb 16, 2017

yann-eugone commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Feb 16, 2017

jordisala1991 Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yann-eugone Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

jordisala1991 commented Feb 16, 2017

greg0ire commented Feb 17, 2017

jordisala1991 commented Feb 17, 2017

OskarStark commented Feb 17, 2017

OskarStark commented Feb 17, 2017

yann-eugone commented Feb 17, 2017

yann-eugone commented Mar 13, 2018

greg0ire commented Mar 13, 2018

OskarStark commented Mar 14, 2018

yann-eugone commented Mar 14, 2018

Choose a reason for hiding this comment

kunicmarko20 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yann-eugone commented Feb 16, 2017 •

edited by jordisala1991

Loading

jordisala1991 Feb 16, 2017 •

edited

Loading

yann-eugone Feb 17, 2017 •

edited

Loading

kunicmarko20 Apr 11, 2018 •

edited

Loading