Leverage str_contains/str_starts_with #41576

derrabus · 2021-06-06T23:23:05Z

Q	A
Branch?	4.4
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets	N/A
License	MIT
Doc PR	N/A

I'd like to use str_contains() and str_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 the strpos() expressions I'm replacing here.

All code changes in this PR were automated, see PHP-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.

carsonbot · 2021-06-07T19:26:06Z

Hey!

I think @fancyweb has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

OskarStark · 2021-06-07T19:42:51Z

src/Symfony/Bundle/FrameworkBundle/Templating/TemplateNameParser.php

            throw new \RuntimeException(sprintf('Template name "%s" contains invalid characters.', $name));
        }

-        if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches) || 0 === strpos($name, '@')) {
+        if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches) || str_starts_with($name, '@')) {


Not directly related and maybe micro optimisation, but wouldn't it be faster to do:

Suggested change

if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches) || str_starts_with($name, '@')) {

if (str_starts_with($name, '@') || !preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches)) {

Yes, probably. But I'd like to keep the change list straightforward and not include additional manual changes for now.

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 #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Spottet while reviewing #41576 by `@derrabus` Commits ------- 32cac1c Remove duplicate catch block

javiereguiluz

A big 👍 from me ... new code is so much more readable 😍

fabpot · 2021-07-06T07:43:45Z

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).
Pondering the pros and the cons, I'm 👎 for such a change until we require 8.1+.

noniagriconomie · 2021-07-06T09:08:03Z

@fabpot hi,

The code is not really different and it introduces a new polyfill as a dep

according to https://packagist.org/packages/symfony/polyfill-php80/dependents?order_by=downloads, this "internal" package is required on major symfony packages already

(which might make the code slower)

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

until we require 8.1+.

I do not understand the 8.1+, sf v6 which require php v8.0+ could be a good candidate no?

thx

fabpot · 2021-07-06T09:13:55Z

until we require 8.1+.

I do not understand the 8.1+, sf v6 which require php v8.0+ could be a good candidate no?

That's a typo, I meant 8.0

Tobion · 2021-07-06T09:58:52Z

@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.
Another argument is that SF 4.4 will be maintained a year longer than PHP 7.4. So at one point symfony 4.4 users are likely to run on PHP 8. Then they will get the performance increase automatically.

So to me there is no downside here and it makes the code future-proof. Please reconsider.

fabpot · 2021-07-06T11:48:56Z

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 · 2021-07-06T18:46:29Z

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  | Deprecations? | no  | Tickets | | License | MIT | Doc PR | added the php80 polyfill to requirements when necessary. some components already had the requirement anyway. Related to #41576 Commits ------- 9d80729 Leverage str_ends_with

Signed-off-by: Alexander M. Turek <me@derrabus.de>

nicolas-grekas · 2021-07-21T12:20:54Z

Thank you @derrabus.

nicolas-grekas · 2021-07-21T13:13:51Z

src/Symfony/Component/Translation/MessageCatalogue.php

@@ -166,7 +166,7 @@ public function add($messages, $domain = 'messages')
        }
        $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)) {


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/A This 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 · 2021-08-19T09:00:53Z

Linking to composer/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/A This is a partial revert of #41576 and is a followup to #42296 Commits ------- 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  | Deprecations? | no  | Tickets | | License | MIT | Doc PR | added the php80 polyfill to requirements when necessary. some components already had the requirement anyway. Related to symfony#41576 Commits ------- 9d80729 Leverage str_ends_with

derrabus requested review from chalasr, dunglas, jderusse, sroze, wouterj, xabbuh and yceruto as code owners June 6, 2021 23:23

carsonbot added the Status: Needs Review label Jun 6, 2021

carsonbot added this to the 4.4 milestone Jun 6, 2021

OskarStark reviewed Jun 7, 2021

View reviewed changes

OskarStark mentioned this pull request Jun 7, 2021

[FrameworkBundle] Remove duplicate catch block #41596

Merged

derrabus force-pushed the improvement/str-contains branch from 79a37f3 to 2ee828b Compare June 8, 2021 20:19

javiereguiluz approved these changes Jun 10, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 10, 2021

This was referenced Jul 4, 2021

[FrameworkBundle] fix removing "action" from route names #41782

Merged

Leverage str_ends_with #41973

Merged

nicolas-grekas mentioned this pull request Jul 21, 2021

[6.0] Update php8 string function #42209

Closed

Leverage str_contains/str_starts_with

e585b26

Signed-off-by: Alexander M. Turek <me@derrabus.de>

nicolas-grekas force-pushed the improvement/str-contains branch from 2ee828b to e585b26 Compare July 21, 2021 12:20

nicolas-grekas merged commit c7dc7f8 into symfony:4.4 Jul 21, 2021

nicolas-grekas reviewed Jul 21, 2021

View reviewed changes

derrabus deleted the improvement/str-contains branch July 21, 2021 16:25

derrabus mentioned this pull request Jul 27, 2021

[Dotenv][Yaml] Remove PHP 8.0 polyfill #42296

Merged

hluup mentioned this pull request Aug 13, 2021

Do not use str_starts_with in translation-status.php #42533

Merged

nigelmann mentioned this pull request Aug 16, 2021

[ExpressionLanguage] [Lexer] Remove PHP 8.0 polyfill #42585

Merged

fancyweb mentioned this pull request Dec 7, 2021

Use str_ends_with() and str_starts_with() #44482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leverage str_contains/str_starts_with #41576

Leverage str_contains/str_starts_with #41576

derrabus commented Jun 6, 2021 •

edited

Loading

carsonbot commented Jun 7, 2021

OskarStark Jun 7, 2021

derrabus Jun 7, 2021

OskarStark Jun 8, 2021

javiereguiluz left a comment

fabpot commented Jul 6, 2021

noniagriconomie commented Jul 6, 2021

fabpot commented Jul 6, 2021

Tobion commented Jul 6, 2021 •

edited by OskarStark

Loading

fabpot commented Jul 6, 2021

derrabus commented Jul 6, 2021

nicolas-grekas commented Jul 21, 2021

nicolas-grekas Jul 21, 2021

nicolas-grekas commented Aug 19, 2021

	if (!preg_match('/^(?:([^:]):([^:]):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches) \|\| str_starts_with($name, '@')) {
	if (str_starts_with($name, '@') \|\| !preg_match('/^(?:([^:]):([^:]):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches)) {

Leverage str_contains/str_starts_with #41576

Leverage str_contains/str_starts_with #41576

Conversation

derrabus commented Jun 6, 2021 • edited Loading

carsonbot commented Jun 7, 2021

OskarStark Jun 7, 2021

Choose a reason for hiding this comment

derrabus Jun 7, 2021

Choose a reason for hiding this comment

OskarStark Jun 8, 2021

Choose a reason for hiding this comment

javiereguiluz left a comment

Choose a reason for hiding this comment

fabpot commented Jul 6, 2021

noniagriconomie commented Jul 6, 2021

fabpot commented Jul 6, 2021

Tobion commented Jul 6, 2021 • edited by OskarStark Loading

fabpot commented Jul 6, 2021

derrabus commented Jul 6, 2021

nicolas-grekas commented Jul 21, 2021

nicolas-grekas Jul 21, 2021

Choose a reason for hiding this comment

nicolas-grekas commented Aug 19, 2021

derrabus commented Jun 6, 2021 •

edited

Loading

Tobion commented Jul 6, 2021 •

edited by OskarStark

Loading