Skip to content

Support filters with CONDITION_OR #151

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

Merged
merged 1 commit into from
Feb 14, 2016
Merged

Support filters with CONDITION_OR #151

merged 1 commit into from
Feb 14, 2016

Conversation

pascal-hofmann
Copy link
Contributor

This PR adds support for StringFilters with CONDITION_OR. These are required for correct filtering in fields of type sonata_type_model_autocomplete with multiple properties.

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

@pascal-hofmann
Copy link
Contributor Author

@rande @soullivaneuh @dunglas: Any plans on merging this PR?

@soullivaneuh
Copy link
Member

From where CONDITION_OR will be from? I mean, with the interface.

Could you please rebase your PR to check CI?

@pascal-hofmann
Copy link
Contributor Author

A CONDITION_OR will be used if you use a form field of type "sonata_type_model_autocomplete" and set "property" to an array of fields.

property: defaults to null. You have to set this to designate which field (or a list of fields) to use for the choice values. This value can be string or array of strings.

See https://sonata-project.org/bundles/admin/master/doc/reference/form_types.html#sonata-type-model-autocomplete

Without my changes, this generates a query where all conditions are combined with "AND", because the CONDITION_OR is silently ignored. So if you have an autocomplete and want the user to be able to search for either firstname or lastname, only items where both firstname and lastname match will be autocompleted.

@pascal-hofmann
Copy link
Contributor Author

My PR is already based on the latest master - I'll try to close and reopen it to trigger CI checks.

@pascal-hofmann
Copy link
Contributor Author

CI build randomly fails (see #153). CS issues are in existing code, and IMHO should be addressed in a commit separate from my changes.

@soullivaneuh
Copy link
Member

I fixed CS on master, please rebase again.

@pascal-hofmann
Copy link
Contributor Author

I rebased, but the CI build failed (see #153). Closing and reopening, maybe it'll work next time...

@pascal-hofmann
Copy link
Contributor Author

@soullivaneuh The failing TravisCI checks relate to this issue: sonata-project/SonataAdminBundle#3150

@pascal-hofmann
Copy link
Contributor Author

Closing and reopening to trigger ci checks.

@pascal-hofmann
Copy link
Contributor Author

Hmpf. I thought the dependency on security-acl was fixed, but that was only on another pr.

@OskarStark
Copy link
Member

@phofmann-trust you can rebase to get the latest travis conf

@pascal-hofmann
Copy link
Contributor Author

Why rebase? On which branch? 😕

@OskarStark
Copy link
Member

on the master branch

@pascal-hofmann
Copy link
Contributor Author

Latest commit on master is from 14 Dec - I already did a rebase on this last year.

@OskarStark
Copy link
Member

i restarted travis, lets see if its working...

@OskarStark
Copy link
Member

@core can you please check travis? maybe here is something different? Oo

@OskarStark
Copy link
Member

travis is green now after a restart

i will let @dbu merge this PR

@dbu
Copy link
Contributor

dbu commented Feb 14, 2016

oskar, i only maintain phpcr-odm, but not mongo odm.

@OskarStark
Copy link
Member

@dbu sorry my fault

Am Sonntag, 14. Februar 2016 schrieb David Buchmann :

oskar, i only maintain phpcr-odm, but not mongo odm.


Reply to this email directly or view it on GitHub
#151 (comment)
.

@OskarStark
Copy link
Member

tests are green, looks valid to me 👍

OskarStark added a commit that referenced this pull request Feb 14, 2016
Support filters with CONDITION_OR
@OskarStark OskarStark merged commit e64cee6 into sonata-project:master Feb 14, 2016
@OskarStark
Copy link
Member

Thank you @phofmann-trust for your contribution!

@pascal-hofmann
Copy link
Contributor Author

@OskarStark Thanks for merging. Awesome! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants