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

FEATURE: Batch rendering of asset variants #2751

Merged
merged 12 commits into from Dec 3, 2019

Conversation

@kdambekalns
Copy link
Member

kdambekalns commented Oct 24, 2019

This provides the following functionality:

  • (re-)render variants based on presets
  • re-render variants when replacing an asset resource

Configuration

The feature comes with no configurable settings itself. But you will need to configure asset
variant presets. Here is an example for a square preset:

Neos:
  Media:
    variantPresets:
      'AcmeCom:Square':
        label: 'AcmeCom Square Preset'
        mediaTypePatterns: ['~image/.*~']
        variants:
          'square':
            label: 'Square'
            adjustments:
              'crop':
                type: 'Neos\Media\Domain\Model\Adjustment\CropImageAdjustment'
                options:
                  aspectRatio: '1:1'

See the variant presets documentation at https://neos-media.readthedocs.io/en/stable/VariantPresets.html for details.

Usage

Simply run the command: ./flow media:rendervariants to generate all missing variants for
configured presets. The option --recreate will also render existing variants again based
on their presets. Using --limit you can only generate an sepcified amount of variants.

Resolves: #2798

kdambekalns added 3 commits Oct 2, 2019
This adds a createVariant() method in addition to the existing
createVariants() method.

Furthermore it prepares the AssetVariantGenerator into the direction
of variants other than ImageVariant by making the code more generic
in naming and types used.
So far ImageVariant was filtered explicitly in various places in
AssetRepository. This makes filtering work against any implementation
of AssetVariantInterface instead.
This adds a media:rendervariants command that will render variants
for all assets that are configured but do not exist.
@kdambekalns kdambekalns self-assigned this Oct 24, 2019
@kdambekalns kdambekalns added this to In progress in Neos 5.1 & Flow 6.1 Release Board via automation Oct 24, 2019
@kdambekalns kdambekalns force-pushed the kdambekalns:feature/render-asset-variants branch from 21c227e to 9ee01e8 Oct 24, 2019
@kdambekalns kdambekalns changed the title Feature/render asset variants FEATURE: Batch rendering of asset variants Oct 24, 2019
@daniellienert

This comment has been minimized.

Copy link
Member

daniellienert commented Oct 27, 2019

@kdambekalns: Still a draft or ready for review?

@kdambekalns kdambekalns marked this pull request as ready for review Oct 28, 2019
@kdambekalns

This comment has been minimized.

Copy link
Member Author

kdambekalns commented Oct 28, 2019

Allowed me to render ~1400 variants in ~45 minutes using ImageMagick on my Mac in Docker without memory issues.

Copy link
Member

daniellienert left a comment

Code looks good, didn't test it.
Lots of changed method signatures, hope we don't break too much stuff with it.

Neos.Media/Classes/Command/MediaCommandController.php Outdated Show resolved Hide resolved
Neos 5.1 & Flow 6.1 Release Board automation moved this from In progress to Reviewer approved Oct 29, 2019
Co-Authored-By: Daniel Lienert <daniel@lienert.cc>
@kdambekalns

This comment has been minimized.

Copy link
Member Author

kdambekalns commented Oct 29, 2019

Lots of changed method signatures

Well, it is only return types (if I didn't miss something now)… and I doubt people override those repository methods. Anything else should be out of "user scope", IMHO. But yes, fingers crossed, of course… 😬

kdambekalns added 5 commits Nov 4, 2019
# Conflicts:
#	Neos.Media/Classes/Command/MediaCommandController.php
#	Neos.Media/Classes/Domain/Repository/AssetRepository.php
This adds strict typing and return type declarations in addition to
further code style tweaks.
Copy link
Member

bwaidelich left a comment

I'm not a big fan of big controllers even if they are command controllers, but well.. it's not the first one and definitely not a reason to block this.
I left some (partly nitpicky) comments but in general I love the style and the little improvements you sneaked in :)

But I have to admit that I failed to actually test this since I can't find any documentation about asset presets out there o.O
Might be related to the late friday evening – I'll try again tomorrow. But a short description in the PR would make it easier to review this (and it helps when creating release notes *g)

