Disable ignoreInternalFunction(False|Null)Return by default

[robchett]

as per #9764

Breaking change for Psalm 6?

[orklah]

Cool!

Seems like it makes a lot of tests fail

Could you look if those are legit failures (because Psalm didn't check possible false/null values where they could happen?). If so, I'd suggest re-enabling the config for Psalm instead of trying to fix every case

[robchett]

@orklah
I've added a new config for specifying which functions to ignore null/false on, which allowed me to find out what functions were not being handled correctly.

<ignoredInternalFunctionFalseReturn>
        <referencedFunction name="getcwd"/>
        <referencedFunction name="ob_get_clean"/>
        <referencedFunction name="glob"/>
        <referencedFunction name="filemtime"/>
        <referencedFunction name="opendir"/>
        <referencedFunction name="scandir"/>
        <referencedFunction name="readdir"/>
        <referencedFunction name="tempnam"/>
        <referencedFunction name="phpversion"/>
        <referencedFunction name="file_get_contents"/>
        <referencedFunction name="reset"/>
        <referencedFunction name="end"/>
        <referencedFunction name="fwrite"/>
        <referencedFunction name="fread"/>
        <referencedFunction name="realpath"/>
        <referencedFunction name="readlink"/>
        <referencedFunction name="json_encode"/>
        <referencedFunction name="curl_init"/>
        <referencedFunction name="base64_decode"/>
        <referencedFunction name="igbinary_serialize"/>
        <referencedFunction name="strtotime"/>
        <referencedFunction name="preg_split"/>
        <referencedFunction name="ini_get"/>
        <referencedFunction name="getopt"/>
    </ignoredInternalFunctionFalseReturn>

    <ignoredInternalFunctionNullReturn>
        <referencedFunction name="preg_replace"/>
        <referencedFunction name="array_shift"/>
        <referencedFunction name="key"/>
    </ignoredInternalFunctionNullReturn>

Most seem safe, but there are a few file/io ones in there that I think I should remove the suppression for and actually add checks too.

Are you happy with this approach for a fine-tuned list of suppressed functions instead of a blanket suppression on all internal methods?

[orklah]

I'm not sure I understood what you meant

I was kinda expecting that the errors Psalm raised on itself were going to be places where a function can legitimately return false/null but we ignored it (probably because it can rarely happen but still). So the fix for that would have been either to fix each place in the code (pretty tedious) or just baseline those

What I understand is that you added a config so we can restore the old ignore for specific functions. I don't think this config will be used by a lot of users. They'll either don't want to bother with false/null to begin with and ignore everything or they'll want to have the cleanest code and fix what they can and baseline the rest in the meantime.

This seems like the kind of feature for power users that I would prefer to be solved with custom stubs if possible (to avoid cluttering the core too much)

[robchett]

Sure, that's fine.

I'll review/fix the ones that look the most likely to return false and baseline the rest.

glob filemtime opendir scandir readdir tempnam file_get_contents reset end fwrite fread realpath readlink

[jack-worman]

Casting false and null types to string is not the correct solution. This is just masking the issue. If an internal function returns null or false, it probably should throw an exception instead, since most of the time these are errors.

At my work, we use https://github.com/thecodingmachine/safe to handle these issues.

[orklah]

Casting false and null types to string is not the correct solution

Indeed but every improvement to the codebase is welcome, even when it's incremental. Before this PR, they were completely ignored and now they're explicitly handled (through casting). If this itch someone the wrong way, a future PR could improve the situation :)

[orklah]

Thanks!