[Uploadable] Remove deprecation warning when calling is_file with a null value

[JoolsMcFly]

Hi!

We've been getting lots of deprecation warning (Deprecated: is_file(): Passing null to parameter #1 ($filename) of type string is deprecated) coming from src/Uploadable/UploadableListener::removeFile:

public function removeFile($filePath)
{
    if (is_file($filePath)) {
        return @unlink($filePath);
    }

     return false;
}

This PR simply adds a check for null:

if (!is_null($filePath) && is_file($filePath)) {

Thanks for reviewing!

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.67%. Comparing base (7622db1) to head (ebae19a).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Uploadable/UploadableListener.php	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2901      +/-   ##
==========================================
+ Coverage   78.66%   78.67%   +0.01%     
==========================================
  Files         167      167              
  Lines        8746     8747       +1     
==========================================
+ Hits         6880     6882       +2     
+ Misses       1866     1865       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mbabker]

Do you have a stack trace showing how this method is being called with null? The doc block says that only strings should be passed in, so something else is already _doing_it_wrong() by calling this method with a null value (and if it's something in this package, that should be updated as well).

[JoolsMcFly]

Hi @mbabker .

Here's what we get in Sentry:

I guess we could add a check in postFlush as well?

[mbabker]

That's helpful as it leads me to two other minor issues:

• The protected getFilePathFieldValue() method isn't guaranteed to return a string if it's reading direct from a class property (so that return should be at least string|null)
• The protected addFileRemoval() method should probably make sure getFilePathFieldValue() returns a non-empty value before writing to the $this->pendingFileRemovals list

Making changes around those two methods in theory should ensure that the listener isn't calling removeFile() with a totally bad parameter.

[JoolsMcFly]

Hey, thanks for your comment. Do you want me to make changes you listed in my PR?

[mbabker]

Yes, that'd be the more correct fix to make here.

[JoolsMcFly]

Hey @mbabker .

Sorry for the delay!

I've implemented what you mentioned but I left the check I originally added as nothing really prevents removeFile from being called with a null parameter.

WDYT?