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

Work around PHP bug #26

Merged
merged 1 commit into from Jun 8, 2019

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Copy link
Member

commented Jun 6, 2019

@@ -258,6 +258,11 @@ protected function scan($input)
$flags = PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_OFFSET_CAPTURE;
$matches = preg_split($regex, $input, -1, $flags);
if (false === $matches) {
// Work around https://bugs.php.net/78122
$matches = array(array($input, 0));

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jun 6, 2019

Member

Should we trigger a deprecation telling that input that triggers error described by preg_last_error will not be supported (as in, we throw an exception) in the next major version of the lexer? Or are we supposed to always allow this situation?

This comment has been minimized.

Copy link
@alcaeus

alcaeus Jun 7, 2019

Member

I don't think that's necessary: there was talk about replacing doctrine/lexer with HOA parser in doctrine/annotations and doctrine/dbal. We can always add the deprecation in 1.1 if we decide to do a 2.0 release.

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jun 7, 2019

Member

Oh ok I didn't realise this package was probably going to be abandoned. I still think a deprecation should be thrown as a way to tell the user they are probably misusing that method and that bad things may happen. That would mean a deprecation w/o any hint about a 2.0 version of the package:

Suggested change
$matches = array(array($input, 0));
@trigger_error(sprintf(
'input "%s" caused preg_split() to fail with error code %d (description at https://www.php.net/manual/en/pcre.constants.php). "%s" may not behave as expected',
$input,
preg_last_error(),
__METHOD__
), E_USER_DEPRECATED);
$matches = [[$input, 0]];
@alcaeus

alcaeus approved these changes Jun 7, 2019

Copy link
Member

left a comment

LGTM, but I'd like a second opinion from @Ocramius or @jwage.

Once merge, I'd like to tag 1.0.2 containing this fix.

@greg0ire

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Work around PHP bug

Technically not a bug as the link shows, it's more of a bugfix that is not BC, if I understand correctly.

Also, can someone please explain to me why this is needed? I'm not familiar with the lexer, I don't even know what it is lexing… DQL, probably? If this is it, in what situation would it be normal to have that kind of weird single-byte characters in a DQL string? And is the current behavior of the lexer correct in that case.

@SenseException

This comment has been minimized.

Copy link

commented Jun 7, 2019

Also: [] instead of array() syntax.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

Also: [] instead of array() syntax.

Please check the CS of the file. I'm just keeping it consistent.

@greg0ire this behavior change breaks the Symfony Validator test suite, in relation with the egulas strict email parser which uses this lexer.

Your concerns are for another PR to me.

@greg0ire

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Your concerns are for another PR to me.

Indeed, let's keep this about restoring the previous behavior

@greg0ire

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Here are the errors:

1) Symfony\Component\Validator\Tests\Constraints\EmailValidatorTest::testStrictWithInvalidEmails with data set #26 ('�@iana.org')
Invalid argument supplied for foreach()

/home/greg/dev/symfony/vendor/doctrine/lexer/lib/Doctrine/Common/Lexer/AbstractLexer.php:261
/home/greg/dev/symfony/vendor/doctrine/lexer/lib/Doctrine/Common/Lexer/AbstractLexer.php:96
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/EmailParser.php:41
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/Validation/RFCValidation.php:30
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/Validation/NoRFCWarningsValidation.php:21
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/EmailValidator.php:37
/home/greg/dev/symfony/src/Symfony/Component/Validator/Constraints/EmailValidator.php:79
/home/greg/dev/symfony/src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php:244

2) Symfony\Component\Validator\Tests\Constraints\EmailValidatorTest::testStrictWithInvalidEmails with data set #27 ('test@�.org')
Invalid argument supplied for foreach()

/home/greg/dev/symfony/vendor/doctrine/lexer/lib/Doctrine/Common/Lexer/AbstractLexer.php:261
/home/greg/dev/symfony/vendor/doctrine/lexer/lib/Doctrine/Common/Lexer/AbstractLexer.php:96
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/EmailParser.php:41
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/Validation/RFCValidation.php:30
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/Validation/NoRFCWarningsValidation.php:21
/home/greg/dev/symfony/vendor/egulias/email-validator/EmailValidator/EmailValidator.php:37
/home/greg/dev/symfony/src/Symfony/Component/Validator/Constraints/EmailValidator.php:79
/home/greg/dev/symfony/src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php:244

@alcaeus alcaeus merged commit 1febd6c into doctrine:master Jun 8, 2019

@alcaeus alcaeus added this to the 1.0.2 milestone Jun 8, 2019

@alcaeus alcaeus added the bug label Jun 8, 2019

@alcaeus alcaeus self-assigned this Jun 8, 2019

@alcaeus

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Released as part of 1.0.2. Thanks @nicolas-grekas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.