Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global search ads wrong condition to custom query #3368

Closed
webdevilopers opened this issue Oct 13, 2015 · 24 comments · Fixed by sonata-project/SonataDoctrineORMAdminBundle#1358

Comments

@webdevilopers
Copy link
Contributor

My CompanyAdmin uses the createQuery method:

    public function createQuery($context = 'list')
    {
        $query = parent::createQuery($context);

        $query->addSelect(
                'PARTIAL ' . $query->getRootAlias() . '.{id, name, phone, fax, postcode, street, city, country}',
                'PARTIAL b.{id, name}'
            )
            ->join($query->getRootAlias() . '.branches', 'b')
        ;

        $query->where($query->getRootAlias() . '.id = 1');

        return $query;
    }

    protected function configureDatagridFilters(DatagridMapper $datagridMapper)
    {
        $datagridMapper
            ->add('name', null, array(
                'show_filter' => true,
                'global_search' => true
            ))
        ;
    }

The where condition is not added with an AND operator but an OR:

SELECT 
  DISTINCT c0_.id AS id_0 
FROM 
  companies c0_ 
  INNER JOIN branches b1_ ON c0_.id = b1_.company_id 
WHERE 
  c0_.id = 1 
  OR c0_.name LIKE ? 
LIMIT 
  10 OFFSET 0

This only happens when using the createQuery method and seems to be a problem of the global search adding it.

Without that method it is generated correctly:

SELECT 
  DISTINCT c0_.id AS id_0 
FROM 
  companies c0_ 
WHERE 
  c0_.name LIKE ? 
LIMIT 
  10 OFFSET 0
@webdevilopers
Copy link
Contributor Author

I also recognized that the link to the list views of the result is no passing the original filter.
The list view will display regular unfiltered results. I think the filter should be passed.

@rewotec
Copy link

rewotec commented Oct 16, 2015

👍

1 similar comment
@eusebiub
Copy link

eusebiub commented Jul 7, 2016

👍

@soullivaneuh
Copy link
Member

Please stop 👍 on comments. Use emoji reactions for that.

@greg0ire
Copy link
Contributor

The only piece where global_search is taken into account is here. Can you tell us whether this piece of code is called, and throw an Exception inside it to see what calls it but shouldn't ?

@greg0ire
Copy link
Contributor

Oh sorry, you're on the search page, right? I thought you had problems with the search handler interfering where it shouldn't. What you expect to happen is the following, right?

WHERE existing_condition AND (filter conditions, separated with OR)

@gprioux
Copy link

gprioux commented Aug 16, 2016

As written in the duplicated #4074 , the expected behaviour is
WHERE existing_condition AND (filter conditions, separated with OR)

@greg0ire
Copy link
Contributor

Step 1 would be to pinpoint the piece of code where this happens, can you help with that @gprioux ?

@verheyenkoen
Copy link
Contributor

verheyenkoen commented Nov 15, 2016

@greg0ire: The default AND condition is switched to OR here.

Fixing that won't be sufficient though, since you want an OR condition between the global_search filters so that would only work for admins with only one global_search filter.

@lopsided
Copy link
Contributor

The following seems to be a workaround for this problem:

// Sonata\AdminBundle\Datagrid\Datagrid
// ~ line 160

        // CHANGE #1: Extract original where conditions and then reset it
        $originalWhereConditions = $this->query->getDQLPart('where');
        $this->query->resetDQLPart('where');

        foreach ($this->getFilters() as $name => $filter) {
            $this->values[$name] = isset($this->values[$name]) ? $this->values[$name] : null;
            $filter->apply($this->query, $data[$filter->getFormName()]);
        }

        // CHANGE #2: Extract the filter where conditions and then reset it again
        $filterWhereConditions = $this->query->getDQLPart('where');
        $this->query->resetDQLPart('where');

        // CHANGE #3: Combine both where condition-sets in a 2-piece AND
        $and = $this->query->expr()->andX();
        $and->add($originalWhereConditions);
        $and->add($filterWhereConditions);
        $this->query->where($and);

It seems the simplest way to fix the problem, not sure if it is a particularly good one. Might be better to rewrite the filter->apply methods to stack onto a proper "ANDX"/"ORX" doctrine expr object instead? Would be a lot more work that though.

@muspelheim
Copy link
Contributor

