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

Switch unicode filename test to 3-byte character #503

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

Conversation

marxjohnson
Copy link
Contributor

MySQL 5.7 uses utf8mb3 encoding by default, meaning 4-byte characters
(such as emoji) are not supported. This ensures compatibility of the
presigned URL testing across databases, and prevents errors when
attempting to visit the settings page.

MySQL 5.7 uses utf8mb3 encoding by default, meaning 4-byte characters
(such as emoji) are not supported. This ensures compatibility of the
presigned URL testing across databases, and prevents errors when
attempting to visit the settings page.
@marxjohnson marxjohnson changed the base branch from DEPRECATED_master to MOODLE_310_STABLE August 8, 2022 15:00
@marxjohnson marxjohnson changed the base branch from MOODLE_310_STABLE to DEPRECATED_master August 8, 2022 15:00
@Peterburnett
Copy link
Contributor

From the MySQL docs:

You should also be aware that the utf8mb3 character set is deprecated and you should expect it to be removed in a future MySQL release. Please use utf8mb4 instead.

This feels like an antifeature and instead the database encoding should be updated. The whole point of the page is to test non-standard chars (such as an emoji), so my gut feel is this is somewhat like commenting out a failing test.

@brendanheywood
Copy link
Contributor

Following the docs this should be utf8mb4_unicode_ci

https://docs.moodle.org/400/en/MySQL_full_unicode_support

The question I have is what is our github CI doing here and are we testing on this correctly across the board?

@brendanheywood
Copy link
Contributor

For reference:

https://github.com/moodlehq/moodle-plugin-ci/blob/d87e9d96dd74f40cb2d93ade8e8d7934775d8ee7/gha.dist.yml#L18-L24

      mariadb:
        image: mariadb:10
        env:
          MYSQL_USER: 'root'
          MYSQL_ALLOW_EMPTY_PASSWORD: "true"
          MYSQL_CHARACTER_SET_SERVER: "utf8mb4"
          MYSQL_COLLATION_SERVER: "utf8mb4_unicode_ci"       

@brendanheywood
Copy link
Contributor

@marxjohnson
Copy link
Contributor Author

Thanks @Peterburnett ,
Unfortunately this goes beyond the unit test and presignedurl_tests.php, the fixture files are used to test the range requests when the plugin settings page is loaded. If the database can't handle the character, that settings page won't load. I agree that the proper long-term fix is to migrate our clients' databases to mb4 collation (or postgres) but that's not an option in the short term, so we need a fix like this to resolve the immediate regression.

The requirement of the fix on this branch wasn't to support emoji specifically, but to support characters outside of the ISO-8859-1 character set, which this still proves.

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.

3 participants