Steps to reproduce:
* create a responsive image mapping
* do NOT select an image style
* view the node - it will throw an exception:

Fatal error: Call to a member function buildUrl() on a non-object in C:\Projects\drupal8\core\modules\responsive_image\responsive_image.module on line 330

CommentFileSizeAuthor
#9 responsive_image-2349859-9.patch6.33 KBnaxoc
#9 responsive_image-2349859-9-test-only.patch5.15 KBnaxoc
#5 responsive_image.module.patch1.18 KBrvolk
#2 responsive_image.module.patch1.21 KBrvolk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

attiks CreditAttribution: attiks commented
Parent issue: » #2260061: Responsive image module does not support sizes/picture polyfill 2.2
Related issues: +#2334387: UI changes to support current responsive image standards
rvolk’s picture

rvolk
Primary language German
Location Frankfurt
CreditAttribution: rvolk commented
Priority: Normal » Major
Status: Active » Needs review
Issue tags: +image_styles
FileSize
responsive_image.module.patch1.21 KB

Check this patch with a non-image-style fallback solution.
The default file_create_url function will be used, if the image_style entity is not available.

There is as well a following error fixed in the patch:

PHP Fatal error: Call to a member function transformDimensions() on a non-object in /core/modules/responsive_image/responsive_image.module on line 319
mgoedecke’s picture

mgoedecke CreditAttribution: mgoedecke commented
Status: Needs review » Reviewed & tested by the community

patch solves both problems

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Reviewed & tested by the community » Needs work 
+  if ($entity = entity_load('image_style', $style_name)
+      and $entity instanceof Drupal\image\Entity\ImageStyle) {

Put both on 1 line and don't use 'and' we *never* do that in core.

rvolk’s picture

rvolk
Primary language German
Location Frankfurt
CreditAttribution: rvolk commented
Status: Needs work » Needs review
FileSize
responsive_image.module.patch1.18 KB

Here we go... thanks for the advice.

Lukas von Blarer’s picture

Lukas von Blarer
he/him
CreditAttribution: Lukas von Blarer commented
Status: Needs review » Reviewed & tested by the community

Work for me. RTBC i guess...

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Looks like this needs an automated test.

naxoc’s picture

naxoc CreditAttribution: naxoc commented
Assigned: Unassigned » naxoc
naxoc’s picture

naxoc CreditAttribution: naxoc commented
Status: Needs work » Needs review
FileSize
responsive_image-2349859-9-test-only.patch5.15 KB
responsive_image-2349859-9.patch6.33 KB

Here is a test that should catch that.

I'm not sure how many ways responsive image mappings can be created but I think we should make the image style field required on the form?

Also, no interdiff - I only touched the test.

The last submitted patch, 9: responsive_image-2349859-9-test-only.patch, failed testing.

attiks’s picture

attiks CreditAttribution: attiks commented
Status: Needs review » Reviewed & tested by the community

Test looks good, back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

So no one seems to have answered the question - should we be able to create mappings without an image style. Looking at the code it seems so since

    $image_styles = image_style_options(TRUE);

passing in TRUE means we get a none option.

Whilst reviewing the code I've realised that ResponsiveImageMappings are missing a dependency on the ImageStyle config entities - created #2383165: ResponsiveImageStyle config entities should depend on the image styles they use

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 82584e6 and pushed to 8.0.x. Thanks!

  • alexpott committed 82584e6 on 8.0.x 
    Issue #2349859 by R. Volk, naxoc: Responsive Image Mappings :: throws a...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.