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 undefined INTL_IDNA_VARIANT_UTS46 if intl ICU < 4.6 #11995

Merged
merged 1 commit into from Jan 8, 2019

Conversation

@rdy4ever
Copy link
Contributor

rdy4ever commented Jan 1, 2019

Questions Answers
Branch? 1.7.5.x
Description? #11579 introduced INTL_IDNA_VARIANT_UTS46 in the idn_to_ascii function, in order to comply with PHP 7.2 standards.
However, this broke e-mail sending for servers running an outdated ICU version (< 4.6). Figures out, there are more shared hostings that run outdated ICU versions with new PHP versions than expected and we should expect to see more and more issues regarding mail sending as more people will upgrade to PS 1.7.5.0.
This PR aims to silently fix this issue. If INTL_IDNA_VARIANT_UTS46 is defined, use it to comply with PHP 7.2 standards. If it's not defined, then use the old INTL_IDNA_VARIANT_2003 as long as it is still supported by PHP (up to the next major release, which will be 8.0.0). In PHP 7.2.0 up to 7.4.0, using INTL_IDNA_VARIANT_2003 will trigger E_DEPRECATED notices while still being the default for the idn_to_ascii function and still working. From PHP 7.4.0 up to next major release, the default will be INTL_IDNA_VARIANT_UTS46 but INTL_IDNA_VARIANT_2003 will still work with warnings. From PHP 8.0.0, we cannot longer use INTL_IDNA_VARIANT_2003 so we'll use the current default. If a server will run the outdated ICU <4.6 and PHP >= 8.0.0, an Exception will be triggered, but untill PS will be compatible with PHP 8.0 there is enough time to for most hosts to upgrade their ICU version.
Type? bug fix
Category? CO
BC breaks? Does it break backward compatibility?no
Deprecations? Does it deprecate an existing feature? no
Fixed ticket? Fixes #11933, #11949
How to test? Try to send an email from the order page from a server running intl ICU < 4.6. With this fix, it will work. Without, you will get an error 500 both in BO and in FO when customer tries to register, reset password, etc.

This change is Reviewable

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Jan 1, 2019

Hi,

thanks for the explanations and for the patch @rdy4ever.

I think the max version of PHP support of PrestaShop 1.7 will be PHP 7, it's already really hard to support 2 major versions of PHP I don't think it will be wise to try to support 3.

Would you mind to apply the linter on your pull request?

composer install && ./vendor/bin/php-cs-fixer fix

Mickaël

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Jan 1, 2019

ping @eternoendless , we should consider backporting this in 1.7.5.1 of needed

@rdy4ever

This comment has been minimized.

Copy link
Contributor Author

rdy4ever commented Jan 1, 2019

hi @mickaelandrieu
Just linted it.

@rdy4ever

This comment has been minimized.

Copy link
Contributor Author

rdy4ever commented Jan 1, 2019

OK, seems I don't know how to properly lint this. PS: I'm on Windows/VS Code

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Jan 1, 2019

Me too when I'm playing video games, but I use WSL (Windows Subsystem Linux) with zsh.

capture

We are not really satisfied asking everyone to fix coding styles, we're looking for a more "contributor-friendly" solution while keeping the quality of code high, more to come later :)

Thanks @rdy4ever

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 2, 2019

Both constants are different and produce a different result :/ (http://www.unicode.org/reports/tr46/#IDNAComparison%3E)
We need to be sure everything will be ok!

@rdy4ever

This comment has been minimized.

Copy link
Contributor Author

rdy4ever commented Jan 2, 2019

Thanks @mickaelandrieu
Do you think we could merge this in 1.7.5.1?
I have a feeling we’ll be seing more bug reports as people upgrade to 1.7.5.0.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 2, 2019

I think we need to target 1.7.5.1. @eternoendless This one is important.

Show resolved Hide resolved classes/Mail.php Outdated
Show resolved Hide resolved classes/Mail.php Outdated

@eternoendless eternoendless added this to the 1.7.5.1 milestone Jan 2, 2019

@eternoendless
Copy link
Member

eternoendless left a comment

Looks good to me

@eternoendless eternoendless force-pushed the rdy4ever:FIx-undefined-INTL_IDNA_VARIANT_UTS46 branch from 77e21c1 to 1de0fc4 Jan 2, 2019

@eternoendless eternoendless changed the base branch from develop to 1.7.5.x Jan 2, 2019

@eternoendless
Copy link
Member

eternoendless left a comment

Rebased into 1.7.5.x and squashed

@MathiasReker

This comment has been minimized.

Copy link
Contributor

MathiasReker commented Jan 3, 2019

@rdy4ever to use PHP CS Fixer on Windows:

cd repo
git checkout branch
./vendor/friendsofphp/php-cs-fixer/php-cs-fixer
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 7, 2019

@rdy4ever / @eternoendless, it seems we need a push to be made on this PR in order to trigger Travis.

Fix undefined INTL_IDNA_VARIANT_UTS46 if intl ICU < 4.6
Use INTL_IDNA_VARIANT_UTS46 when possible, otherwise switch to the old
value INTL_IDNA_VARIANT_2003

@PierreRambaud PierreRambaud force-pushed the rdy4ever:FIx-undefined-INTL_IDNA_VARIANT_UTS46 branch from 1de0fc4 to 968a2cf Jan 7, 2019

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 7, 2019

Done, waiting for CI :)

@Quetzacoalt91 Quetzacoalt91 merged commit 0d4177a into PrestaShop:1.7.5.x Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 8, 2019

Thank you all!

@nataniko

This comment has been minimized.

Copy link

nataniko commented Feb 14, 2019

This bug is very important to fix. None of the payment methods are work in my prestashop 1.7.5.0 installation no matter php 5.6 or 7.0
This also cause an "unexpected error" if pay via PayPal express.
ICU v4.2.1

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Feb 14, 2019

Hi @nataniko this bug has been solved by this PR and will be shipped with patch version 1.7.5.1 which will be released very soon 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment