there are some places in drupal where it would be better(faster) to use strpos than strstr

8124 passes, 0 fails, and 0 exceptions

CommentFileSizeAuthor
#20 352880-replace-strstr-with-strpos-D7.patch3.78 KBDave Reid
#15 strstr_to_strpos.patch3.51 KBdawehner
#10 strstr_to_strpos_4.patch3.68 KBdawehner
#7 strstr_to_strpos_3.patch3.49 KBdawehner
#2 strstr_to_strpos_2.patch4.09 KBdawehner
strstr_to_strpos_1.patch4.04 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Needs work

You will need to strictly compare the result with FALSE, like this:

if (strpos($comment->comment, $keyword) !== FALSE || strpos($comment->subject, $keyword) !== FALSE)

... because strpos() can return 0.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
strstr_to_strpos_2.patch4.09 KB

thx

here is the new patch

8124 passes, 0 fails, and 0 exceptions

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Category: feature » task
Status: Needs review » Needs work

Doesn't apply any longer.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
strstr_to_strpos_3.patch3.49 KB

i rerolled this patch

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Needs work

Looks like we're missing the same !== FALSE on $checked = strpos($format->roles, ",$rid,");.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
strstr_to_strpos_4.patch3.68 KB

oh thx, for the review

next patch

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

rerole

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
strstr_to_strpos.patch3.51 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
352880-replace-strstr-with-strpos-D7.patch3.78 KB

Re-rolled and re-posted to catch the testing bot again.

c960657’s picture

c960657 CreditAttribution: c960657 commented

Looks good to me.

chx’s picture

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

I like it too and so does the bot.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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