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

[3.x]: Unable to eager load multiple transforms with srcsets #11209

Closed
Wiejeben opened this issue May 11, 2022 · 4 comments
Closed

[3.x]: Unable to eager load multiple transforms with srcsets #11209

Wiejeben opened this issue May 11, 2022 · 4 comments

Comments

@Wiejeben
Copy link
Contributor

Wiejeben commented May 11, 2022

What happened?

Description

Update: I made a pull request #11210.

Essentially what I'm trying to do is to eager load 3 different transforms with a 2x srcset:

  • cardIndexLarge with 2x
  • cardIndexMedium with 2x
  • cardIndexSmall with 2x

In order to eager load the base transforms I wrote the following:

{% set query = craft.entries.section('blogArticles').orderBy('postDate').limit(8).with([
    ['coverImage', {withTransforms: ['cardIndexLarge', 'cardIndexMedium', 'cardIndexSmall']}],
]) %}

As far as I'm aware, the current transform eager loading implementation only supports a single transform when combined with eager loading srcsets. This is to us the last step to achieving full eager loading!

When I add 2x as the last item in the transforms array it will only eager load cardIndexSmall with 2x. However if I add the srcset after cardIndexLarge it will actually overwrite cardIndexMedium and cardIndexSmall to the cardIndexLarge 2x variant.

The overwrites happen because the $sizeValue, $sizeUnit at 461 are being remembered from the previous loop:
https://github.com/craftcms/cms/blob/a7b735321468a6cf7d73a0eb582aa610eb5fc390/src/services/AssetTransforms.php#L

In order to fix this I added unset($sizeValue, $sizeUnit); to the end of the foreach loop, this would allow me to apply srcset eager loading to multiple transforms like this:

{% set query = craft.entries.section('blogArticles').orderBy('postDate').limit(8).with([
    ['primaryImage', {withTransforms: ['cardIndexLarge', '2x', 'cardIndexMedium', '2x', 'cardIndexSmall', '2x']}],
]) %}

Apart from the format it would eager load all srcsets correctly (that might be a different issue?):
Screenshot 2022-05-11 at 20 58 17

Unless the format is applied from the base transform during eager loading the srcset transforms will still be requested in every loop:
Screenshot 2022-05-11 at 20 58 55

I have validated this fix will continue to work even when adding multiple srcsets like this:

{% set query = craft.entries.section('blogArticles').orderBy('postDate').limit(8).with([
    ['primaryImage', {withTransforms: ['cardIndexLarge', '2x', '3x', 'cardIndexMedium', '2x', 'cardIndexSmall', '2x']}],
]) %}

Craft CMS version

3.4.41

PHP version

8.0.17

Operating system and version

Linux 5.10.104-linuxkit

Database type and version

MySQL 8.0.28

Image driver and version

Imagick 3.7.0 (ImageMagick 7.1.0-16)

Installed plugins and versions

No response

@Wiejeben Wiejeben changed the title [3.x]: Unable to eager load multiple transforms with multiple srcsets [3.x]: Unable to eager load multiple transforms with srcsets May 11, 2022
Wiejeben added a commit to Wiejeben/cms that referenced this issue May 11, 2022
This pull request would fix srcset eager loading by maintaining the `format` provided by the the base transform. 

Also this pull request would add support for allowing srcset transforms on multiple transforms, see craftcms#11209.
@brandonkelly
Copy link
Member

This is already possible. You just need to add 2x after each of the normal transforms:

{% set query = craft.entries()
  .section('blogArticles')
  .orderBy('postDate')
  .limit(8)
  .with([
    ['coverImage', {
      withTransforms: ['cardIndexLarge', '2x', 'cardIndexMedium', '2x', 'cardIndexSmall', '2x']
    }],
  ]) %}

@Wiejeben
Copy link
Contributor Author

Wiejeben commented May 13, 2022

Unfortunately that doesn't seem to work:

Twig:

{% set query = craft.entries.section('blogArticles').orderBy('postDate').limit(8).with([
    ['primaryImage', {
        withTransforms: ['cardIndexLarge', '2x', 'cardIndexMedium', '2x', 'cardIndexSmall', '2x']
    }],
]) %}

After that I dd() the $indexCondition variable inside \craft\services\AssetTransforms::eagerLoadTransforms that is used to query for the indexes:
image

This happens because the AssetsHelper::parseSrcsetSize($transform); fails on cardIndexMedium inside a try-catch, however $sizeValue and $sizeUnit still exist from the previous iteration. Adding an unset() fixes this.

With this I also discovered that format isn't being applied from the base transform, causing the N+1 problem to exist still. When requesting srcset transforms this is however required:
Screenshot 2022-05-11 at 20 58 55

brandonkelly added a commit that referenced this issue May 17, 2022
brandonkelly added a commit that referenced this issue May 17, 2022
@brandonkelly
Copy link
Member

Sorry, there was a bug that prevented this from working as intended. Fixed now for the next v3 and v4 releases.

@brandonkelly
Copy link
Member

3.7.43 and 4.0.3 are out now with that fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants