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

dev/core#2793 - Add REGEXP/NOT REGEXP SQL operators #21288

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

pfigel
Copy link
Contributor

@pfigel pfigel commented Aug 27, 2021

Overview

This adds the REGEXP and NOT REGEXP operators to API4 and SearchKit.

Before

REGEXP/NOT REGEXP were not usable as operators in API4 or SearchKit.

After

REGEXP/NOT REGEXP can be used as operators in API4 and SearchKit.

Technical Details

Some questions re: case-sensitivity in https://lab.civicrm.org/dev/core/-/issues/2793; solved via #21301

@civibot
Copy link

civibot bot commented Aug 27, 2021

(Standard links)

@civibot civibot bot added the master label Aug 27, 2021
@colemanw

This comment has been minimized.

@pfigel pfigel force-pushed the add-regexp-ops branch 2 times, most recently from a6cc231 to 89c338e Compare August 27, 2021 21:20
@pfigel pfigel changed the title [WIP] Add REGEXP/NOT REGEXP SQL operators [WIP] dev/core#2793 - Add REGEXP/NOT REGEXP SQL operators Aug 27, 2021
@pfigel pfigel changed the title [WIP] dev/core#2793 - Add REGEXP/NOT REGEXP SQL operators dev/core#2793 - Add REGEXP/NOT REGEXP SQL operators Sep 3, 2021
@pfigel pfigel marked this pull request as ready for review September 3, 2021 19:52
@colemanw
Copy link
Member

colemanw commented Sep 4, 2021

@pfigel this looks great. One question though: if case-sensitivity has been solved in the SQL functions front, should we remove the /i from the php-based search?

@pfigel
Copy link
Contributor Author

pfigel commented Sep 4, 2021

@colemanw Good question. If we include /i, the default behaviour of REGEXP in PHP-based queries matches the default behaviour of REGEXP in MySQL. At the same time, this prevents us from doing any case-sensitive matching in PHP-based queries, which can be achieved via BINARY() in MySQL.

I'd say: Include /i if we want to prioritize compatibility, or remove it if we want to go with principle of least surprise (my assumption being that most developers would expect regular expressions to be case-sensitive by default).

@colemanw
Copy link
Member

colemanw commented Sep 4, 2021

I say remove it. You can always build insensitivity into your regex if you need it (e.g. /[a-zA-Z]/) but you can't take it out if we force it to be there.

This adds the REGEXP and NOT REGEXP operators to API4 and SearchKit.
@pfigel
Copy link
Contributor Author

pfigel commented Sep 6, 2021

That makes sense - should be good now.

@colemanw colemanw merged commit 9644a7c into civicrm:master Sep 6, 2021
@colemanw
Copy link
Member

colemanw commented Sep 6, 2021

Just noting that this does affect SearchKit. I presume that users who don't know what "Matches Regexp" means will stay away from it.

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.

3 participants