Skip to content

[9.x] Fix Stringable typehints with Enumerable - #44030

Merged
taylorotwell merged 3 commits into
laravel:9.xfrom
erikgaal:patch-stringable-types
Sep 7, 2022
Merged

[9.x] Fix Stringable typehints with Enumerable#44030
taylorotwell merged 3 commits into
laravel:9.xfrom
erikgaal:patch-stringable-types

Conversation

@erikgaal

@erikgaal erikgaal commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Fixes a bug with static analysis using PHPStan that was introduced in #44012. I've taken the liberty of making the Str and Stringable helpers work with iterators where possible, and broadened the types as well.

Below is an example of the errors and how they are fixed now.
https://phpstan.org/r/a8f703b0-5d8c-470e-8b7f-2ce3adbcdf1a

* @param string|array<string>|Enumerable<array-key, string> $subject
* @return string
*/
public static function replace($search, $replace, $subject)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could also make this work with iterable, but then we need to call iterator_to_array, which might not be as efficient? Not sure if that's true though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Made it work with iterable now. Did some benchmarks and tried to keep the same performance by not over-naively passing everything to collect.

@nbayramberdiyev

Copy link
Copy Markdown
Contributor

This makes sense. PHPStan documentation says:

If Collection|Foo[] means “Collection or array” in your case even if Collection implements Traversable, you need to disambiguate the type by using Collection|array<Foo> instead.

@taylorotwell
taylorotwell merged commit 3665096 into laravel:9.x Sep 7, 2022
@erikgaal
erikgaal deleted the patch-stringable-types branch September 8, 2022 08:53
@vlakoff

vlakoff commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

Code style:

In contains() and endsWith() you used:

        if (! is_iterable($needles)) {
            $needles = (array) $needles;
        }

Whereas in startsWith() you used:

        if (! is_iterable($needles)) {
            $needles = [$needles];
        }

Additionally, in is() and isMatch() there are:

        if (! is_iterable($pattern)) {
            $pattern = [$pattern];
        }

Suggestions:

  • At least, contains(), startsWith() and endsWith should be harmonized.
  • To improve further, the above five methods should be harmonized.

@vlakoff

vlakoff commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

Note that, provided $needles equals null, (array) $needles gives [] whereas [$needles] gives [null]. The former doesn't enter into the foreach, the latter relies on (string) $needle !== ''.

In the case at hand, for converting non-iterable to array, I would definitely pick [$needles]:

  • If receiving $needles null, the most probable case if that the method dynamically receives one needle (which may be undefined, expecting the method to return false). Thus, consistently putting this value in a [<value>] array, even when it's null, makes more sense than creating an empty array.
  • More consistent with, provided $needles empty string '', (array) $needles and [$needles] both of which give [''].
  • In any case, we can't remove the string cast in (string) $needle !== '', as the method may be provided array containing null (e.g. ['foo', null, 'bar']).
  • Proper handling if provided a Stringable object (casting object to array would be wrong).
  • [$item] encapsulation is more descriptive, less surprising, has less pitfalls than these (array) $item casts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants