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

fix(upload): normalize the string used as basename #15151

Merged
merged 1 commit into from Dec 20, 2022

Conversation

nathan-pichon
Copy link
Member

What does it do?

Add a little .normalize(); function call to avoid problem with combined unicode.

See documentation:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/normalize#description

Why is it needed?

Combine unicode leads to an error in the Mysql database that doesn't use utf8mb4 charset. We may introduce the use of utf8mb4 in the future, but for now, it seems to be too big change for little added value.

How to test it?

Please follow the instruction on the issue:
#14793

Related issue(s)/PR(s)

Let us know if this is related to any issue/pull request
#14793

@nathan-pichon nathan-pichon added source: core:upload Source is core/upload package pr: fix This PR is fixing a bug labels Dec 12, 2022
@nathan-pichon nathan-pichon self-assigned this Dec 12, 2022
@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Base: 59.88% // Head: 59.88% // No change to project coverage 👍

Coverage data is based on head (0cf5d3c) compared to base (45831f3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15151   +/-   ##
=======================================
  Coverage   59.88%   59.88%           
=======================================
  Files        1348     1348           
  Lines       32809    32809           
  Branches     6257     6256    -1     
=======================================
  Hits        19647    19647           
  Misses      11306    11306           
  Partials     1856     1856           
Flag Coverage Δ
back 50.07% <100.00%> (ø)
front 64.34% <ø> (ø)
unit_back 50.07% <100.00%> (ø)
unit_front 64.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core/upload/server/services/upload.js 28.57% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nathan-pichon nathan-pichon marked this pull request as ready for review December 12, 2022 13:50
Copy link
Contributor

@Marc-Roig Marc-Roig left a comment

Choose a reason for hiding this comment

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

LGTM! Tested with Mysql 5.7 & Mysql 8

@nathan-pichon nathan-pichon merged commit 76bdf07 into main Dec 20, 2022
@nathan-pichon nathan-pichon deleted the fix/media-upload-diacritics branch December 20, 2022 14:45
@gu-stav gu-stav added this to the 4.5.5 milestone Dec 22, 2022
@gu-stav
Copy link
Contributor

gu-stav commented Dec 22, 2022

@nathan-pichon Please don't forget to add a milestone when merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: core:upload Source is core/upload package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants