Skip to content

[Validator] Simplify NoSuspiciousCharactersValidator #54062

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

Conversation

MatTheCat
Copy link
Contributor

Q A
Branch? 7.0
Bug fix? no
New feature? no
Deprecations? no
Issues N/A
License MIT

php/php-src#10647 has been fixed in PHP 8.1.17. Now that Symfony requires PHP ≥ 8.2, we can avoid calling Spoofchecker::isSuspicious for every check by leveraging its $errorCode parameter.

@carsonbot carsonbot added this to the 7.1 milestone Feb 26, 2024
@MatTheCat MatTheCat changed the base branch from 7.1 to 7.0 February 26, 2024 11:14
@nicolas-grekas nicolas-grekas modified the milestones: 7.1, 7.0 Feb 26, 2024
@MatTheCat
Copy link
Contributor Author

MatTheCat commented Feb 27, 2024

I forgot the case where many checks fail at once; adding a test. This will also solve #54062 (comment).

@MatTheCat MatTheCat force-pushed the simplify_no-suspicious-characters-validator branch from 816e1fc to c24e3e7 Compare February 27, 2024 12:54
@nicolas-grekas
Copy link
Member

Thank you @MatTheCat.

@nicolas-grekas nicolas-grekas merged commit f8611cf into symfony:7.0 Feb 27, 2024
@MatTheCat MatTheCat deleted the simplify_no-suspicious-characters-validator branch February 27, 2024 15:58
->assertRaised();
;

while ($message = next($errors)) {
Copy link
Member

Choose a reason for hiding this comment

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

foreach ($errors as $code => $message) would be a lot more readable IMO (you will need a special handling of the first item s next ones to create $violations)

[NoSuspiciousCharacters::HIDDEN_OVERLAY_ERROR => 'Using hidden overlay characters is not allowed.'],
];

yield 'Fails both HIDDEN_OVERLAY and RESTRICTION_LEVEL checks' => [
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this test be added in older branches as well (and so all the test refactoring) ?

xabbuh added a commit that referenced this pull request Nov 14, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[Validator] fix compatibility with PHP < 8.2.4

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The fix for php/php-src#10647 (on which #54062) relies on was first released with PHP 8.2.4.

Commits
-------

2d713ea fix compatibility with PHP < 8.2.4
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.

6 participants