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

Properly handle Cloudinary public IDs and extensions for media and raw resources #125

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

newtonjob
Copy link
Contributor

Historically, Cloudinary is known to not play nice with extensions in public ids for media resources. This is so that we can easily transform the resources to various other types for delivering with possible transformations.

When resources are being uploaded, the package currenty removes the extension from the public ID passed to Cloudinary.
This is great, as Cloudinary recommends to not include file extensions in public IDs for media resources, however, they SHOULD be included for raw resources.

Screenshot 2024-08-14 at 03 12 28

https://cloudinary.com/documentation/image_upload_api_reference

This PR solves two problems:

  • Removes extensions from media resources public_id but not raw resources.
  • Allows other methods on the Storage facade like exists, delete, rename, etc to still work when the public_id is supplied with its extension.
// Assuming the default storage disk is `cloudinary`.

Storage::delete('avatar-1.jpg'); // Works

Storage::delete('avatar-1'); // Still works

// This makes it possible to do this;
$path = $request->file('avatar')->store();

Storage::delete($path);
// Which wasn't possible before this PR.

This way, people can switch storage disks and not make any changes to their code.

@colbyfayock
Copy link
Collaborator

hey @newtonjob, im not familiar enough with Laravel to confidently review this, but i did want to make sure this is taking into consideration that it's possible for someone to have set a public ID with a file extension, though it's not particularly recommended and sometimes may happen accidentally

so if for instance, you strip the file extension of a public ID like myid.jpg where .jpg is part of the ID, it would break

i suspect though that in this case you may be stripping it through the upload lifecycle which may be a bit different and probably more appropriate, but thought id mention this to avoid any potential issues

@newtonjob
Copy link
Contributor Author

newtonjob commented Aug 14, 2024

Thanks @colbyfayock. You've got a valid point that it's possible for someone to have accidentally set a public ID with a file extension. But that's not currently possible if they uploaded the resource via the adapter in this package, since it currently does strip the extension before upload. See here:

$fileExtension = pathinfo($publicId, PATHINFO_EXTENSION);
$newPublicId = $fileExtension ? substr($publicId, 0, - (strlen($fileExtension) + 1)) : $publicId;

This PR is just making sure that the adapter is consistently stripping the extensions when interacting with other methods like Storage::exists(), Storage::rename(), Storage::delete(), etc.

@colbyfayock
Copy link
Collaborator

perfect thanks for confirming @newtonjob 🙏

@unicodeveloper
Copy link
Collaborator

Just here to say that I really like this PR. Nice work @newtonjob

@joshmanders
Copy link
Collaborator

Thank you, @newtonjob!

@joshmanders joshmanders merged commit 6f0713d into cloudinary-community:master Aug 14, 2024
5 checks passed
@newtonjob newtonjob deleted the extensions branch August 14, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discrepancy file extention between upload and get the file
4 participants