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

Bugfix: Fix unescaped dash in character group in regex #14223

Merged
merged 1 commit into from Jul 2, 2019

Conversation

@mvorisek
Copy link
Contributor

commented Jun 14, 2019

Questions Answers
Branch? develop
Description? PHP needs 2 backslashes to produce one backslash in the string itself.
Regex needs 2 backslashes to produce one backslash in the regex itself.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? Try with php7.3 and PHP7.1

This change is Reviewable

@mvorisek mvorisek requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 14, 2019

@prestonBot

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

Hello @mvorisek!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@mvorisek mvorisek closed this Jun 14, 2019

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Resubmit #14224 (review) to develop branch.

@mvorisek mvorisek reopened this Jun 17, 2019

@mvorisek mvorisek changed the title Fix PHP typo Bugfix: Fix unescaped dash in character group in regex Jun 17, 2019

@PierreRambaud PierreRambaud removed the 1.7.6.x label Jun 17, 2019

@PierreRambaud PierreRambaud added this to the 1.7.7.0 milestone Jun 17, 2019

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Can you please merge so I can delete the fork?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Regarding the tests look like your fix break the autoloader 🤔

@PierreRambaud
Copy link
Contributor

left a comment

Need investigation to find why tests are failing.

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Can you confirm that the build is broken by exactly this PR? Try to cherry pick the commit on top of some non-failing branch.

The regexes seems to be working for me, see https://3v4l.org/hBE88

I have also added there an example why the 4 backslashes are important to properly escape 1 backslash.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

yes it's because of your PR :/
It breaks the Autoloader

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Oh, I think I found it. The original code seems to me to be actually wrong:

$namespacePattern = '[\\a-z0-9_]*[\\]';

parses to string [\a-z0-9_]*[\], the first backslash is (currently) honored in the regex as the next char does not need an escape char and it is withing character group (where escape characters are currently handled less strictly).

But the second backslash is is escaping the following ] bracket. And due to the usage later with additional ] (which closes the character group) it "works/compiles".

From my point of view the "namespace pattern" is not intended to parse any ] characters.

Is it the case?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

The best way to test if everything is ok is to update your PR :D

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Oh, I think I found it. The original code seems to me to be actually wrong:

Yeah, the code was wrong and it worked by coincidences.

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

The code is final and passing the CI tests. Can it be reviewed and merged?

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Thank you @mvorisek

@matks matks merged commit 0144bac into PrestaShop:develop Jul 2, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.