-
-
Notifications
You must be signed in to change notification settings - Fork 814
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
DedupeFind: Allow search datatable by regex on server side #8463
DedupeFind: Allow search datatable by regex on server side #8463
Conversation
@nielosz can you please open a ticket on https://issues.civicrm.org for this? (or link to it if one is already open). Thanks! (ok to have one issue for these 2 related PRs) |
I opened two tickets because I want the core team to have all choices :) |
I'm reviewing this now. |
Heh @nielosz as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR and the related #8464 ? |
hey @JoeMurray, yeah. I'll have a look. Do you have a list of PRs to be proofed? |
Prs are listed on here https://docs.google.com/spreadsheets/d/1fmHFOZ83ectSPCMWvXwCeJgrw4KpYi8JEV2ZDkd6XDU/edit#gid=529353056 More detail at You can add your name to the contributors list... |
@JoeMurray I'll note that this PR and the other related one on CRM-18694 are likely being subsumed under a larger rewrite of the function in question. @eileenmcnaughton should probably comment on whether that's still the case. |
Thanks, @josephlacey - feel free to pick PRs if none have been assigned. Cheers! |
Yeah - I'll look at this more this week - definitely don't want to be messing with this function without adding tests. My initial concern was that this might be insecure. But I don't think it is. It will also perform poorly on large data sets - but that seems to be no worse than the existing LIKE code - in both cases a large prev_next cache will be slow here. |
@nielosz could you please create a JIRA issue for this explaining the intent behind this PR? Thanks. |
@eileenmcnaughton would you be interested in reviewing this PR? |
yes |
@nielosz I'm wondering about this PR. Although you talk about changing to regex I see comments elsewhere suggesting this is about assuming people enter the postcode from the first char & not showing them every result with that char. If that is the case then a more performant option is to swap from LIKE '%xyx%'; to LIKE 'xyz%' This would work to do that (Note I've done that against my PR not core due to the degree of change) |
@eileenmcnaughton, you're totally right. Just "hardcoding" the handling of zip codes would fully satisfy CRM-18694. I just wanted to be more generic. But reading you're hint on performance I must admit I didn't know that like queries make use of indices if the fist char isn't a wildcard. That means that your approach increases performance compared to the default '%keyword%' clauses. |
Yeah I did quite a bit of digging on that & did this write-up OK - so I'll add this to my (big) PR & close your 2. If you have time to checkout the stuff in the big merge-stuff pr (#8456) that would be amazing. I think John Kingsnorth will take a look but the more eyes the better |
Okay, thanks! |
per discussion above - closing these in favour of the already merged chagnes |
DataTable's column search() method allows searching by regex but this has to be reflected serverside.
In template use
dtCol.search("myregex", true, false)
and mysql's extended regular expressions are being utilized:where colName REGEX 'myregex'
See CRM-18694