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

Add compile-time checks for the "matches" operator #4330

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

fabpot
Copy link
Contributor

@fabpot fabpot commented Sep 21, 2024

No description provided.

@fabpot fabpot force-pushed the matches-detect-issues-at-compile-time branch 2 times, most recently from 04f1839 to 3e4bcdc Compare September 21, 2024 08:27
@fabpot fabpot force-pushed the matches-detect-issues-at-compile-time branch from 3e4bcdc to d1beac4 Compare September 21, 2024 08:29
{
if ($right instanceof ConstantExpression) {
$regexp = $right->getAttribute('value');
set_error_handler(static fn ($t, $m) => throw new SyntaxError(\sprintf('Regexp "%s" passed to "matches" is not valid: %s.', $regexp, substr($m, 14)), $lineno));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number 14 feels like a bit magical to me, can we add a comment or use strlen('...') (which will be inlined to 14) instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also somehow doesn't look right:

Twig\Error\RuntimeError: Regexp "3" passed to "matches" is not valid: Delimiter must not be alphanumeric%sbackslash%sin "index.twig" at line 2

It seems like there's at least a space missing before in "index.twig" at line 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message is correct, here, this is only the format to cover differences between PHP versions.

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

Successfully merging this pull request may close these issues.

3 participants