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

Modified Bundle.js because of breaking Encoding in Production Mode. #10563

Merged
merged 3 commits into from
Sep 1, 2017

Conversation

ulftietze
Copy link
Member

@ulftietze ulftietze commented Aug 17, 2017

Description

This pull request prevents wrong or too much encoding of UTF-8 Files.

Fixed Issues (if relevant)

  1. Merging / Bundling JS Files Production Mode breaks Regular Expressions #10562: Merging / Bundling JS Files Production Mode breaks Regular Expressions
  2. maybe Bad ukrainian character encoding in minicart when Enabled JavaScript Bundling in production mode #6733: Bad ukrainian character encoding in minicart when Enabled JavaScript Bundling in production mode

Manual testing scenarios

  1. Reproduce the issue Merging / Bundling JS Files Production Mode breaks Regular Expressions #10562

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 17, 2017

CLA assistant check
All committers have signed the CLA.

@ishakhsuvarov ishakhsuvarov self-assigned this Aug 17, 2017
@ishakhsuvarov ishakhsuvarov added this to the August 2017 milestone Aug 17, 2017
@ulftietze
Copy link
Member Author

Hey @ishakhsuvarov,

could you restart the travis-ci integration test? Just one of them failes with a timeout ^^

Thanks, Ulf

@ishakhsuvarov
Copy link
Contributor

@ulftietze Restarted.
Please consider covering changes with tests.

@ulftietze
Copy link
Member Author

Hey @ishakhsuvarov,

same problem as last time, the Job gives us a timeout. Do you know, why this is happening?

I tried to modify the test for this class, but unfortunatly there are only a test which tests perhaps ~20% of the Bundle.php Class.
Also it's an deprecated class and i did just a small change for this class. May we can agree, that a full unit test (which i could do but with a ton of mocking) is a little bit overhead.

Thanks, Ulf

@ishakhsuvarov
Copy link
Contributor

@ulftietze We are looking into functional test timeouts. I've restarted the job again.
Could you please provide some manual test cases which are fixed with this PR?

@kkkonrad
Copy link

can you check validation messages in customer registration page? looks like on this page validation messages not work when store is on Production mode+enabled all JS optimizaton, i test it on 2.1.3, 2.1.8 and 2.2.0-rc and looks like never get simple translation "This is required field."

@ulftietze
Copy link
Member Author

Moin @kkkonrad,

i tried to reproduce your issue with my fix but i just can't :/ I think, that this could be a conflict with another corebug i think. This could possible be #7862.

Greets, Ulf

@ulftietze
Copy link
Member Author

Moin @ishakhsuvarov,

i don't know, how explict you are expecting to get the tests. But i did a very short script, where you explictly can see the difference. Or at least the problem :)

<?php

$jsToParse = 'if (!(value.match(/^([a-zA-ZßäöüÄÖÜ\s.]{3,})+$/))) {
                return this.customValidationError($t(\"Please provide a correct string.\"));
           }';

print_r(mb_convert_encoding($jsToParse, "UTF-8") . PHP_EOL);
print_r(utf8_encode($jsToParse) . PHP_EOL);

This script has the following output:

// Correct:
if (!(value.match(/^([a-zA-ZßäöüÄÖÜ\s.]{3,})+$/))) {
                return this.customValidationError($t(\"Please provide a correct string.\"));
           }
// Wrong:
if (!(value.match(/^([a-zA-Z�äöü���\s.]{3,})+$/))) {
                return this.customValidationError($t(\"Please provide a correct string.\"));
           }

Greets, Ulf

@ishakhsuvarov
Copy link
Contributor

@ulftietze Thanks for providing details.
I could not see any effect from the fix on the latest develop branch.

I assume this happens because \Magento\Framework\View\Asset\Bundle is deprecated and unused. Moving the fix to the \Magento\Deploy\Package\Bundle\RequireJs however, seems to solve the issue. Could you please re-check this?

@magento-team magento-team merged commit 88267a4 into magento:develop Sep 1, 2017
magento-team pushed a commit that referenced this pull request Sep 1, 2017
magento-team pushed a commit that referenced this pull request Nov 10, 2017
magento-team pushed a commit that referenced this pull request Nov 10, 2017
Tasks
- MAGETWO-80207 [2.2.x] - Modified Bundle.js because of breaking Encoding in Production -
 Mode. #10563
- MAGETWO-80188 [2.2.x] - Prevent change log entry when nothing has changed #4893
- MAGETWO-70323 [GITHUB] Can't add to cart Bundle product with simple product option price as "zero" #8969
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants