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

[11.x] Add mariadb to databases config #48455

Conversation

Jubeki
Copy link
Contributor

@Jubeki Jubeki commented Sep 19, 2023

This adds proper support for MariaDB, as the default/recommended collation for mysql and mariadb has changed.

See laravel/laravel#6239
See laravel/laravel#6240
See laravel/laravel#6241

Changes:

  • Splits of the MariaDB Database Configuration.
  • Changes default collation for MySQL to utf8mb4_0900_ai_ci
  • Changes default collation for MariaDB to utf8mb4_uca1400_ai_ci
  • Test MariaDB Configuration against MariaDB database.

Changes to the config still needs to be applied to Testbench.

@driesvints driesvints changed the title [slim-skeleton-11.x] Add mariadb to databases config [11.x] Add mariadb to databases config Sep 19, 2023
@Jubeki
Copy link
Contributor Author

Jubeki commented Sep 19, 2023

Any ideas why the MariaDB 10 tests fail? I can't find a location were I missed adding mariadb as an available connection.

@@ -140,7 +140,7 @@ jobs:
- name: Execute tests
run: vendor/bin/phpunit tests/Integration/Database
env:
DB_CONNECTION: mysql
DB_CONNECTION: mariadb
Copy link
Member

Choose a reason for hiding this comment

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

Why change this?

Copy link
Member

Choose a reason for hiding this comment

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

@taylorotwell these are the mariadb tests and previously it used the mysql connection. Now it uses its dedicated mariadb connection which this PR creates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that the MariaDB Database tests are run against the MariaDB configuration instead of the MySQL configuration. Because of the changed collation in the mysql configuration which does not exist in MariaDB, this would throw an error.

@crynobone
Copy link
Member

This should be expected since Testbench holds it's own configuration files.

@Jubeki
Copy link
Contributor Author

Jubeki commented Sep 19, 2023

This should be expected since Testbench holds it's own configuration files.

I forget about Testbench. Thanks!

@taylorotwell taylorotwell marked this pull request as draft September 20, 2023 15:03
@Tristan-MyAnaPro
Copy link

Hi @Jubeki / @driesvints

Now that we'll have a dedicated config for MariaDB, shouldn't the default collation for MariaDB be uca1400_ai_ci ?

uca1400_ai_ci is based on Unicode 14 and available in MariaDB since 10.10.1 (Laravel 11.x will support
MariaDB 10.11+ so we're covered https://laravel.com/docs/master/database).

Note that I used uca1400_ai_ci and not utf8mb4_uca1400_ai_ci as this collation can be used with any of these charset utf8mb3, utf8mb4, ucs2, utf16, utf32 (although I don't really understand why you'd want to use a less than 4 bytes charset (utf8mb3) with this collation 🤔)

What do you think about it ?

@Jubeki
Copy link
Contributor Author

Jubeki commented Sep 21, 2023

@Tristan-MyAnaPro this seems like a good idea.
Seems like there are already plans to make uca1400_ai_ci the default. See https://jira.mariadb.org/browse/MDEV-30164

Probably as of MariaDB 11.4. See https://jira.mariadb.org/browse/MDEV-19123

@taylorotwell taylorotwell force-pushed the slim-skeleton-11.x branch 2 times, most recently from b70ebc8 to d8f44e3 Compare October 2, 2023 18:23
@Jubeki Jubeki force-pushed the add-mariadb-to-databases-config branch from 31a0985 to bb53692 Compare October 5, 2023 07:31
@driesvints
Copy link
Member

@Jubeki are you still working on this one?

@Jubeki
Copy link
Contributor Author

Jubeki commented Oct 10, 2023

@driesvints in my opinion I am finished with this PR. I only rebase against slim-skeleton-11.x sometimes.

One point for discussion open is only:

  • Change default charset for mariadb to uca1400_ai_ci in the config without waiting for MariaDB to make the change. (Which I would be in favour of)

(Taylor put the PR into Draft, so I left it like that)

@grooverdan
Copy link
Contributor

Note - MDEV-32336 deb default config - use collation-server = utf8mb4_uca1400_ai_ci MariaDB/server#2775 for 11.3

That without an explicit collation utf8mb4 uses the collation uca1400_ai_ci. Applying this to Debian/Ubuntu will flow through to the mariadb:11.3 container on the next release.

If you want to ensure that the next version of MariaDB (server and container) to be released is going to be Laravel compatible (and give you a chance to tell MariaDB to fix things before release :-) ) you can use the following container images:

  • quay.io/mariadb-foundation/mariadb-devel:very-latest (beta/rc)
  • quay.io/mariadb-foundation/mariadb-devel:latest (stable)
  • quay.io/mariadb-foundation/mariadb-devel:latest-lts (stable and long term maintained)
  • quay.io/mariadb-foundation/mariadb-devel:ealiest-lts (old and still maintained)

These are the latest version of completed code merged into the main branches - https://quay.io/repository/mariadb-foundation/mariadb-devel?tab=tags . The code there has passed review and is expected to work so this isn't a beta tree, just an unreleased version.

information:

Or just ask me. Happy to help.

@driesvints
Copy link
Member

@Jubeki we might want to resend in this PR as Taylor probably force-pushed his commits.

@Jubeki Jubeki force-pushed the add-mariadb-to-databases-config branch from 9659479 to 15afe53 Compare November 8, 2023 08:28
@Jubeki
Copy link
Contributor Author

Jubeki commented Nov 8, 2023

@grooverdan Thanks for the information about the changes in MariaDB. I applied the default collation in the config accordingly. I don't think the newer container image is needed here, because the tested mariadb version already supports the new collation, but of course the Laravel Team can decide to test against the new image. (FYI Laravel 11 will probably be released in February 2024 as far as I know).

@driesvints I rebased against slim-skeleton-11.x. Should I mark this PR as Ready for Review?

@driesvints
Copy link
Member

@Jubeki yeah go for it

@Jubeki Jubeki marked this pull request as ready for review November 8, 2023 08:36
@taylorotwell taylorotwell merged commit 220e722 into laravel:slim-skeleton-11.x Nov 8, 2023
7 of 16 checks passed
@Jubeki Jubeki deleted the add-mariadb-to-databases-config branch November 8, 2023 19:12
taylorotwell pushed a commit that referenced this pull request Nov 28, 2023
* Add MariaDB to databases config and Change MySQL 8 collation

* Update tests workflow for MariaDB

* Update default collation for mariadb
jordyvanderhaegen added a commit to esign/laravel-linkable that referenced this pull request Mar 12, 2024
GitHub actions throws exceptions for unkown utf8mb4_0900_ai_ci which is the new default in Laravel 11.
laravel/framework#48455
jordyvanderhaegen added a commit to esign/laravel-database-trigger that referenced this pull request Mar 12, 2024
GitHub actions throws exceptions for unkown utf8mb4_0900_ai_ci which is the new default in Laravel 11.
laravel/framework#48455
jordyvanderhaegen added a commit to esign/laravel-database-auditing that referenced this pull request Mar 13, 2024
GitHub actions throws exceptions for unkown utf8mb4_0900_ai_ci which is the new default in Laravel 11.
laravel/framework#48455
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.

None yet

6 participants