fix: encode percent signs in filenames when generating URL

[moemadeldin]

Fixes #3926

[freekmurze]

Thanks for the PR, and for catching the bug. I don't think we can merge this as-is though, because getPathRelativeToRoot() is used for much more than URL generation. Adding rawurlencode() there will break filesystem operations for any file whose name contains a %.

Callers of getPathRelativeToRoot() that are not URL-related:

• DefaultUrlGenerator::getPath() – returns an absolute filesystem path (getRootOfDisk() . getPathRelativeToRoot())
• Support\FileRemover\DefaultFileRemover and FileBaseFileRemover – delete files from disk
• MediaCollections\Filesystem::copyToMediaLibrary() – copies source files
• Media::getDiskSize() – calls Storage::disk(...)->size(->getPathRelativeToRoot(...))
• Media::toMailAttachment() – builds an email attachment from the disk path

With this change, a file saved as IMG_5405%20copy.jpg on disk would have its URL correct, but deleting it, getting its size, attaching it to a mail, or reading its absolute path would all fail because the disk would be asked for IMG_5405%2520copy.jpg.

The encoding should happen only when building a URL. Two things to change:

1. Encode in the URL-generating methods instead of in getPathRelativeToRoot(). The most targeted spots are DefaultUrlGenerator::getUrl() and DefaultUrlGenerator::getTemporaryUrl(). You could either URL-encode the path segment by segment there, or add a small helper like getUrlEncodedPathRelativeToRoot() on the base class that the URL generator uses. rawurlencode on the whole path is wrong because it would also encode the / separators; split on /, encode each segment, and join again.
2. The conversion branch has the same issue. A conversion URL for a media with % in the name (or a conversion file name that contains one) is currently unfixed. The encoding needs to apply there too.

Also small nits in the test file: missing newline at end of file, and a blank line before the new it(...) block to match the file style.

Happy to help once those are addressed.

[moemadeldin]

@freekmurze Thanks for the detailed feedback! I've addressed all the points:

• Moved the encoding out of getPathRelativeToRoot() into a getUrlEncodedPathRelativeToRoot() helper on the base class
• Applied the same fix to getTemporaryUrl()
• Added a test confirming filesystem path is not affected
• Fixed the blank line and end-of-file newline nits

[freekmurze]

Thanks for addressing the feedback — this looks good now. Encoding happens only in the URL-generating methods, per-segment, so filesystem operations are unaffected and conversion URLs are covered via the same helper.

[francoism90]

@moemadeldin @freekmurze This change causes some of images (thumbs - conversions) not to shown anymore over S3.

protected function getUrlEncodedPathRelativeToRoot(): string
    {
        $segments = explode('/', $this->getPathRelativeToRoot());

        return implode('/', array_map(rawurlencode(...), $segments));
    }

This seems weird to me. This breaks the path into multiple segments, but I don't know for sure if S3 has issues with this? Could it be file names are already encoded on creation, and now it cannot make a match anymore?

[moemadeldin]

@francoism90 Thanks for catching this! Tracked it down and opened #3933 with a proposed fix.