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

Unable to specify asset type for assets lookup #129

Closed
AndikanGabriel opened this issue Sep 6, 2024 · 1 comment · Fixed by #143
Closed

Unable to specify asset type for assets lookup #129

AndikanGabriel opened this issue Sep 6, 2024 · 1 comment · Fixed by #143
Labels
bug Something isn't working

Comments

@AndikanGabriel
Copy link

@joshmanders the current implementation of the asset lookup function in the storage driver defaults to the image resource type and does not provide an option to specify other resource types, such as video. This limitation prevents one from retrieving details for video assets using the Storage facade.

Steps to reproduce

1. Upload a video or audio file
Storage::upload($file) returns void, I'll use the Cloudinary API directly:

$publicId = \Cloudinary::uploadFile($file)->getPublicId()

2. Check if uploaded file exists using the public id

Storage::disk('cloudinary')->fileExists($publicId)

Expected behaviour

The fileExists method should return true for files that have been successfully uploaded and exist on Cloudinary's server, regardless of whether the file is an image or video.

Actual behaviour

The fileExists method returns false for files that are confirmed to exist on the server. This is likely because the current implementation defaults to checking only image resources, ignoring other types such as video.

Debugging

Upon examining the Cloudinary PHP SDK, specifically the method responsible for setting the asset type, and the Cloudinary Laravel Adapter, it is clear that the package defaults to image and lacks an interface to specify other resource types.

$this->adminApi()->asset($this->preparePublicId($path));

Observe in L250, that there's no interface to specify the resource type. Additionally, I noticed that the media extension property is missing mp3 support: Reference.

My work around

To address this issue, I extended the Cloudinary storage driver and updated the media extension property. For each method relying on the assets api, I modified the code to first check for image resources and then fall back to video resources if the initial check fails. Here is the affected code snippet:

public function read(string $path): string
{
$resource = (array)$this->adminApi()->asset($this->preparePublicId($path));
return file_get_contents($resource['secure_url']);
}

Here is the updated code snippet:

    public function read(string $path): string
    {
        try {
            $resource = (array) $this->adminApi()->asset($this->preparePublicId($path));
        } catch (Exception) {
            $resource = (array) $this->adminApi()->asset(
                $this->preparePublicId($path), ['resource_type' => 'video']
            );
        }

        return file_get_contents($resource['secure_url']);
    }

This modification ensures that the asset API can handle video and other resource types gracefully.

Additionally, considering Cloudinary supports three resource types, the code could be further enhanced to handle image, video, and raw resources:

        foreach (['image', 'video', 'raw'] as $type) {
            try {
                $this->adminApi()->asset(
                    $this->preparePublicId($path), ['resource_type' => $type]
                );
                return true;
            } catch (Exception) {
            }
        }

        return false;

Request

  1. Please provide guidance on integrating the resource_type parameter into the existing storage driver methods.
  2. Is there a planned update to support multiple resource types and include missing media extensions like mp3?
  3. Is there a better approach to lookup asset details for a video resource type using the Storage facade?
  4. Does my modification align with the package's code standard? If so, would a PR be welcomed?

Thank you for your attention to this request. Improving support for various resource types will significantly enhance the flexibility and functionality of the package.

@joshmanders joshmanders added the bug Something isn't working label Sep 9, 2024
@joshmanders
Copy link
Collaborator

Awesome, thank you for this @AndikanGabriel. I will look into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants