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

BUG Fix compatibility issues with mysql 5.7 and above (fluent 4) #618

Open
wants to merge 1 commit into
base: 4.4
Choose a base branch
from

Conversation

tractorcow
Copy link
Collaborator

Fixes #257


Deprecation::notification_version('4.0.0', 'tractorcow/silverstripe-fluent');

// Credit to https://github.com/sunnysideup thank you for the mysql fix
DB::query("SET SESSION sql_mode='REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE';");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same problem as #617

Need to conditional and apply only to mysql, not postgres / sqlite etc...

@sminnee
Copy link

sminnee commented Oct 18, 2020

FYI we're looking at introducing a sql_mode yml config option here and so it might be worth using that instead:
silverstripe/silverstripe-framework#9721

It would mean that you need Silverstripe 4.7 to support MySQL 5.7, though.

@tractorcow
Copy link
Collaborator Author

I much prefer the config option to this; I don't want to have to conditionally run this query based on DB backend.

@sminnee
Copy link

sminnee commented Oct 21, 2020

Note that I assume Fluent doesn't support PGSQL at all right now as there's no way of turning off strict group by on PGSQL.

@tractorcow
Copy link
Collaborator Author

tractorcow commented Oct 23, 2020

It doesn't support postgresql any less than framework does. Until a while back the tests were running postgres (they got turned off at some point I can't recall). It looks like the SS3 -> SS4 rewrite. We should turn it back on to see what's broken.

@tractorcow
Copy link
Collaborator Author

tractorcow commented Oct 23, 2020

It looks like the original issue was due to COALESCE, which fluent probably should not be using anymore for conditions, since locale row should be deterministic (the localisation row exists, or it doesn't).

Here's the comment on FluentExtension::localiseCondition().

* Note that unlike localiseSelect, this uses a simple COLASECLE() for performance and to reduce
* overall query size.

We probably should just remove COALESCE anyway, but better to turn on postgres tests before we go making any assumptions. :) It should probably mirror localiseSelect() in that it uses a case - when chain for each fallback locale.

Next time I get some free time (hah) I'll try this out.

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

Successfully merging this pull request may close these issues.

2 participants