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

Add AssetFile endpoint for retrieving file streams #36

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

apotek
Copy link
Contributor

@apotek apotek commented May 31, 2024

Resolves #35

Description

This merge request adds a new Endpoint for retrieving file streams. Some file formats, such as watermarked assets, do not return a URL via the AssetLink Endpoint. In order to handle assets that have not been pre-generated and do not have a link, we need to use the AssetFile endpoint /webapi/mediafile/assetfile/getassetfile_42R_v1.

We also need to refactor the Response object to no longer assume all responses have JSON data. The Response object can contain a stream resource with binary data instead, so decoding the body will throw an exception.

This is a requirement/blocker for this orange_dam issue: https://www.drupal.org/project/orange_dam/issues/3451497

@apotek apotek self-assigned this May 31, 2024
@apotek apotek requested a review from agarzola May 31, 2024 07:41
Copy link
Contributor

@adamzimmermann adamzimmermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions, but looking good.

src/Endpoints/AssetFile.php Outdated Show resolved Hide resolved
src/Endpoints/AssetFile.php Outdated Show resolved Hide resolved
src/Factory.php Show resolved Hide resolved
src/Factory.php Show resolved Hide resolved
src/Http/Response.php Show resolved Hide resolved
Add some documentation to the new endpoint.
@apotek apotek requested review from adamzimmermann and removed request for agarzola and adamzimmermann June 3, 2024 20:42
Copy link
Contributor

@adamzimmermann adamzimmermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked this through with @apotek and I think I'm sold. ✅

@apotek
Copy link
Contributor Author

apotek commented Jun 4, 2024

@adamzimmermann It looks like your approval doesn't count. @agarzola would you be able to rubber stamp this knowing that Adam and I have discussed this extensively and he approved it.

@apotek apotek requested a review from agarzola June 4, 2024 17:05
Copy link
Member

@agarzola agarzola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this based on Adam's approval and changes made in response to his previous review questions.

@apotek apotek merged commit cf5dde0 into 1.x Jun 5, 2024
4 checks passed
@apotek apotek deleted the 35-assetfile-endpoint branch June 5, 2024 14:04
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.

Add getassetfile endpoint
3 participants