foreach ($classNames as $className) {
/** @var AssetRepository $repository */
$repositoryClassName = $this->reflectionService->getClassSchema($className)->getRepositoryClassName();
$repository = $this->objectManager->get($repositoryClassName);

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Nov 29, 2019

Member

Instead of the inline @var I'd go for a real check

Suggested change
$repository = $this->objectManager->get($repositoryClassName);
$repository = $this->objectManager->get($repositoryClassName);
if (!$repository instanceof AssetRepository) {
throw new \RuntimeException(sprintf('Expected instance of %s, got %s', AssetRepository::class, get_class($repository)), 1575043393);
}

IDEs will pick up on that and we don't risk edge cases

This comment has been minimized.

Copy link
@kdambekalns

kdambekalns Dec 2, 2019

Author Member

Well, the only requirement is "a repository for the class implementing VariantSupportInterface", so… will that always be an AssetRepository?

This comment has been minimized.

Copy link
@kdambekalns

kdambekalns Dec 2, 2019

Author Member

If we had some interface for repositories of assets implementing VariantSupportInterface

Neos.Media/Classes/Command/MediaCommandController.php Outdated Show resolved Hide resolved
* @param AssetInterface $asset
* @param string $presetIdentifier
* @param string $variantIdentifier
* @return AssetVariantInterface The created variant (if any)

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Nov 29, 2019

Member

I'm stumbling upon the nullable return type here (and for recreateVariant()):
Isn't null always something unexpected that the calling side might want to know about?
Since this method can throw an AssetVariantGeneratorException anyways I would much prefer a non-nullable return type with a corresponding exception if the variant couldn't be generated, since otherwise there are chances of null pointer errors which can be really hard to debug

This comment has been minimized.

Copy link
@kdambekalns

kdambekalns Dec 2, 2019

Author Member

Good point. I added this to handle non-Image somewhat gracefully, but maybe throwing an exception is better. Then again, most of the variant-handling code is "soft" currently, e.g. when asking for a non.-existing variant you get the original back (IIRC).

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Dec 2, 2019

Member

To be honest, I think that's gracefulness applied in a counter-productive way.
If one wants createVariant() to ignore any error, a try/catch block can be used. But with the nullable return type you have to check for null. And even if you do, you might not know why it didn't work (asset was not an image, variant preset was not a VariantPreset (why not?), variantConfiguration was not an instance of Variant).
It might make more sense to make the calling side more graceful so that it only invokes this method if the asset is an Image etc.

when asking for a non.-existing variant you get the original back

Uh, is that so? I would not expect that. But I get what you say, and maybe it is less confusing to go for one style

This comment has been minimized.

Copy link
@kdambekalns

kdambekalns Dec 3, 2019

Author Member

Well, what could lead to no variant being returned? The obvious case is an error, then an exception makes sense.

But it might well be, that there is no preset (ok, can be seen as an error) or that the preset media type does not match the asset (is that an error?)

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 30, 2019

btw: Sorry if my first "review" seemed negative, it wasn't meant to. I'm just a bit puzzled that such a great feature isn't really exposed to the "outside" due to missing docs. But that's not related to this change of course.
I'll try to get it to run now (sorry, didn't manage due to bad connection in the train – hopefully I find some time in the next days!)

@bwaidelich bwaidelich moved this from Reviewer approved to Reviews needed in Neos 5.1 & Flow 6.1 Release Board Nov 30, 2019
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 30, 2019

I added an issue for this in order to simplify release management and to collect higher-level details for the release notes: #2798

@bwaidelich bwaidelich removed this from Reviews needed in Neos 5.1 & Flow 6.1 Release Board Nov 30, 2019
Co-Authored-By: Bastian Waidelich <b.waidelich@wwwision.de>
*/
public function createVariant(AssetInterface $asset, string $presetIdentifier, string $variantIdentifier): ?AssetVariantInterface
{
// Currently only Image Variants are supported. Other asset classes can be supported, as soon as there is a common

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Dec 2, 2019

Member

Wouldn't it make sense to change the signature to createVariant(Image $image, ...) and change it to the interface as soon as that exists? (And since Image will implement it, existing code should not break)

This comment has been minimized.

Copy link
@kdambekalns

kdambekalns Dec 3, 2019

Author Member

Well, the concept is about asset variants, so… and again, here I followed the style this was implemented already. 🤷‍♂️

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Dec 2, 2019

@kdambekalns Thanks a lot for the update, the example and the link to the neos media documentation (which I wasn't aware of.. o.O)

When following the example I ran into 2 errors, but they are not directly related I think:

Notice: Undefined index: label in VariantPreset.php line 51

Apparently Variant Presets need a label – I adjusted your config snippet accordingly

The node type "Neos.NodeTypes:AssetList" is not available and no fallback NodeType is configured.

I had a node of that type in my demo site installation, maybe due to some old db state. After removing that, it works as expected.
Very neat!

Copy link
Member

markusguenther left a comment

Test it with the small site of mine. Sadly has not that much images but it worked fine :)
Thanks @kdambekalns for bringen that up 👍

@markusguenther

This comment has been minimized.

Copy link
Member

markusguenther commented Dec 3, 2019

Bildschirmfoto 2019-12-03 um 09 20 19
As mentioned not much images ;)

@kdambekalns

This comment has been minimized.

Copy link
Member Author

kdambekalns commented Dec 3, 2019

I see there is still room for improvement, but some of that must only be done in a major release, and other things need some more thinking. Overall IMHO none of the issues are blockers. 😎

Copy link
Member

kitsunet left a comment

While I think it has room for improvement and refactoring, this is a neat feature and seems to work fine for now.

@kitsunet

This comment has been minimized.

Copy link
Member

kitsunet commented Dec 3, 2019

Would merge after travis is done...

@davidspiola davidspiola merged commit f7002f6 into neos:master Dec 3, 2019
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kitsunet kitsunet deleted the kdambekalns:feature/render-asset-variants branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.