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]: Image dimension calculation is buggy when upscaleImages is set to false #11837

Closed
proimage opened this issue Aug 29, 2022 · 13 comments · Fixed by #11940
Closed

[3.x]: Image dimension calculation is buggy when upscaleImages is set to false #11837

proimage opened this issue Aug 29, 2022 · 13 comments · Fixed by #11940
Assignees

Comments

@proimage
Copy link

proimage commented Aug 29, 2022

What happened?

Description

Given an image Asset of 1080x594, which smaller than a defined transform size, the calculated dimension values are inconsistent and unexpected if the upscaleImages config setting is false.

Steps to reproduce

  1. In config/general.php, add 'upscaleImages' => false,
  2. Add the following code to a template, ensuring that the image selected is smaller than the specified 1280px width (I presume that's the trigger for the issue):
    {% set image = entry.assetField.one() %}
    {% do image.setTransform({ width: 1280, height: 720 }) %}
    {{ tag('img', {
        src: image.url,
        width: image.width,
        height: image.height,
    }) }}

Expected behavior

I'd expect the resulting image to have the same aspect ratio as 1280x720 (16:9), which it does, and I'd expect the width="..." and height="..." attributes to reflect the resulting image dimensions (line-breaks added):

<img
    src="/path/to/images/_1280x720_crop_center-center_none/my-awesome-image.jpg" 
    width="1056"
    height="594">

Actual behavior

The output image is 1056x594px (🎉), but the attributes are width="1080" height="334" (🤪) (line-breaks added):

<img
    src="/path/to/images/_1280x720_crop_center-center_none/my-awesome-image.jpg" 
    width="1080"
    height="334">

Related to

#5288

Craft CMS version

3.7.43

PHP version

7.4.30

Operating system and version

Linux 5.10.102.1-microsoft-standard-WSL2

Database type and version

MySQL 8.0.26

Image driver and version

Imagick 3.6.0 (ImageMagick 6.9.11-60)

Installed plugins and versions

  • Azure Blob Storage 1.0.1
  • Button Box 2.0.4
  • Colorit 1.1.2.1
  • Cookie Consent Banner 1.2.9
  • Cookies 1.1.15
  • Craft Color Mixer 1.0.9
  • Field Manager 2.2.4
  • Freeform 3.13.10
  • Icon Picker 1.1.12
  • Imager X v3.5.8
  • Inventory 2.1.1
  • Isolate 1.4.4
  • Linkit 1.1.12.1
  • LJ Range Slider 1.0.0
  • Maps 3.9.3
  • MatrixMate 1.4.3
  • Minify 1.2.11
  • Mix 1.5.2
  • Navigation 1.4.26
  • Neo 2.13.10
  • oEmbed 1.3.15
  • Postmark 2.1.0
  • Redactor 2.10.8
  • SEO 3.7.4
  • Super Dynamic Fields 1.0.7
  • Super Table 2.7.2
  • Table Maker 2.0.1
  • Tags 1.0.9
  • Twigpack 1.2.17
  • Wordsmith 3.3.1.1
brianjhanson added a commit that referenced this issue Aug 30, 2022
Fixes an issue where the height of a non-cropped image being transformed was being calculated as a percentage of the height rather than of the width.

#11837
brianjhanson added a commit that referenced this issue Aug 30, 2022
Fixes an issue where the height of a non-cropped image being transformed was being calculated as a percentage of the height rather than of the width.

#11837

Fixes an issue where the height of a non-cropped image being transformed was being calculated as a percentage of the height rather than of the width.

#11837
@brianjhanson
Copy link
Contributor

Thanks for reporting! That's been fixed for the next Craft 3 and 4 releases.

To get the fix early, change your craftcms/cms constraint in composer.json to v3.x-dev as 3.7.53.1 and run composer update.

@proimage
Copy link
Author

I see 3.7.53.1 was released, but the release notes don't mention this fix; did it make it in?

@brianjhanson
Copy link
Contributor

That version number is a little confusing because 3.7.53.1 was released before we made this fix. This change is still on deck to be included in the next release.

@brandonkelly
Copy link
Member

Craft 3.7.54 and 4.2.4 are now tagged with that fix.

@proimage
Copy link
Author

proimage commented Sep 14, 2022

I'm still seeing incorrect calculations after updating, although at least the aspect ratio of the width and height is correct.

Source image: 1080×594px (which is just a smidge wider than 16:9)

The code:

{% set aspectRatio = 16/9 %}
{% set width = 1280 %}
{% set height = width / (aspectRatio) %}
{% do imgAsset.setTransform({ width: width, height: height }) %}
{{ tag('img', {
	src: imgAsset.url(),
	width: imgAsset.width(),
	height: imgAsset.height(),
}) }}

Resulting image dimensions: 1056×594px ( = 16:9 with no upscaling)

Resulting code (+ linebreaks for formatting):

<img
    src="https://example.com/images/_1280x720_crop_center-center_none/MyImage.jpg"
    width="1080" height="608">

1080×608px is 16:9, but it's the wrong 16:9. ;)

I'd expect the <img> tag's width and height attributes to be 1056 and 594, respectively. Instead, it seems to be ignoring the setting for no upscaling when outputting the calculated image dimensions.

@brandonkelly
Copy link
Member

@proimage Should definitely be fixed now via #11940. Can you please test on your end, before we tag a new release with this?

To do that, change your craftcms/cms constraint in composer.json to v3.x-dev as 3.7.54, and run composer update.

@proimage
Copy link
Author

Can you please test on your end, before we tag a new release with this?

Sure thing; I'll do that in about 12 hours or so. ;)

@proimage
Copy link
Author

proimage commented Sep 15, 2022

Ok, it looks like that fixed the problem I reported, but it may have broken something else. This code is now triggering an error:

{% set image = entry.imageField.one() %}
{% set thumb = {
    mode: 'crop',
    width: min(1440, image.width),
    height: 'auto'
} %}
{# The next line triggers the error; it works without `thumb`, eg `image.width()`. #}
{{ image.width(thumb) }}
{# This line is also affected #}
{{ image.height(thumb) }}

The cause of the error seems to be when image.width() tries to parse the thumb object when the latter has 'auto' as the height. When the height is a number, there's no error.

The error I get:

yii\base\ErrorException: A non-numeric value encountered in /var/www/html/vendor/craftcms/cms/src/elements/Asset.php:2197
Stack trace:
#0 /var/www/html/vendor/craftcms/cms/src/web/ErrorHandler.php(87): yii\base\ErrorHandler->handleError(2, 'A non-numeric v...', '/var/www/html/v...', 2197)
#1 /var/www/html/vendor/craftcms/cms/src/elements/Asset.php(2197): craft\web\ErrorHandler->handleError(2, 'A non-numeric v...', '/var/www/html/v...', 2197, Array)
#2 /var/www/html/vendor/craftcms/cms/src/elements/Asset.php(1424): craft\elements\Asset->_dimensions(Object(craft\models\AssetTransform))
#3 /var/www/html/vendor/twig/twig/src/Extension/CoreExtension.php(1566): craft\elements\Asset->getWidth(Array)
#4 /var/www/html/vendor/craftcms/cms/src/helpers/Template.php(106): twig_get_attribute(Object(craft\web\twig\Environment), Object(Twig\Source), Object(craft\elements\Asset), 'width', Array, 'method', false, false)
#5 /var/www/html/storage/runtime/compiled_templates/2b/2b05dcedd78ae515396ffc9faecd357bfd19babcd06e7bacd2e95753ace1303a.php(122): craft\helpers\Template::attribute(Object(craft\web\twig\Environment), Object(Twig\Source), Object(craft\elements\Asset), 'width', Array, 'method')
#6 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_c4463042e5a00014908c878313f844d44f941dd7cb3b3b622f0f1e9a3180d2f8->doDisplay(Array, Array)
#7 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#8 /var/www/html/storage/runtime/compiled_templates/f1/f10a2ef3d300b8830a60f8a0b03b61e307215dd6d0363efb612984d2439b7411.php(173): Twig\Template->display(Array)
#9 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_10a633089c5fede12dae1c48497f483ac9c6cbd9afc0ee9d25f89346636c7f95->doDisplay(Array, Array)
#10 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#11 /var/www/html/storage/runtime/compiled_templates/a4/a4d2359fff63a9aa14acaf37b48c62598843c3256148fbe5957b9dc772d69498.php(136): Twig\Template->display(Array)
#12 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_c1412d7a8e5d26b501ab725074d150225447c61a68054b2f9465de04d20d3884->doDisplay(Array, Array)
#13 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#14 /var/www/html/storage/runtime/compiled_templates/b8/b8cbd219714ec10e45c6e49988c11cb90110dcd96aba4c3a1d1e38c54ee61a85.php(68): Twig\Template->display(Array)
#15 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_7c3eccff55686859f14cf3f47fc6a0910fe1f27322f9fd80e467c904398a81d1->doDisplay(Array, Array)
#16 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#17 /var/www/html/storage/runtime/compiled_templates/d0/d06cf984d284420cda2e469e6925026ea0cfec78329db3c6ddba0e0f825543f7.php(135): Twig\Template->display(Array)
#18 /var/www/html/vendor/twig/twig/src/Template.php(182): __TwigTemplate_6b529a398574d3ff5464205a39665bc6c29024a485ec2d6c69f45c4815a1aa07->block_content(Array, Array)
#19 /var/www/html/storage/runtime/compiled_templates/23/23ac34d3209c200b5e5489b5899a45608df60511e3a3f9286d13f017bc93a790.php(360): Twig\Template->displayBlock('content', Array, Array)
#20 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_a3fd9445c8ddbfedae54914e7a1ef443f06f67edea79aba9a86a70c20c1f552d->doDisplay(Array, Array)
#21 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#22 /var/www/html/storage/runtime/compiled_templates/d0/d06cf984d284420cda2e469e6925026ea0cfec78329db3c6ddba0e0f825543f7.php(43): Twig\Template->display(Array, Array)
#23 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_6b529a398574d3ff5464205a39665bc6c29024a485ec2d6c69f45c4815a1aa07->doDisplay(Array, Array)
#24 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#25 /var/www/html/storage/runtime/compiled_templates/53/5351d94fc2d736efbf2ef2fbafdaf4add66d8ed78896795e0a322292f21b03ce.php(44): Twig\Template->display(Array, Array)
#26 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_e6f7454ce5a599cae1e52ee943260c4fd8fcb389abc0afc2604190893f7eb576->doDisplay(Array, Array)
#27 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#28 /var/www/html/vendor/twig/twig/src/Template.php(390): Twig\Template->display(Array)
#29 /var/www/html/vendor/twig/twig/src/TemplateWrapper.php(45): Twig\Template->render(Array, Array)
#30 /var/www/html/vendor/twig/twig/src/Environment.php(318): Twig\TemplateWrapper->render(Array)
#31 /var/www/html/vendor/craftcms/cms/src/web/View.php(408): Twig\Environment->render('_pages/index', Array)
#32 /var/www/html/vendor/craftcms/cms/src/web/View.php(461): craft\web\View->renderTemplate('_pages/index', Array)
#33 /var/www/html/vendor/craftcms/cms/src/web/Controller.php(205): craft\web\View->renderPageTemplate('_pages/index', Array, 'site')
#34 /var/www/html/vendor/craftcms/cms/src/controllers/TemplatesController.php(102): craft\web\Controller->renderTemplate('_pages/index', Array)
#35 [internal function]: craft\controllers\TemplatesController->actionRender('_pages/index', Array)
#36 /var/www/html/vendor/yiisoft/yii2/base/InlineAction.php(57): call_user_func_array(Array, Array)
#37 /var/www/html/vendor/yiisoft/yii2/base/Controller.php(178): yii\base\InlineAction->runWithParams(Array)
#38 /var/www/html/vendor/yiisoft/yii2/base/Module.php(552): yii\base\Controller->runAction('render', Array)
#39 /var/www/html/vendor/craftcms/cms/src/web/Application.php(293): yii\base\Module->runAction('templates/rende...', Array)
#40 /var/www/html/vendor/yiisoft/yii2/web/Application.php(103): craft\web\Application->runAction('templates/rende...', Array)
#41 /var/www/html/vendor/craftcms/cms/src/web/Application.php(278): yii\web\Application->handleRequest(Object(craft\web\Request))
#42 /var/www/html/vendor/yiisoft/yii2/base/Application.php(384): craft\web\Application->handleRequest(Object(craft\web\Request))
#43 /var/www/html/web/index.php(22): yii\base\Application->run()
#44 {main}

@brandonkelly
Copy link
Member

@proimage The width and height params have never intentionally supported 'auto' values, and that would cause a PHP TypeError in Craft 4.

We’ve just made the code defensive for it though, so if possible it will catch those values and replace them with null, and log a warning about it. If you run composer update you’ll get that change.

@proimage
Copy link
Author

proimage commented Sep 16, 2022

Ahh, of course! 🤦‍♂️ I got so tangled up in the weeds with this that I didn't take a step back and think if 'auto' even makes sense there, seeing as it's Twig and not CSS.

As far as the real bug, it seems tenacious, or perhaps this is another bug that snuck into the patches.

{# imgAsset is a 3600×2400 file #}
{% set transform = {
	width: 1280,
	height: 720
} %}
{% do imgAsset.setTransform(transform) %}
{{ tag('img', {
	src: imgAsset.url,
	width: imgAsset.width,
	height: imgAsset.height
}) }}

The output is an img tag with a correctly-sized file, but incorrect width="..." and height="..." attributes, eg:

<img
    src="/path/to/images/_1280x720_crop_center-center_none/MyImage.jpg"
    width="3600" height="2025">

It's worth noting that if I alter the transform to omit either the width or the height, the output works as expected:

{# imgAsset is a 3600×2400 file #}
{% set transform = {
	width: 1280
} %}
{% do imgAsset.setTransform(transform) %}
{{ tag('img', {
	src: imgAsset.url,
	width: imgAsset.width,
	height: imgAsset.height
}) }}
<img
    src="/path/to/images/_1280xAUTO_crop_center-center_none/MyImage.jpg"
    width="1280" height="854">

Since the bug seems to only happen when both a width and a height are specified, would this be a good time or a bad time to dredge up #5288? 😉

@brandonkelly
Copy link
Member

@proimage Thanks! That’s been fixed as well now.

@proimage
Copy link
Author

@brandonkelly Hooray! That latest patch seems to have finally resolved all the situations I was encountering. Thanks for persevering with me on this—I really appreciate you all. :)

@brandonkelly
Copy link
Member

Thanks for all the help! Craft 3.7.55 and 4.2.5 are out with those fixes.

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

Successfully merging a pull request may close this issue.

3 participants