Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip double / prefix when uploading to Media root directory #491

Merged
merged 4 commits into from
Mar 14, 2022

Conversation

Flynsarmy
Copy link
Contributor

@Flynsarmy Flynsarmy commented Mar 12, 2022

When uploading to the Media Manager's root directory, the media.file.upload event will prefix the $path variable with two / characters. This PR strips the first one.

Steps to Replicate:

  • In any plugin add the following to your Plugin.php's boot method:
Event::listen('media.file.upload', function (\Backend\Widgets\MediaManager $mediaWidget, string $path, \Symfony\Component\HttpFoundation\File\UploadedFile $uploadedFile) {
    \Log::info($path);
});
  • Go to Backend - Media section and upload an image to the root directory.
  • Open /storage/logs/system.log and see the filepath logged was //myimage.jpg

The problem in depth

When uploading to the root directory, the following line produces a path of / because you're uploading to the Media Manager's root directory:

$path = !empty($this->uploadPath) ? $this->uploadPath : Request::input('path');

On the very next line we call validatePath:
$path = MediaLibrary::validatePath($path);

which strips the / suffix but then adds it back as a prefix:
$path = '/'.trim($path, '/');

and finally on the next line, we add yet another / resulting in the behaviour we see in my issue:
$filePath = $path . '/' . $fileName;

This should result in uploads to the root directory of the Media Manager having a $path of /myimage.jpg as it should.

@LukeTowers LukeTowers added maintenance PRs that fix bugs, are translation changes or make only minor changes needs review Issues/PRs that require a review from a maintainer labels Mar 12, 2022
@LukeTowers
Copy link
Member

Waiting for the other changes we discussed @Flynsarmy :)

@Flynsarmy
Copy link
Contributor Author

Flynsarmy commented Mar 13, 2022

I didn't include them in this PR because they're not part of the bug outlined. Would you prefer I tack them on here or put them in a separate PR that'd make more sense for them?

@LukeTowers
Copy link
Member

I'd prefer they were included in the PR so we can make the single PR be about making the file.upload event actually useful rather than smaller pieces of the overall puzzle.

@LukeTowers LukeTowers changed the base branch from develop to wip/1.2 March 14, 2022 01:15
@LukeTowers LukeTowers merged commit 765f55e into wintercms:wip/1.2 Mar 14, 2022
@LukeTowers LukeTowers added Status: Completed and removed needs review Issues/PRs that require a review from a maintainer labels Mar 14, 2022
@LukeTowers LukeTowers added this to the v1.2.0 milestone Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PRs that fix bugs, are translation changes or make only minor changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants