Problem/Motivation

PHPStorm can automatically replace strpos and substr with the applicable PHP 8 function for more readable and easier to grok code.

Steps to reproduce

Run the PHPStorm scanner and fixer.

Proposed resolution

Do it. Decide if we want to backport to Drupal 9.5 (we have a polyfil).

Remaining tasks

  1. #3328443: Replace most strpos() === 0 or !== 0 with str_starts_with()
  2. #3328454: Replace most strpos() !== FALSE or === FALSE with str_contains()
  3. #3328456: Replace substr($a, 0, $i) with str_starts_with()
  4. #3328457: Replace most substr($a, $i) where $i is negative with str_ends_with()
  5. Rebase this MR once those are in and fix the last few references.
  6. Review MR
  7. Commit

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3324560

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

alexpott created an issue. See original summary.

alexpott opened merge request !3049

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

This change will need https://www.drupal.org/project/drupal/issues/3324540 to land because it change locale.module and that has a PHPCS issue in it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Overall this looks good, but a couple of questions about vendor-copied code. Also this is probably disruptive to other patches given it touches a large number of files, so I think either we do it now to all active branches (including 9.5.x to make backports easier) or we wait until the beta window of 10.1.x and do it then.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Also this is probably disruptive to other patches given it touches a large number of files

I'm not convinced - it's single line changes in a few files. Given now we have MRs if people use them this type of change is not actually disruptive.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Also is this worth raising a coding standards issue over? Coder could prevent this from creeping back in, I guess.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Re coding standards - if there was a PHPCS rule that we could just enable - or something in PHPStan then sure but I don't think this is worth our time to implement ourselves.

Re files from other projects I think given that there is no logic change here I think it is fine to change them. If we copied them back again we could either re-do the changes or not - as the logic is the same I think it doesn't really matter.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

OK, points taken. This definitely makes things more readable in many cases so I think we should try to get this in.

FWIW I couldn't find anything for PHPCS but PHP-CS-Fixer does have a rule for it: https://cs.symfony.com/doc/rules/alias/modernize_strpos.html - but adopting that is a whole other can of worms to open.

I think if we are to do this we should backport as well to keep things in sync as much as possible, helping us avoid separate patches for future bug fixes.

Rajeshreeputra made their first commit to this issue’s fork.

catch’s picture

catch
he/him
Primary language English
commented

Removing commit credit for a rebase-only change to the MR.

Found one out of scope change, maybe phpstan being greedy?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The out of scope change is due to PHPStorm automatically changing list() to [].

We fixed it once across core in #3222769: [November 8, 2021] Replace all list (array destructuring) assignment to the array syntax

Then it was accidentally reverted again in #3181778: [w/c September 17th] Replace t() with $this->t() in all plugins

This is the only instance of list() in core so I think we should let this slide and fix it again here.

xjm’s picture

xjm
she/her
Primary language English
commented

Hm, I might've split this into two MRs. 282 lines is sort of on the outer edge for multiple replace patterns. That said, it'd be more work at this point to split it than to just spend the extra time reviewing the different patterns.

The random list() really gets to me though. So I made #3327950: Remove last remaining list() from core. :P

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: Unassigned » xjm

OK actually I'm definitely feeling directed attention fatigue with the multiple replace patterns. I'll update the MR after #3327950: Remove last remaining list() from core lands since I'm the one advocating that it should have its own commit. When I do that, I might also split up the MR up myself -- if nothing else, that will help me concentrate on the changes.

xjm’s picture

xjm
she/her
Primary language English
commented
Category: Task » Plan
Status: Reviewed & tested by the community » Needs work

Working on it now.

xjm’s picture

xjm
she/her
Primary language English
commented

One question: Should we actually be changing the code in Drupal\Component\Annotation\Doctrine? It's ignored for phpcs purposes.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Active
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

 

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dimitriskr made their first commit to this issue’s fork.

dimitriskr’s picture

dimitriskr commented
Issue summary: View changes
Status: Active » Needs work

Updated remaining tasks. No other references are found.
I saw that in some previous commits, some preg_match() was switched with str_starts_with(). I didn't check other instances of preg_match() that could be replaces with these new functions.

dimitriskr’s picture

dimitriskr commented
Status: Needs work » Needs review
mstrelan’s picture

mstrelan commented

I decided to check with rector if it could find any more to replace. I applied the rules StrContainsRector, StrStartsWithRector and StrEndsWithRector. It found one additional file to update, which I have pushed a commit for. The change replaces strpos() !== FALSE with str_contains(substr()) which means more function calls, but I think these are more readable than before. Happy to revert if others disagree.

dimitriskr’s picture

dimitriskr commented

Hey mstrelan,

Thank you for looking into this issue
I don't see a commit pushed to this branch/MR

mstrelan’s picture

mstrelan commented

Ah I forgot to request push access. Have pushed now.

mstrelan changed the visibility of the branch 11.x to hidden.

mstrelan’s picture

mstrelan commented

Rebased against latest 11.x and also push an 11.x branch to the fork to workaround ci issues with spell check job.

catch’s picture

catch
he/him
Primary language English
commented
Assigned: xjm » Unassigned
Status: Needs review » Reviewed & tested by the community

This looks good, it's no longer touching the Doctrine forked code, can't see anything to complain about.

quietone’s picture

quietone commented
Title: Replace strpos/substr with PHP 8's str_starts_with() / str_contains() / str_ends_with() » Replace strpos/substr with str_starts_with() / str_contains() / str_ends_with()

Shorten the title

  • quietone committed d74c1cad on 11.x 
    Issue #3324560 by alexpott, mstrelan, dimitriskr, xjm, longwave, catch:...

  • quietone committed e5ce10b4 on 10.3.x 
    Issue #3324560 by alexpott, mstrelan, dimitriskr, xjm, longwave, catch:...
quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Fixed

I also didn't find anything here to prevent this being committed.

Committed d74c1ca and pushed to 11.x and e5ce10b and pushed to 10.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.