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#2165 Test for Handle emojis less fatally where not supported #18918

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 3, 2020

Overview

Test for emoji find - demonstrates that using an emoji in the string currently results in a fatal error

Before

No test

After

test

Technical Details

What to do?

I'm honestly tempted just to swap the emoji for the word emoji when it is in the string - that would mean it's not found - which seems correct to me

We can use a construct something like

    $collation = CRM_Core_BAO_SchemaHandler::getInUseCollation();

    if (stripos($collation, 'utf8mb4') === FALSE) {
      // kill the owl
    }

Comments

@civibot
Copy link

civibot bot commented Nov 3, 2020

(Standard links)

@civibot civibot bot added the master label Nov 3, 2020
@mattwire
Copy link
Contributor

mattwire commented Nov 8, 2020

@eileenmcnaughton check your style

@eileenmcnaughton
Copy link
Contributor Author

@totten @seamuslee001 @colemanw this implements the approach we discussed - ie saying that by definition if your search criteria requires an emjoi and your database does not support emoji then the criteria resolves to false. When digging into the code this was the cleanest place to put it & given we currently get a hard fail putting it someone comprehensive seems good.

I've tried to limit calls to && max(array_map('ord', str_split($criterion))) >= 240) { as much as possible by doing cheaper checks first - but I note that not that in fact this function is not quite as high traffic as it seems as usually not that many criteria are set, even if many queries are run

@seamuslee001
Copy link
Contributor

The idea seems to make sense to me but test fail seems related

@eileenmcnaughton eileenmcnaughton force-pushed the emoji branch 5 times, most recently from efb2e56 to 9de6b36 Compare November 30, 2020 00:24
…ported

Fix emoji handling by returning always-false when unwelcomely present
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 passing now

@seamuslee001
Copy link
Contributor

So this in essence passes but doesn't return any results which seems most sensible IMO

merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants