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

Add REGEXP_REPLACE support (including unit test) #120

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

Zodiac1978
Copy link

Fixes #47

@Zodiac1978
Copy link
Author

Happy to get your review @bgrgicak ! :)

@Zodiac1978
Copy link
Author

@adamziel I have now additionally added checks for if any of the required parameters are null.

Copying the result from MySQL:

If expr, pat, or repl is NULL, the return value is NULL.
https://dev.mysql.com/doc/refman/8.4/en/regexp.html#function_regexp-replace

@bgrgicak bgrgicak added the enhancement New feature or request label Jun 13, 2024
Copy link
Collaborator

@bgrgicak bgrgicak left a comment

Choose a reason for hiding this comment

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

Thank you!

tests/WP_SQLite_Translator_Tests.php Show resolved Hide resolved
tests/WP_SQLite_Translator_Tests.php Show resolved Hide resolved
Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

Lgtm!

@adamziel
Copy link
Collaborator

Actually, let’s also test for empty string pattern, not just a null one

Comment on lines +527 to +530
/* Return null if the pattern is empty - this changes MySQL/MariaDB behavior! */
if ( empty( $pattern ) ) {
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh! Wha is the MySQL/MariaDB behavior? to just use an empty pattern? If so, let's do that.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines +507 to +520
/*
* 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.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Would you be up for adding a support for those?

I will try to do that!

@adamziel
Copy link
Collaborator

I just had a moment to review this in a more focused setting and I left a few more notes. It is pretty close!

@aristath aristath changed the base branch from main to develop July 19, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support regex_replace
3 participants