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.

Sign up for GitHub

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

Jump to bottom

!!! FEATURE: Add getVariant(string, string) to VariantSupportInterface #2701

Merged
merged 1 commit into from
Sep 11, 2019
Merged

!!! FEATURE: Add getVariant(string, string) to VariantSupportInterface #2701

merged 1 commit into from
Sep 11, 2019

Conversation

kdambekalns
Copy link
Member

This adds a new method to the VariantSupportInterface, allowing
to fetch a preset-generated variant of an image directly.

public function getVariant(string $presetIdentifier, string $presetVariantName): ?ImageVariant;

@kdambekalns
FEATURE: Add getVariant(string, string) to VariantSupportInterface
6d6edde 
This adds a new method to the `VariantSupportInterface`, allowing
to fetch a preset-generated variant of an image directly.

    public function getVariant(string $presetIdentifier, string $presetVariantName): ?ImageVariant;
@kdambekalns kdambekalns self-assigned this Sep 11, 2019
kitsunet
kitsunet approved these changes Sep 11, 2019
View reviewed changes
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Something I said we would need from the beginning YAY
All for it.

@bwaidelich
Copy link
Member

Looks good, but it should be marked breaking (!!!) as it changes an @api interface, right?

@kdambekalns kdambekalns changed the title FEATURE: Add getVariant(string, string) to VariantSupportInterface !!! FEATURE: Add getVariant(string, string) to VariantSupportInterface Sep 11, 2019
daniellienert
daniellienert approved these changes Sep 11, 2019
View reviewed changes
Copy link
Member

@daniellienert daniellienert left a comment

Choose a reason for hiding this comment

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

Looks good. I assume it works as expected ;)

mficzel
mficzel approved these changes Sep 11, 2019
View reviewed changes
Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

Fine by reading, the api method makes sense and i already know a usecase in kaleidoscope for that.

Even if the implementation would have an issue we always do a bugfix release.

@kdambekalns
Copy link
Member Author

i already know a usecase in kaleidoscope for that.

Hehe… me too: sitegeist/Sitegeist.Kaleidoscope#14

@kdambekalns kdambekalns merged commit a3b3f85 into neos:5.0 Sep 11, 2019
@kdambekalns kdambekalns deleted the feature/asset-getvariant branch September 11, 2019 15:44
@kdambekalns
Copy link
Member Author

By now I realized this is not really future-proof, as it returns ?ImageVariant - but eventually there will be other asset types supporting variants… ideas on how to solve this are welcome.

@kdambekalns kdambekalns mentioned this pull request Sep 27, 2019
Merged
@bwaidelich
Copy link
Member

eventually there will be other asset types supporting variants

Good point.. and we all missed it while reviewing it 🙈
It should have been getVariant(...): AssetVariantInterface in the interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kitsunet kitsunet kitsunet approved these changes

@daniellienert daniellienert daniellienert approved these changes

@mficzel mficzel mficzel approved these changes

@robertlemke robertlemke Awaiting requested review from robertlemke

Assignees

@kdambekalns kdambekalns

Labels
!!! Next major P: Media T: PHP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kdambekalns @bwaidelich @kitsunet @daniellienert @mficzel