Skip to content

Create str_icontains PHP function#18705

Closed
adamcable wants to merge 2 commits intophp:masterfrom
adamcable:master
Closed

Create str_icontains PHP function#18705
adamcable wants to merge 2 commits intophp:masterfrom
adamcable:master

Conversation

@adamcable
Copy link
Copy Markdown

As a PHP developer of 20 years I'm somewhat accustomed to adding "i" into the function name when I'm after a case-insensitive version. Therefore, I found it a bit odd I couldn't do it with the new "str_contains" function, so have added a case-insensitive version. Tests added too.

Assume this would need a RFC to be accepted.

As a PHP developer of 20 years I'm somewhat accustomed to adding "i" into the function name when I'm after a case-insensitive version. Therefore, I found it a bit odd I couldn't do it with the new "str_contains" function, so have added a case-insensitive version.
Tests added too.
@ndossche
Copy link
Copy Markdown
Member

Hi, welcome. On first glance code seems good.
This indeed requires an RFC. The detailed process for RFCs is described over at https://wiki.php.net/rfc/howto 🙂

@ndossche
Copy link
Copy Markdown
Member

Oh BTW, the CI is failing because you didn't regenerate the arginfo files (or it was, but you didn't add it to your commit).
Normally make regenerates this file from the stub.php file. If it didn't, run ./build/gen_stub.php

@adamcable
Copy link
Copy Markdown
Author

Oh BTW, the CI is failing because you didn't regenerate the arginfo files (or it was, but you didn't add it to your commit). Normally make regenerates this file from the stub.php file. If it didn't, run ./build/gen_stub.php

Thank you - really appreciate that 👍

@TimWolla
Copy link
Copy Markdown
Member

This indeed requires an RFC.

It appears simple and obvious enough to be okay without RFC (not much discussion to be had here), but I'm okay with this going through the RFC process.


Should we also add str_istarts_with and str_iends_with? Naming is not particularly pretty with the second underscore, but that seems most consistent.

Copy link
Copy Markdown
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation + tests LGTM.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented May 29, 2025

This indeed requires an RFC.

It appears simple and obvious enough to be okay without RFC (not much discussion to be had here), but I'm okay with this going through the RFC process.

Considering there is going to be bikeshed, and I don't remember why the last time this was not included, I do think an RFC is probably necessary.

Should we also add str_istarts_with and str_iends_with? Naming is not particularly pretty with the second underscore, but that seems most consistent.

Bikeshed: surely stri_contains and stri_starts_with makes more sense...

@ndossche
Copy link
Copy Markdown
Member

ndossche commented May 29, 2025

It appears simple and obvious enough to be okay without RFC (not much discussion to be had here), but I'm okay with this going through the RFC process.

and I don't remember why the last time this was not included

Already some history posted on the ML, and they're questioning the feature: https://externals.io/message/127504#127505

@adamcable
Copy link
Copy Markdown
Author

Thanks all :)
Definitely not precious on where the i goes - will see what other comments we have on the ML.

@alecpl
Copy link
Copy Markdown

alecpl commented May 31, 2025

Because str_ireplace exists, a consistent name would be str_icontains.

@adamcable
Copy link
Copy Markdown
Author

Thanks all.

I'll create a RFC for this, and if successful look to do the same with str_​starts_​with and str_ends_with :)

@vrubleg
Copy link
Copy Markdown

vrubleg commented Jul 14, 2025

Maybe str_ireplace should be deprecated and replaced by stri_replace, that would open a door for all other stri_ functions.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Apr 6, 2026

The RFC was declined, so closing this.

@Girgias Girgias closed this Apr 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants