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

Make sure the key used for translation is case sensitive. #15511

Merged
merged 3 commits into from Oct 22, 2019

Conversation

@jocel1
Copy link
Contributor

jocel1 commented Sep 12, 2019

This allows to handle "Order" & "order" keys properly for example

Questions Answers
Branch? develop
Description? International / Translation BO is not able to properly update translation keys with the same name, but different case
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #15510 / #10701
How to test? See the related ticket

This change is Reviewable

This allows to handle "Order" & "order" keys properly for example
@jocel1 jocel1 requested a review from PrestaShop/prestashop-core-developers as a code owner Sep 12, 2019
@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

khouloudbelguith commented Sep 17, 2019

Hi @jocel1,

I tried to translate my theme
childtheme.zip
=> NOK
image
With deveop branch => OK

Thanks!

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Sep 17, 2019

hi @khouloudbelguith can you give us more information about what you receive from your 400 bad request? or logs?

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

khouloudbelguith commented Sep 17, 2019

Hi @PierreRambaud,

On the Preview
{"error":"Warning: Illegal offset type"}
In the var/log, I have this file
dev.log

Thanks!

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 3, 2019

Hi @jocel1 @PierreRambaud ,

I've got the same problem as @khouloudbelguith on develop, with the "Classic" theme :
When I try to save the translation, I've got a, error : "Warning: Illegal offset type"

When I try to access the translation page again, I have the error again.

Here in video :
https://drive.google.com/open?id=1SQgeAXd5tfUG5LadILuXoi8SclmT98qZ

Thanks :)

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Oct 3, 2019

@Robin-Fischer-PS Do you reinstall PrestaShop after switching branch?

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 3, 2019

@PierreRambaud I indeed did a reinstallation of PS

@jocel1

This comment has been minimized.

Copy link
Contributor Author

jocel1 commented Oct 3, 2019

Are we sure it's related to this PR? Perhaps the branch needs to be rebased to develop, just to be sure?

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 3, 2019

@jocel1 Without the PR it works well, so I think it's related to the PR.

@jocel1

This comment has been minimized.

Copy link
Contributor Author

jocel1 commented Oct 15, 2019

Could you show me the SHOW CREATE TABLE ps_translation; output ?

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 16, 2019

Hi @jocel1 . After reproducing the bug, this is the output :

CREATE TABLE ps_translation (
id_translation int(11) NOT NULL AUTO_INCREMENT,
id_lang int(11) NOT NULL,
key varbinary(8000) NOT NULL,
translation text COLLATE utf8_unicode_ci NOT NULL,
domain varchar(80) COLLATE utf8_unicode_ci NOT NULL,
theme varchar(32) COLLATE utf8_unicode_ci DEFAULT NULL,
PRIMARY KEY (id_translation),
KEY IDX_ADEBEB36BA299860 (id_lang),
KEY key (domain)
) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci

@jocel1

This comment has been minimized.

Copy link
Contributor Author

jocel1 commented Oct 16, 2019

@Robin-Fischer-PS ok got it thanks, it should be VARCHAR BINARY and not VARBINARY, I'll double check doctrine to see what's the right type

@jocel1

This comment has been minimized.

Copy link
Contributor Author

jocel1 commented Oct 16, 2019

my latest commit should fix properly the issue

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Oct 16, 2019

This is maybe because of https://github.com/PrestaShop/PrestaShop/blob/develop/app/config/config.yml#L85 which force doctrine to think it uses mysql 5.1. 🤔

@jocel1

This comment has been minimized.

Copy link
Contributor Author

jocel1 commented Oct 21, 2019

@PierreRambaud not sure, anyway the latest fix is definitely better (defining a case sensitive collation for this field)

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 22, 2019

Hi @jocel1 ! Thanks for the correction, it works fine now 😄

@Robin-Fischer-PS Robin-Fischer-PS added this to the 1.7.7.0 milestone Oct 22, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 22, 2019

Thank you @jocel1

@matks matks merged commit 944dc00 into PrestaShop:develop Oct 22, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.