Problem/Motivation

Note:
If you only want to determine if a particular needle occurs within haystack, use the faster and less memory intensive function strpos() instead.

-- @see https://www.php.net/manual/en/function.strstr.php

Proposed resolution

For example:

-        $this->assertTrue(strstr($style, 'width: 555px;') !== FALSE, 'Dialog width respected.');
+        $this->assertTrue(strpos($style, 'width: 555px;') !== FALSE, 'Dialog width respected.');

Current occurrences in core

grep -nri 'str[i]*str\(.*\) [!=]== FALSE' core
core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php:108:    $this->assertTrue(stristr($top_form_elements[0]->getText(), 'Title field is required.') !== FALSE);
core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php:132:        if (strstr($exception->getMessage(), 'not clickable') === FALSE) {
core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTestBase.php:88:    if (stristr($this->getUrl(), 'admin/structure') === FALSE) {
core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php:153:      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'), 'private') !== FALSE, 'Cache-Control header was sent.');
core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php:61:        $this->assertTrue(strstr($style, 'width: 555px;') !== FALSE, 'Dialog width respected.');
core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php:65:        $this->assertTrue(strstr($style, 'width: 555px;') === FALSE, 'Dialog width reset to default.');
core/modules/settings_tray/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php:154:    $this->assertTrue(strstr($href, "/admin/structure/block/manage/custom/settings-tray?destination=$destination") !== FALSE);
core/modules/filter/src/Plugin/Filter/FilterAlign.php:27:    if (stristr($text, 'data-align') !== FALSE) {
core/modules/filter/src/Plugin/Filter/FilterCaption.php:31:    if (stristr($text, 'data-caption') !== FALSE) {
core/modules/responsive_image/tests/src/Functional/ResponsiveImageFieldDisplayTest.php:265:      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'), 'private') !== FALSE, 'Cache-Control header was sent.');
core/modules/editor/src/Plugin/Filter/EditorFileReference.php:69:    if (stristr($text, 'data-entity-type="file"') !== FALSE) {
core/modules/editor/src/EditorXssFilter/Standard.php:102:    if (stristr($html, 'data-') !== FALSE) {
core/modules/media/src/Plugin/Filter/MediaEmbed.php:280:    if (stristr($text, '<drupal-media') === FALSE) {

Remaining tasks

Replace usages

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#30 interdiff.22-30.txt4.1 KBspokje
#30 3132758-30.patch8.01 KBspokje
#28 interdiff_22-28.txt1.2 KBsahil rohilla01
#28 3132758-28.patch1.2 KBsahil rohilla01
#27 3132758-27.patch1.2 KBsahil rohilla01
#27 interdiff_22-27.txt1.2 KBsahil rohilla01
#22 interdiff_13-22.txt7.05 KBspokje
#22 3132758-22.patch4.14 KBspokje
#13 3132758-13-interdiff.txt1.36 KBkim.pepper
#13 3132758-13.patch10.85 KBkim.pepper
#11 3132758-11-interdiff.txt3 KBkim.pepper
#11 3132758-11.patch9.49 KBkim.pepper
#9 3132758-9-interdiff.txt3.68 KBkim.pepper
#9 3132758-9.patch7.44 KBkim.pepper
#7 3132758-7.patch3.76 KBkim.pepper

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Title: Replace strstr() with strpos where possible » Replace strstr() with strpos() where possible
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Issue summary: View changes
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Issue summary: View changes
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Title: Replace strstr() with strpos() where possible » Replace strstr()/stristr() with strpos()/stripos() where possible
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes

Did a search for occurrences in core, and added to IS.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
StatusFileSize
new 3132758-7.patch3.76 KB

Seems like a quick fix.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Needs work

Thanks @kim.pepper, But I guess you missed checking stristr()/ replacing stristr() with stripos()

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 3132758-9.patch7.44 KB
new 3132758-9-interdiff.txt3.68 KB

Oh nice catch.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes
StatusFileSize
new 3132758-11.patch9.49 KB
new 3132758-11-interdiff.txt3 KB

Also need to check for === FALSE.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Needs work 
--- a/core/lib/Drupal/Component/Assertion/Inspector.php
+++ b/core/lib/Drupal/Component/Assertion/Inspector.php
@@ -306,7 +306,7 @@ public static function assertAllNumeric($traversable) {
    * @param mixed $traversable
    *   Variable to examine.
    * @param bool $case_sensitive
-   *   TRUE to use strstr(), FALSE to use stristr() which is case insensitive.
+   *   TRUE to use strpos(), FALSE to use stripos() which is case insensitive.
    *
    * @return bool
    *   TRUE if $traversable can be traversed and all members are strings
@@ -316,14 +316,14 @@ public static function assertAllMatch($pattern, $traversable, $case_sensitive =
     if (static::assertTraversable($traversable)) {
       if ($case_sensitive) {
         foreach ($traversable as $member) {
-          if (!(is_string($member) && strstr($member, $pattern))) {
+          if (!(is_string($member) && strpos($member, $pattern) !== FALSE)) {
             return FALSE;
           }
         }
       }
       else {
         foreach ($traversable as $member) {
-          if (!(is_string($member) && stristr($member, $pattern))) {
+          if (!(is_string($member) && stripos($member, $pattern) !== FALSE)) {
             return FALSE;

+++ b/core/lib/Drupal/Component/Assertion/Inspector.php
@@ -306,7 +306,7 @@ public static function assertAllNumeric($traversable) {
    * @param mixed $traversable
    *   Variable to examine.
    * @param bool $case_sensitive
-   *   TRUE to use strstr(), FALSE to use stristr() which is case insensitive.
+   *   TRUE to use strpos(), FALSE to use stripos() which is case insensitive.
    *
    * @return bool
    *   TRUE if $traversable can be traversed and all members are strings
@@ -316,14 +316,14 @@ public static function assertAllMatch($pattern, $traversable, $case_sensitive =

Almost perfect, 4 more lines to do.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 3132758-13.patch10.85 KB
new 3132758-13-interdiff.txt1.36 KB

Fixes #12

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Needs work

@kim.pepper, I am really sorry, actually, looking into it again, searching by strstr/stristr is more appropriate. So there are more to replace.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Postponed

As discussed in slack, there are a number of occurrences in test assertions that should probably be cleaned up first.

Postponing on #3132778: Replace usages of assertions with strstr with assertStringContainsString()/assertStringNotContainsString()

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Related issues: +#3132778: Replace usages of assertions with strstr with assertStringContainsString()/assertStringNotContainsString()
mondrake’s picture

mondrake
Location 🇮🇹
commented 
+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -150,7 +150,7 @@ public function _testImageFieldFormatters($scheme) {
-      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'), 'private') !== FALSE, 'Cache-Control header was sent.');
+      $this->assertTrue(strpos($this->drupalGetHeader('Cache-Control'), 'private') !== FALSE, 'Cache-Control header was sent.');
 

+++ b/core/modules/responsive_image/tests/src/Functional/ResponsiveImageFieldDisplayTest.php
@@ -262,7 +262,7 @@ protected function doTestResponsiveImageFieldFormatters($scheme, $empty_styles =
-      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'), 'private') !== FALSE, 'Cache-Control header was sent.');
+      $this->assertTrue(strpos($this->drupalGetHeader('Cache-Control'), 'private') !== FALSE, 'Cache-Control header was sent.');

these can be replaced by $this->assertSession()->responseHeaderContains() and in fact are included in #3131186-8: Replace assertions involving calls to drupalGetHeader() with session-based assertions, where possible

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Related issues: +#3131186: Replace assertions involving calls to drupalGetHeader() with session-based assertions, where possible

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

spokje’s picture

spokje
Primary language Dutch
commented
Version: 9.3.x-dev » 9.4.x-dev
Status: Postponed » Needs work

Unpostponing since #3132778: Replace usages of assertions with strstr with assertStringContainsString()/assertStringNotContainsString() and #3131186: Replace assertions involving calls to drupalGetHeader() with session-based assertions, where possible are (long) in.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3132758-22.patch4.14 KB
new interdiff_13-22.txt7.05 KB

Patched against 9.4.x-dev

$ grep -nri 'str[i]*str\(.*\) [!=]== FALSE' core
core/modules/editor/src/EditorXssFilter/Standard.php:102:    if (stristr($html, 'data-') !== FALSE) {
core/modules/editor/src/Plugin/Filter/EditorFileReference.php:85:    if (stristr($text, 'data-entity-type="file"') !== FALSE) {
core/modules/filter/src/Plugin/Filter/FilterAlign.php:27:    if (stristr($text, 'data-align') !== FALSE) {
core/modules/filter/src/Plugin/Filter/FilterCaption.php:69:    if (stristr($text, 'data-caption') !== FALSE) {
core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTestBase.php:88:    if (stristr($this->getUrl(), 'admin/structure') === FALSE) {
core/modules/media/src/Plugin/Filter/MediaEmbed.php:280:    if (stristr($text, '<drupal-media') === FALSE) {
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Doing a search of core I found 9 instances of this including comments but the patch has 6 replacements. Could you recheck?

sahil rohilla01’s picture

sahil rohilla01 commented
StatusFileSize
new interdiff_22-27.txt1.2 KB
new 3132758-27.patch1.2 KB

I have made 2 replacement as mentioned in comment #26. Didn't find third one @smustgrave.

sahil rohilla01’s picture

sahil rohilla01 commented
StatusFileSize
new 3132758-28.patch1.2 KB
new interdiff_22-28.txt1.2 KB

Please Forgot above comment #27
I have made 2 replacement as mentioned in comment #26. but didn't find third one @smustgrave.

smustgrave’s picture

smustgrave commented

From what I can tell the patches in #27 and #28 don't contains the changes from #22

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3132758-30.patch8.01 KB
new interdiff.22-30.txt4.1 KB
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks @spokje!

znerol’s picture

znerol commented

PHP 8 introduced str_contains(). That should be used instead of strpos(). However, there is no replacement for stripos() afaik.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Postponed

Yeah, let's hold off on this in favor of #3324560: Replace strpos/substr with str_starts_with() / str_contains() / str_ends_with(). Thanks!

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.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.