Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Leverage str_contains/str_starts_with#41576
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJun 7, 2021
Hey! I think@fancyweb has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
| } | ||
| if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/',$name,$matches) ||0 ===strpos($name,'@')) { | ||
| if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/',$name,$matches) ||str_starts_with($name,'@')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not directly related and maybe micro optimisation, but wouldn't it be faster to do:
| if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/',$name,$matches) ||str_starts_with($name,'@')) { | |
| if (str_starts_with($name,'@') ||!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/',$name,$matches)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, probably. But I'd like to keep the change list straightforward and not include additional manual changes for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes definitely not part of this PR 👍
This PR was merged into the 4.4 branch.Discussion----------[FrameworkBundle] Remove duplicate catch block| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Spottet while reviewing#41576 by `@derrabus`Commits-------32cac1c Remove duplicate catch block
79a37f3 to2ee828bCompare
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A big 👍 from me ... new code is so much more readable 😍
fabpot commentedJul 6, 2021
Even if I appreciate the effort in modernizing the code base, I don't see any benefits of such a change. The code is not really different and it introduces a new polyfill as a dep (which might make the code slower). |
noniagriconomie commentedJul 6, 2021
@fabpot hi,
according tohttps://packagist.org/packages/symfony/polyfill-php80/dependents?order_by=downloads, this "internal" package is required on major symfony packages already
this indeed can be the main reason, when running on sf v4 or v5 without php v8n thus leveraging the polyfill, but I can not tell if it has an impact or not :s
I do not understand the thx |
fabpot commentedJul 6, 2021
That's a typo, I meant 8.0 |
Tobion commentedJul 6, 2021 • edited by OskarStark
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by OskarStark
Uh oh!
There was an error while loading.Please reload this page.
@fabpot as already said by@noniagriconomie, the php80 polyfill is already a requirement in 4.4 by major symfony components like httpkernel and console. So it's already part of every symfony app. So there is no additional dependency. I'd also argue it really makes the code mroe readable and it clarifies the intent. There are several workarounds implementations, some better suited than others (see rfc). By using the standard functions it also normalizes those inconsistencies. You said it might make the code slower. I'd say it makes it faster, especially on PHP 8.0+. One of the arguments for introducing the functions was that the alternatives are not efficient. So to me there is no downside here and it makes the code future-proof. Please reconsider. |
fabpot commentedJul 6, 2021
ok, let's do it then, but we need to enable the rule on PHP CS fixer to be sure we will catch "old" usages in PRs. |
derrabus commentedJul 6, 2021
We need to get the rule merged into CS Fixer then. 😓 |
This PR was merged into the 4.4 branch.Discussion----------Leverage str_ends_with| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets || License | MIT| Doc PR |added the php80 polyfill to requirements when necessary. some components already had the requirement anyway.Related to#41576Commits-------9d80729 Leverage str_ends_with
Signed-off-by: Alexander M. Turek <me@derrabus.de>
2ee828b toe585b26Comparenicolas-grekas commentedJul 21, 2021
Thank you@derrabus. |
| $intlDomain =$domain; | ||
| $suffixLength =\strlen(self::INTL_DOMAIN_SUFFIX); | ||
| if (\strlen($domain) <$suffixLength ||false ===strpos($domain,self::INTL_DOMAIN_SUFFIX, -$suffixLength)) { | ||
| if (\strlen($domain) <$suffixLength ||!str_contains($domain,self::INTL_DOMAIN_SUFFIX, -$suffixLength)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this change is wrong, it might be good to report it to php-cs-fixer
This PR was merged into the 4.4 branch.Discussion----------[Dotenv][Yaml] Remove PHP 8.0 polyfill| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#42280| License | MIT| Doc PR | N/AThis is a partial revert of#41576 and#41973.Commits-------08ecbf5 Remove polyfills from Yaml and Dotenv
…ndrik Luup)This PR was merged into the 4.4 branch.Discussion----------Do not use str_starts_with in translation-status.php| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | N/A| License | MIT| Doc PR | N/A`translation-status.php` is a standalone script that might be run with PHP version that is less than <8.0 where `str_starts_with` is not available. I propose that we revert the change that was made in#41576 instead of trying to load the polyfill somehow.Commits-------94d843f Do not use str_start_with
nicolas-grekas commentedAug 19, 2021
Linking tocomposer/composer#10024 for references. |
…lmann)This PR was submitted for the 5.3 branch but it was merged into the 4.4 branch instead.Discussion----------[ExpressionLanguage] [Lexer] Remove PHP 8.0 polyfill| Q | A| ------------- | ---| Branch | 5.3| Bug fix | yes| New feature | no| Deprecations | no| Tickets |Fix#42280| License | MIT| Doc PR | N/AThis is a partial revert of#41576 and is a followup to#42296Commits-------d2f39e9 Remove polyfills from ExpressionLanguage
This PR was merged into the 4.4 branch.Discussion----------Leverage str_ends_with| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets || License | MIT| Doc PR |added the php80 polyfill to requirements when necessary. some components already had the requirement anyway.Related tosymfony#41576Commits-------9d80729 Leverage str_ends_with
Uh oh!
There was an error while loading.Please reload this page.
I'd like to use
str_contains()andstr_starts_with()whenever possible. On PHP 8, this is a native function and for all earlier versions, we can maintain the most efficient way to perform those operations in the polyfill package. And apart from that, I find the new functions more intuitive than thestrpos()expressions I'm replacing here.All code changes in this PR were automated, seePHP-CS-Fixer/PHP-CS-Fixer#5754
Of course, this is more than just a CS change. If you don't feel comfortable merging this change into 4.4, I can easily redo the PR for 5.4.