glob: allow empty string for pattern

[Hanmac]

Fixed problem with #9752

glob should be able to allow empty string for pattern

[orklah]

Thanks!

[kkmuffme]

@orklah that merge was too early, bc this will hide tons of unwanted behavior (that's why I PRed the fix initially) Can you please "unmerge" (revert) it?

@Hanmac The solution is to create a return type provider for sprintf to return non-empty-string/non-falsy-string if the pattern contains anything but placeholders (or if only placeholders but they are non-empty, then non-empty too)
#9752 (comment)

[Hanmac]

@kkmuffme that doesn't work
You can't predict if sprintf would return an empty or non empty string with the tools psalm provides (as far as I saw that parts with glob)

It's either your MR was merged too early

[kkmuffme]

You can't predict if sprintf would return an empty or non empty string with the tools psalm provides

Why? Just create a return type provider for sprintf, e.g. it already works when using %d placeholders https://psalm.dev/r/34a1d49484
@Hanmac

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/34a1d49484

<?php


function takesAnInt( int $arg ) {
 	$r = sprintf( '%d', $arg );
    /** @psalm-trace $r */;
    
    return $r;
}

Psalm output (using commit b99857c):

INFO: Trace - 6:27 - $r: non-empty-string

INFO: MissingReturnType - 4:10 - Method takesAnInt does not have a return type, expecting non-empty-string

[VincentLanglet]

@orklah that merge was too early, bc this will hide tons of unwanted behavior (that's why I PRed the fix initially) Can you please "unmerge" (revert) it?

It's your PR which was merged to early.

Your unwanted behavior are perfectly valid since the glob method does accept an empty string.
I have use case with empty string, which are not bugs. If you consider empty-string is an unwanted value, you can create your own glob method to restrict the param, or a more specific stub in your project.

@Hanmac The solution is to create a return type provider for sprintf to return non-empty-string/non-falsy-string if the pattern contains anything but placeholders (or if only placeholders but they are non-empty, then non-empty too) #9752 (comment)

The sprintf example was only one example.

I can just write a method which does

function searchFirst(string $userInput): ?string
{
     return glob($userInput)[0] ?? null;
}

And there is no valid argument to force myself writing

function searchFirst(string $userInput): ?string
{
     if ('' === $userInput) {
        return null;
     }

     return glob($userInput)[0] ?? null;
}

Because I could use the same to ask you changing every strlen call from

function computeLength(string $userInput): int
{
     return strlen($userInput);
}

to

function computeLength(string $userInput): int
{
     if ('' === $userInput) {
          return 0;
     }

     return strlen($userInput);
}

And there is tons of more complex example.

As soon as glob('') doesn't throw an exception, it's valid. There is nothing more to add.

And your PR, with all the useless non-empty-string annotation added,
just show how much noise you need to add for no reason
#9752

[kkmuffme]

function computeLength(string $userInput): int
{
     return strlen($userInput);
}

This example makes no sense, as you shouldn't create wrappers for native functions in the first place. This is a strawman argument.

---

And there is no valid argument to force myself writing

I agree with you 99.9%.
The 0.01% is for performance perhaps (you'd need to check php-src to check if it calls the file system on an empty pattern, as I am not that familiar with it), but that's ok.

The reason I created the PR originally is for other reasons though: I needed to have glob return a list<non-empty-string> instead of list<string> and I just took a shortcut. There should have been a return type provider added instead.

The current PR has a bug though, see #9820 - @VincentLanglet or @Hanmac please add a PR with that.

[Hanmac]

The current PR has a bug though, see #9820 - @VincentLanglet or @Hanmac please add a PR with that.

No. your PR broke it first, YOU try to fix it without causing glob(sprintf("%s", $val)) to fail

[kkmuffme]

No. your PR broke it first, YOU try to fix it without causing glob(sprintf("%s", $val)) to fail

This isn't how this works, otherwise we could NEVER add any change, since any PR would have to take care of all possible cases that exist somewhere.
The problem with this code is not with "glob" but with the missing return type provider for sprintf.

However the example of @VincentLanglet is a valid use case though, which is why I agree that this PR makes sense.
However, we should then extend this PR further so we get the correct type for an empty string too, shouldn't we?

[VincentLanglet]

The reason I created the PR originally is for other reasons though: I needed to have glob return a list<non-empty-string> instead of list<string> and I just took a shortcut. There should have been a return type provider added instead.

And the shortcut has impact on others code base, so let's avoid it.

Since glob('') returns an empty array, this is not incompatible with list<non-empty-string>
So you don't have to force the param to be non-empty-string in order to add list<non-empty-string> as return type.

The current PR has a bug though, see #9820 - @VincentLanglet or @Hanmac please add a PR with that.

It's not a bug.

[kkmuffme]

It's not a bug.

Yes, it is: https://psalm.dev/r/4e94df6823
This array can NEVER have non-empty-string values, but the trace has them.
Now wherever you pass that to you suddenly have to typeguard for non-empty-strings even though there cannot be any in there ever in this case.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/4e94df6823

<?php

$result = glob( '' );
if ( $result !== false ) {
	$result[] = rand( 0, 5 ); 
    /** @psalm-trace $result */;
}

Psalm output (using commit b99857c):

INFO: Trace - 6:32 - $result: non-empty-list<int<0, 5>|non-empty-string>

[kkmuffme]

Just to sum it up: this PR is fine for the majority of cases, however we should add a custom return type provider for glob in the case of an empty string param - #9820