Skip to content

Conversation

ntzm
Copy link
Contributor

@ntzm ntzm commented Jan 20, 2018

I've made a PR to deal with this in core: laravel/framework#22864

Basically there was inconsistency between some DB servers, and also %, _ and \ weren't being escaped.

Copy link
Member

@AlexVanderbist AlexVanderbist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! It's definitely an interesting one. I'd prefer to see this fixed in core but as that might take a couple more weeks I'll pull this in here for now 👍

->addBinding("%{$this->escapeLike($value)}%");
}

private function escapeLike(string $value): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this one protected? This way it's a bit easier for people to extend if they'd want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course!

@AlexVanderbist AlexVanderbist merged commit 5cfece7 into spatie:master Jan 22, 2018
@ntzm ntzm deleted the escape-like branch January 22, 2018 15:51
@AlexVanderbist
Copy link
Member

Hi @ntzm

After merging this in we still ran into some problems. In tests using sqlite everything seems to work perfectly fine.
On MySQL however the ESCAPE part of the query seems to be escaped by Laravel itself and ends up looking something like this:

select * from `media` where `name` LIKE %test% ESCAPE \"\\\"

Any ideas for a quick fix?

@ntzm
Copy link
Contributor Author

ntzm commented Jan 23, 2018

Damn sorry, should have tested. I'm at work now, I'll figure something out later. Probably worth reverting if you have more stuff to release. My bad

AlexVanderbist added a commit that referenced this pull request Jan 23, 2018
AlexVanderbist added a commit that referenced this pull request Jan 23, 2018
@AlexVanderbist
Copy link
Member

Don't worry about it! Reverted it for now but I'll keep an eye on laravel/framework#22864 for any updates as well 👍

@ntzm
Copy link
Contributor Author

ntzm commented Jan 23, 2018

Thanks, and sorry again!

@ntzm
Copy link
Contributor Author

ntzm commented Jan 25, 2018

I can't for the life of me figure out how to prevent that from happening

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.

2 participants