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

MySQLi escapeLikeStringDirect() #2229

Merged
merged 7 commits into from Sep 18, 2019

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Sep 17, 2019

Description
As @michalsn points out, some MySQL commands (e.g. "SHOW ____") don't support custom escape characters supplied via "ESCAPE x". Currently listTables() is the only framework function suffering from this limitation, but it causes listTables() with $constrainPrefix to fail to constrain correctly.

This PR defines a new platform-specific function escapeLikeStringDirect() that can be used for those rare instances when both "SHOW" and "LIKE" are retried in the same query. This new function applies the default escape character \ directly the the "LIKE" string for any wildcards. This also has the MySQLi driver's _listTables() function use this new method instead to the generic one designed for "SELECT" statements.

Refs:

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

Someone tell me how those are valid Postgres strings:

1) CodeIgniter\Database\Live\EscapeTest::testEscape
586Failed asserting that two strings are equal.
587--- Expected
588+++ Actual
589@@ @@
590-'SELECT * FROM brands WHERE name = 'O\'Doules''
591+'SELECT * FROM brands WHERE name = 'O''Doules''
592
593/home/travis/build/codeigniter4/CodeIgniter4/tests/system/Database/Live/EscapeTest.php:31
594
5952) CodeIgniter\Database\Live\EscapeTest::testEscapeString
596Failed asserting that two strings are equal.
597--- Expected
598+++ Actual
599@@ @@
600-'SELECT * FROM brands WHERE name = 'O\'Doules''
601+'SELECT * FROM brands WHERE name = 'O''Doules''
602
603/home/travis/build/codeigniter4/CodeIgniter4/tests/system/Database/Live/EscapeTest.php:41

@jim-parry
Copy link
Contributor

Looks like two diferent escaping expectations, one with ' and one with '' ro end up with a single quote inside a single-quoted string

Can't attest to their validity, but it looks to me like the search is for "O'Doules"

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

Surely that can't be right? How could Postgres escape a character within a string wrapped in ' with another '? Maybe I need to do some Postgres research...

@jim-parry
Copy link
Contributor

I have seen that convention before, with other programming languages. I don't know anything about Postgres.

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

Yeah me neither! I thought I was adding some easy tests, but I suppose it was foolish to assume the same escape character across all drivers.

@michalsn
Copy link
Member

Escaping single quotes ' by doubling them '' is a standard way for PostgreSQL.

https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS

@MGatner
Copy link
Member Author

MGatner commented Sep 18, 2019

@michalsn Thanks for elucidating! I'm trying a version using each driver's escape char instead.

@MGatner
Copy link
Member Author

MGatner commented Sep 18, 2019

Finally got it! Someone please review - basically:

  • Added escapeLikeStringDirect() to handle MySQLi SHOW ... LIKE ... query escaping
  • Added EscapeTest.php for testing escapeLikeStringDirect() and a variety of other DB escape types

@lonnieezell
Copy link
Member

Looks fine to me, but I'm definitely not an expert on it. I'll take yours - and the tests - word for it.

@lonnieezell lonnieezell merged commit 91e1c5d into codeigniter4:develop Sep 18, 2019
@MGatner
Copy link
Member Author

MGatner commented Sep 18, 2019

I'm no expert but I feel a lot closer after 7 commits 😳

Put it on the list for 4.5 when we rewrite the Database layer that escape types should be differentiated better and implementations standardized better.

@MGatner MGatner deleted the mysqli-listtables branch September 18, 2019 16:21
@lonnieezell
Copy link
Member

Oh, so now the database rewrite is an actual thing :)

I definitely think it's showing it's age and the fact that it's been updated and worked on for 10+ years. Part of me would really like to rewrite that at some point, but not looking forward to it either lol

@MGatner
Copy link
Member Author

MGatner commented Sep 18, 2019

Oh yes, most definitely! I'm creating a milestone and assigning only you, with a November deadline. Good luck!

@MGatner
Copy link
Member Author

MGatner commented Sep 18, 2019

Jokes aside I just looked at my PR history and 11 out of the last 15 were for the database layer, mostly all bug fixes.

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.

None yet

4 participants