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

Improve Comparison::CONTAINS: allow to use custom position for % and _ wildcard characters #1150

Open
wants to merge 2 commits into
base: old-prototype-3.x
Choose a base branch
from

Conversation

Fedik
Copy link
Contributor

@Fedik Fedik commented Sep 28, 2014

This pull prevent forced wrapping of the value in to %
and allow to use the custom position for % and _ wildcard characters in case Comparison::CONTAINS

Allow to build the contains expressions like:

Criteria::expr()->contains('myField', 'some string%');
Criteria::expr()->contains('myField', '%some string');
Criteria::expr()->contains('myField', '%10\%'); // search for 10% at end of the string
Criteria::expr()->contains('myField', 's_m_ string');

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3328

We use Jira to track the state of pull requests and the versions they got
included in.

@stof
Copy link
Member

stof commented Oct 4, 2014

your implementation is wrong. % and _ chars need to be escaped all the time (actually, the list should be taken from https://github.com/doctrine/dbal/blob/7175964c30f4fd54c90f6f9c7c6f7bf49fc1c939/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php#L597). Otherwise, the behavior of contains() will not be consistent between the SQL matching and the in-memory matching.

@Fedik
Copy link
Contributor Author

Fedik commented Oct 4, 2014

not very understand why wrong,
in original was just "%{$value}%", unescaped ... and I am a bit extend this 😄

And example: if I will search (escaped) s\_m\_ string then SQL search exactly for string s_m_ string, means will ignore the wildcard characters

or "original" also wrong?

@stof
Copy link
Member

stof commented Oct 4, 2014

@Fedik the original is missing the escaping of $value before wrapping it in %

@Fedik
Copy link
Contributor Author

Fedik commented Oct 4, 2014

but
if do like "\%{$value}\%" and value example will be some text,
then final SQL will be field LIKE \%some text\% ...
so SQL will search for exact %some text% instead of any_string some text any_string

that is why I am confused by your comment 😄

ah, I understand, sorry 😄

ok, but here is problem,
what if I want search 'string starts on TEXT'?
after this pull I can just do Criteria::expr()->contains('myField', 'TEXT%');
and if by your suggestion $value will be escaped then I cannot do this

@stof
Copy link
Member

stof commented Oct 4, 2014

@Fedik I'm saying it should escape $value. You own proposal is escaping the extra placeholders added by Doctrine and leaves the value unescaped, so doing exactly the opposite

@Fedik
Copy link
Contributor Author

Fedik commented Oct 4, 2014

@stof sorry, made long editing of previous comment 😉

@stof
Copy link
Member

stof commented Oct 4, 2014

@Fedik your proposal of Criteria::expr()->contains('myField', 'TEXT%') does not respect the Criteria API if you expect it to search only at the beginning of values, and would require to make the API tied to SQL (which is the opposite of its goal) and totally weird (unless you require putting the % at the beginning and the end explicitly all the time, which is a BC break and still tied to SQL).

Your expectation is that some magic values turn contains() into a startsWith criteria. this does not make sense.

@Ocramius
Copy link
Member

@Fedik what @stof is saying is that Doctrine\Common\Collections\ArrayCollection#matching($criteria) won't work like the Doctrine\ORM\PersistentCollection#matching($criteria) with these different contains() semantics.

@Ocramius Ocramius self-assigned this Oct 19, 2014
@Fedik
Copy link
Contributor Author

Fedik commented Oct 19, 2014

@Ocramius thanks, this part was confusing for me, I thought only about SQL part, I very bad in Doctrine architecture.

So this in Doctrine\Common\Collections\Expr\ClosureExpressionVisitor.php#walkComparison ?:

case Comparison::CONTAINS:
  return function ($object) use ($field, $value) {
    return false !== strpos(ClosureExpressionVisitor::getObjectFieldValue($object, $field), $value);
  };

Then have no idea how to do this 😄 ... hm, build some preg_macth...

Maybe some suggestion how to make this work? or really no way?

@Ocramius
Copy link
Member

@Fedik preg_match could work here, yes.

@Fedik
Copy link
Contributor Author

Fedik commented Oct 25, 2014

ok, so I made pull also for doctrine/collections#45

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

Successfully merging this pull request may close these issues.

4 participants