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 extension placeholder for serialized medias #127

Merged
merged 6 commits into from Feb 21, 2023

Conversation

Prokyonn
Copy link
Member

No description provided.

@Prokyonn
Copy link
Member Author

@alexander-schranz @wachterjohannes to prevent the BC break, we could maybe add a config like this:

sulu_headless:
    media:
        use_preferred_extension: true

WDYT?

@@ -94,15 +94,19 @@ public function serialize(MediaInterface $media, string $locale, ?SerializationC
$preferredExtension = $this->imageConverter->getSupportedOutputImageFormats($formatMediaApi->getMimeType())[0] ?? null;
if ($preferredExtension) {
$fileName = \pathinfo($fileName)['filename'] . '.' . $preferredExtension;
$mediaData['preferredExtension'] = $preferredExtension;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$mediaData['preferredExtension'] = $preferredExtension;
$mediaData['formatPreferredExtension'] = $preferredExtension;

So they are beside each other in the json response.

@@ -94,15 +94,19 @@ public function serialize(MediaInterface $media, string $locale, ?SerializationC
$preferredExtension = $this->imageConverter->getSupportedOutputImageFormats($formatMediaApi->getMimeType())[0] ?? null;
if ($preferredExtension) {
Copy link
Member

Choose a reason for hiding this comment

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

I think formatUri, formatPreferredExtension should be null if there is no preferredExtension as I think that indicates that there are no formats available for this media. E.g. .doc, .xls, ...

Content/Serializer/MediaSerializer.php Outdated Show resolved Hide resolved
@@ -80,6 +80,7 @@
"version": 1,
"subVersion": 0,
"name": "test-image.png",
"preferredExtension": "png",
Copy link
Member

Choose a reason for hiding this comment

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

would replace @string@ in formatUri to check the Uri is as expected.

Comment on lines 117 to 118
$this->formatCache->getMediaUrl(1, 'media-1.extension', '{format}', 1, 0)
->willReturn('/media/1/{format}/media-1.extension?v=1-0')
Copy link
Member

Choose a reason for hiding this comment

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

Can we create a real instance of LocalFormatCache so we avoid this mock.

@alexander-schranz
Copy link
Member

alexander-schranz commented Feb 20, 2023

@Prokyonn if somebody is using ^0.9 they will not get automatically 0.10, as ^ in pre stable versions is handled like 0.9.* so active update is required and so I'm fine with this bc break and no additional flag.

UPGRADE.md Outdated
Comment on lines 20 to 21
"formatUri": "/media/1/{format}/media-1.{extension}?v=1-0",
"preferredExtension": "png"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"formatUri": "/media/1/{format}/media-1.{extension}?v=1-0",
"preferredExtension": "png"
"formatUri": "/media/1/{format}/media-1.{extension}?v=1-0",
"formatPreferredExtension": "png"

@wachterjohannes
Copy link
Member

@alexander-schranz @wachterjohannes to prevent the BC break, we could maybe add a config like this:

sulu_headless:
    media:
        use_preferred_extension: true

WDYT?

@Prokyonn would also go with a flag and throw a deprecation message when it is not set! else the PR LGTM.

@Prokyonn
Copy link
Member Author

as discussed internally, we'll omit the configuration BC layer, because of

if somebody is using ^0.9 they will not get automatically 0.10, as ^ in pre stable versions is handled like 0.9.* so active update is required and so I'm fine with this bc break and no additional flag.

@alexander-schranz alexander-schranz added the Feature New functionality not yet included in the Bundle label Feb 21, 2023
@alexander-schranz alexander-schranz enabled auto-merge (squash) February 21, 2023 13:46
@alexander-schranz alexander-schranz merged commit 9ec2faa into sulu:0.x Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality not yet included in the Bundle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants