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 support for sql_mode and fix set charset #128

Merged
merged 7 commits into from
Dec 19, 2023
Merged

Add support for sql_mode and fix set charset #128

merged 7 commits into from
Dec 19, 2023

Conversation

ruifil
Copy link
Contributor

@ruifil ruifil commented Jul 6, 2023

src/MysqlConfig.php Outdated Show resolved Hide resolved
@xpader
Copy link

xpader commented Jul 17, 2023

I think getCharset() should never return null, cause there always have charset setted, from client set or server default, if we not set charset in client, getCharset() may return a value that auto detected.

@ruifil
Copy link
Contributor Author

ruifil commented Aug 7, 2023

I think getCharset() should never return null, cause there always have charset setted, from client set or server default, if we not set charset in client, getCharset() may return a value that auto detected.

I don't agree due to the fact getCharset() is a method of the connection parameter bag, if is empty don't change the database charset on database connection.
In order to know the database default connection charset, we can do show variables like "character_set_client"; after connection.

An alternative solution is to set the charset to utf8mb4/utf8mb4_general_ci but call SET NAMES in any case on each database connection.

@kelunik
Copy link
Member

kelunik commented Aug 7, 2023

It should likely be utf8mb4_0900_ai_ci and always call set names of course, but then we'd probably need fallback for older versions of mysql? So maybe it's better to really default to the connection default set by the server?

@trowski trowski merged commit b4ab0d6 into amphp:3.x Dec 19, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants