Add REGEXP_REPLACE support (including unit test)#120
Add REGEXP_REPLACE support (including unit test)#120Zodiac1978 wants to merge 10 commits intoWordPress:developfrom
Conversation
|
Happy to get your review @bgrgicak ! :) |
|
@adamziel I have now additionally added checks for if any of the required parameters are null. Copying the result from MySQL:
|
|
Actually, let’s also test for empty string pattern, not just a null one |
| /* Return null if the pattern is empty - this changes MySQL/MariaDB behavior! */ | ||
| if ( empty( $pattern ) ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Oh! Wha is the MySQL/MariaDB behavior? to just use an empty pattern? If so, let's do that.
There was a problem hiding this comment.
It is throwing an error (because without pattern it can't work at all):
Error in the SQL query (3685): Illegal argument to a regular expression.
@bgrgicak was suggesting to return null instead to avoid breaking things, I assume.
There was a problem hiding this comment.
Null seemed like a better response from an error. @Zodiac1978 let's update the comment to make it clear what this changes MySQL/MariaDB behavior means.
| /* | ||
| * If the original query says REGEXP BINARY | ||
| * the comparison is byte-by-byte and letter casing now | ||
| * matters since lower- and upper-case letters have different | ||
| * byte codes. | ||
| * | ||
| * The REGEXP function can't be easily made to accept two | ||
| * parameters, so we'll have to use a hack to get around this. | ||
| * | ||
| * If the first character of the pattern is a null byte, we'll | ||
| * remove it and make the comparison case-sensitive. This should | ||
| * be reasonably safe since PHP does not allow null bytes in | ||
| * regular expressions anyway. | ||
| */ |
There was a problem hiding this comment.
Does regexp_replace support the BINARY keyword? I couldn't find anything. If it doesn't, let's remove this comment and the special casing for the null byte.
There was a problem hiding this comment.
Isn't the binary-mode something global?
See: https://mariadb.com/docs/server/ref/mdb/cli/mariadb/binary-mode/
| * | ||
| * @return Array if the field parameter is an array, or a string otherwise. | ||
| */ | ||
| public function regexp_replace( $field, $pattern, $replacement ) { |
There was a problem hiding this comment.
I just checked and in MySQL REGEXP_REPLACE() also takes three optional parameters:
pos: The position in expr at which to start the search. If omitted, the default is 1.
occurrence: Which occurrence of a match to replace. If omitted, the default is 0 (which means “replace all occurrences”).
match_type: A string that specifies how to perform matching. The meaning is as described for REGEXP_LIKE().
Would you be up for adding a support for those? If not, that's fine, but let's at add them to the function signature and fail with an informative warning (REGEXP_REPLACE() don't support non-default values for $pos (fourth argument), .... if you need it then you can contribute here: ....) if they're set to non-default values.
There was a problem hiding this comment.
Would you be up for adding a support for those?
I will try to do that!
|
I just had a moment to review this in a more focused setting and I left a few more notes. It is pretty close! |
|
@JanJakes is this PR still relevant? |
|
@bgrgicak I think it is; we only need to update the tests as well for the new driver. I can take a look at this. |
4 participants
Fixes #47