@greg0ire Hi, what do you think about @lopsided's solution?

@greg0ire
Copy link
Contributor

greg0ire commented May 2, 2017

I think it could work, and I would approve a PR proposing that. The second solution sound better, but would indeed be more work, and might not be as backwards-compatible.

@muspelheim
Copy link
Contributor

@greg0ire I can make PR with this changes, and may be some code refactoring

@greg0ire
Copy link
Contributor

greg0ire commented May 2, 2017

Please do :)

@nico-dangerous
Copy link

Hi,
any news on this ?

Thanks

@VincentLanglet
Copy link
Member

We started a reflexion to fix the bug here #5589

@stale
Copy link

stale bot commented Jan 30, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@verheyenkoen
Copy link
Contributor

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

I'm not doing PHP/Symfony/Sonata Admin anymore so don't know if this still an issue for anyone. Feel free to close.

@webdevilopers
Copy link
Contributor Author

I explained to @core23 on Twitter that I no longer maintain the projects that use Sonata.
I will try to comment on the issues anyway. I will close them if nobody else suggested a PR or similar.

There have been some proposals for this issue. From my POV it can be closed.
Please tell me if I have to close it personally.

@VincentLanglet
Copy link
Member

The bug is not solved. And it's not easy to fix.
Personally I would prefer to let the issue opened until a PR to fix this is merged.

@core23 core23 added the keep label Jan 31, 2020
@tonyellow
Copy link

Unfortunately I can confirm that this is still a problem on latest version.

@VincentLanglet
Copy link
Member

Unfortunately I can confirm that this is still a problem on latest version.

Yes, the issue seems really hard to fix and nobody made a PR to fix this ATM.
Please be patient or try to make a PR, I can help you if you want ;)

@danuddara
Copy link

Instead of modifying the dataGrid, I modified the searchHandler to make this work. I have reused the code suggested above here. This solved my issue and might help someone who's looking to fix it. I override this SearchHandler service using a compiler pass.

diff --git a/src/Search/SearchHandler.php b/src/Search/SearchHandler.php
index 6947b104..d25b64f3 100644
--- a/src/Search/SearchHandler.php
+++ b/src/Search/SearchHandler.php
@@ -13,6 +13,7 @@ declare(strict_types=1);
 
 namespace Sonata\AdminBundle\Search;
 
+use Doctrine\Common\Collections\ArrayCollection;
 use Sonata\AdminBundle\Admin\AdminInterface;
 use Sonata\AdminBundle\Admin\Pool;
 use Sonata\AdminBundle\Datagrid\PagerInterface;
@@ -59,6 +60,17 @@ class SearchHandler
     {
         $datagrid = $admin->getDatagrid();
 
+        /**
+         * @var $query QueryBuilder
+         */
+        $query = $datagrid->getQuery();
+
+        /*keep the original data for later use*/
+        $originalWhereConditions = $query->getDQLPart('where');
+        $originalParamters = $query->getParameters();
+        $query->resetDQLPart('where'); //reset where part
+        $query->setParameters([]);//reset parameters
+
         $found = false;
         foreach ($datagrid->getFilters() as $filter) {
             /** @var $filter FilterInterface */
@@ -76,6 +88,24 @@ class SearchHandler
 
         $datagrid->buildPager();
 
+        //after doing normal build then add the orginals back.This will then build the query correctly
+        $filterWhereConditions = $query->getDQLPart('where');
+        $query->resetDQLPart('where');
+
+        $condition = $filterWhereConditions; //by default keep the filter where clause to add it to the query
+        //orverride if there are any orginal paramters
+        if($filterWhereConditions && $originalWhereConditions) {
+            $condition = $query->expr()->andX();
+            $condition->add($originalWhereConditions);
+            $condition->add($filterWhereConditions);
+            $filterParamters = $query->getParameters();
+            $all_parameters = new ArrayCollection(array_merge($filterParamters->toArray(),$originalParamters->toArray()));
+
+            $query->setParameters($all_parameters);
+
+        }
+        $query->where($condition);
+
         $pager = $datagrid->getPager();
         $pager->setPage($page);
         $pager->setMaxPerPage($offset);

@VincentLanglet
Copy link
Member

This unfortunaly works only when you use the SonataDoctrineOrmBundle.

But the methods you're calling are not available in the ProxyQueryInterface, so that does not allow to make a general fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment