Enabling filter's default parameters in config files #159

JJK801 · Feb 25, 2013

Hi stof,

This update is comming from troubles we had to set a filter parameter at the application start, it enable default values in the config files without breaking older configs.

Jérémy

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

@@ -60,7 +62,28 @@ private function enableFilters(EntityManager $entityManager)

        $filterCollection = $entityManager->getFilters();
        foreach ($this->enabledFilters as $filter) {
-            $filterCollection->enable($filter);
+            $oFilter = $filterCollection->enable($filter);


Why oFilter ?

$filter is the filter name and $oFilter the filter object, is it a problem?

we are not using the o*, s* convention to name variable starting with their type. So it is weird to have it used in a single place.

ok, $filterObject sounds better?

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

+            <xsd:element name="parameters" type="parameters" minOccurs="0" maxOccurs="1" />
+        </xsd:choice>
+        <xsd:attribute name="name" type="xsd:string" use="required" />
+        <xsd:attribute name="class" type="xsd:string" />


This is a BC break for XML users

I've run some tests and seems good with class as node value as the current config

yeah, I missed mixed="true"

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

+            <filter name="myFilter" enabled="true" class="Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\TestFilter">
+                <parameters>
+                    <parameter name="myParameter">myValue</parameter>
+                </parameters>


It should not look this way in XML but several <parameter> tags

stof · Feb 25, 2013

your XML configuration will not work(try to put at lest 2 parameters) as it is not how prototype nodes should be represented.

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

+    /**
+     * Enable filters for an given entity manager
+     *
+     * @param EntityManager $entityManager


This is wrong

oh yes, i missed that...

JJK801 · Feb 25, 2013

This will fix previouly said bad things

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

@@ -60,7 +62,28 @@ private function enableFilters(EntityManager $entityManager)

        $filterCollection = $entityManager->getFilters();
        foreach ($this->enabledFilters as $filter) {
-            $filterCollection->enable($filter);
+            $filterObject = $filterCollection->enable($filter);
+            if( null !== $filterObject ) {


spaces should be around the braces, not inside them

ok, i will fix it

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

+     * Set defaults parameters for a given filter
+     *
+     * @param string $name Filter name
+     * @param object $filter Filter object


instead of object, it would be better to use SQLFilter as it is the actual requirement (I would even typehint it)

I was not sure SQLFilter is a requirement, so i'm going to implement it

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

+        foreach ($calls as $call) {
+            if ($call[0] == $methodName) {
+                if ($called > $nbCalls) { break; }
+                else {


Please follow the Symfony2 CS

Ok sorry, my CS may be a little bit outdated :/

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

+            } else {
+                $this->fail("Method '".$methodName."' is expected to be called ". $nbCalls ." time, definition contain ". $called ." calls.");
+            }
+        }


I would replace all this with

$this->assertEquals($nbCalls, $call, sprintf('The method "%s" should be called %d times', $methodName, $nbCalls));

my first choice was similar but i changed it to get the same model as assertDICDefinitionMethodCallOnce, but you're right, this solution is much better

stof · Feb 25, 2013

stof reviewed Feb 25, 2013

View changes

stof · Feb 25, 2013

Sign in to view

+            if ($call[0] == $methodName) {
+                if ($called > $nbCalls) { break; }
+                else {
+                    if ($params[$called] !== null) {


what if it is not set ?

:/ lets fix it ;)

stof · Feb 26, 2013

stof reviewed Feb 26, 2013

View changes

stof · Feb 26, 2013

Sign in to view

@@ -15,6 +15,7 @@
 namespace Doctrine\Bundle\DoctrineBundle;

 use Doctrine\ORM\EntityManager;
+use  Doctrine\ORM\Query\Filter\SQLFilter;


there is an extra space here

stof · Feb 26, 2013

stof reviewed Feb 26, 2013

View changes

stof · Feb 26, 2013

Sign in to view

+     */
+    private function setFilterParameters($name, SQLFilter $filter)
+    {
+        if( !empty($this->filtersParameters[$name]) ) {


space spacing issue several times in this method

stof · Feb 26, 2013

stof reviewed Feb 26, 2013

View changes

stof · Feb 26, 2013

Sign in to view

+            if ($call[0] == $methodName) {
+                if ($called > $nbCalls) {
+                    break;
+                } else {


no need to use else as you are breaking the loop in the if

intel352 · Mar 3, 2013

+1 -- I need this functionality as well (ability to set parameters for filters within config).

omansour · Mar 4, 2013

👍 we need this PR at work

trsteel88 · Jul 14, 2013

Is there a way to use the service container to inject a value?

Eg, I have a Site Context and I want to use the SiteId from that Entity.

May relate to #134

Jérémy JOURDIN added some commits Feb 22, 2013

Added default parameters for query filters

8610564

XML Configuration support update + Tests update

12cab9a

XML Configuration support update + Tests update

78d8c10

Jérémy JOURDIN added some commits Feb 25, 2013

Fixs from pull request comments

eab53d0

Fixs from pull request comments

4ea05ac

Fix coding standards and some mistakes

6432345

Removing some unexpected spaces

35b3043

stof added a commit that referenced this pull request Mar 5, 2013

Merge pull request #159 from JJK801/filters
Enabling filter's default parameters in config files

d355798

stof merged commit d355798 into doctrine:master Mar 5, 2013
1 check passed

1 check passed

default The Travis build passed
Details

JJK801 deleted the JJK801:filters branch Mar 6, 2013

doctrine/DoctrineBundle

Join GitHub today

Enabling filter's default parameters in config files #159

Conversation

JJK801 commented Feb 25, 2013

Jérémy JOURDIN added some commits Feb 22, 2013

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

stof commented Feb 25, 2013

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

Jérémy JOURDIN added some commits Feb 25, 2013

This comment has been minimized.

JJK801 commented Feb 25, 2013

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

stof reviewed Feb 25, 2013 View changes

This comment has been minimized.

This comment has been minimized.

stof reviewed Feb 26, 2013 View changes

This comment has been minimized.

stof reviewed Feb 26, 2013 View changes

This comment has been minimized.

stof reviewed Feb 26, 2013 View changes

This comment has been minimized.

This comment has been minimized.

intel352 commented Mar 3, 2013

This comment has been minimized.

omansour commented Mar 4, 2013

stof added a commit that referenced this pull request Mar 5, 2013

Hide details View details stof merged commit d355798 into doctrine:master Mar 5, 2013 1 check passed

1 check passed

JJK801 deleted the JJK801:filters branch Mar 6, 2013

This comment has been minimized.

trsteel88 commented Jul 14, 2013

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 25, 2013

View changes

stof reviewed Feb 26, 2013

View changes

stof reviewed Feb 26, 2013

View changes

stof reviewed Feb 26, 2013

View changes

stof merged commit `d355798` into doctrine:master Mar 5, 2013
1 check passed