ImageFormatter::viewElements() does:

  $url = NULL;
  if ($image_link_setting == 'content') {
      $entity = $items->getEntity();
      if (!$entity->isNew()) {
        $url = $entity->urlInfo();
      }
    }

(accounts for node preview - the parent node doesn't have a URL yet)

ResponsiveImageFormatter::viewElements() doesn't bother:

  $url = NULL;
  if ($this->getSetting('image_link') == 'content') {
    $url = $items->getEntity()->urlInfo();
  }

But Entity::urlInfo() throws an Exception if the entity isNew() :-)

CommentFileSizeAuthor
#11 2424697-ResponsiveImageFormatter_preview-11.patch2.56 KBjedihe
#11 2424697-ResponsiveImageFormatter_preview-11-TEST-ONLY.patch1.43 KBjedihe
#11 interdiff.txt1.27 KBjedihe
#10 interdiff.txt1.27 KBjedihe
#10 responsiveimageformatter-exception-on-node-preview-2424697-TEST-ONLY.patch1.43 KBjedihe
#10 2424697-ResponsiveImageFormatter_preview-10.patch2.56 KBjedihe
#4 2424697-fixed-after-patch-4.png106.32 KBjedihe
#4 2424697-error-before-patch-4.png30.67 KBjedihe
#1 2424697-ResponsiveImageFormatter_preview-1.patch1.12 KByched

Comments

yched’s picture

yched commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 2424697-ResponsiveImageFormatter_preview-1.patch1.12 KB

Patch fixes changes ResponsiveImageFormatter to do the same as ImageFormatter.
I guess that logic could be moved up to a separate helper method in ImageFormatterBase, not fully sure that's really worth it...

Also, this will need tests...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

For test inspiration: see \Drupal\image\Tests\ImageFieldDisplayTest::_testImageFieldFormatters(), specifically this bit: 

    // Ensure that preview works.
    $this->previewNodeImage($test_image, $field_name, 'article');

Should be added to \Drupal\responsive_image\Tests\ResponsiveImageFieldDisplayTest().

jedihe’s picture

jedihe commented
Issue tags: +LatinAmerica2015
jedihe’s picture

jedihe commented
Issue summary: View changes
StatusFileSize
new 2424697-error-before-patch-4.png30.67 KB
new 2424697-fixed-after-patch-4.png106.32 KB

With help from nelllaparedes and joosuuee, we did manual testing of the patch from #1. Results are good, Drupal breaks when trying to preview a node with an image field using a responsive image formatter (linked to 'content' in "manage display" options for the content type); the patch fixes the issue as expected .

Testing steps:

  1. Got a new env for D8 (standard profile) and installed Drush 7 with Composer.
  2. Enabled responsive images module with drush: drush en -y responsive_image.
  3. Created a new responsive image style, using the default image styles for the three breakpoints.
  4. We set the Responsive Image formatter for the Image field in the display options for the Article content type, also enabling the option to link to content (this last one setting is required to induce breakage).
  5. Browsed to /node/add/article, populated basic fields and chose an image for the Image field.
  6. Clicked the "preview" button and the site broke, stating that an error occurred (no details about the error were displayed).

After applying the patch, the same steps as above didn't give us an error. The node preview displayed just normally.

These screenshots show the two states (broken/fixed):

Broken:

Broken state

Fixed:

Fixed state

jedihe’s picture

jedihe commented
Status: Needs review » Reviewed & tested by the community
jedihe’s picture

jedihe commented
Issue summary: View changes
yched’s picture

yched commented
Status: Reviewed & tested by the community » Needs work

@jedihe: thanks for the screenshots and detailed steps to reproduce !

However, "Needs tests" more specifically means adding testing code in our test suite so that the behavior is automatically tested against potential regressions later.

Thus, the correct issue status is "needs work" atm, until those tests are added to the patch. Will try to work on that, but can't honestly say it's on the top of my list, so anyone is more than welcome to pick it up.

(and thanks a lot @Wim for the pointers in #2, finding the right place for new tests to fit given the size of our test suite can be a bit daunting...)

yched’s picture

yched commented

Also, @jedihe++ for the inception-style screenshots :-p

jedihe’s picture

jedihe commented

:D Apologies for not using the right status.

After our testing, we worked with @dalguete to get a test (following Wim's excellent suggestion). Our guesswork got us a few steps forward, but not to a working version.

jedihe’s picture

jedihe commented
StatusFileSize
new 2424697-ResponsiveImageFormatter_preview-10.patch2.56 KB
new responsiveimageformatter-exception-on-node-preview-2424697-TEST-ONLY.patch1.43 KB
new interdiff.txt1.27 KB

Finally managed to get the test working as needed: it fails without the fix in place, and the fix makes it pass. Let's see what our helpful bot has to say.

The test was written jointly with @dalguete.

jedihe’s picture

jedihe commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.27 KB
new 2424697-ResponsiveImageFormatter_preview-11-TEST-ONLY.patch1.43 KB
new 2424697-ResponsiveImageFormatter_preview-11.patch2.56 KB

Just learned on IRC that I must switch to Needs Review for the bot to see the patches. I'm reattaching the files, too.

Files are exactly the same as in #10, interdiff is against patch in #1.

The last submitted patch, 11: 2424697-ResponsiveImageFormatter_preview-11-TEST-ONLY.patch, failed testing.

jedihe’s picture

jedihe commented
Issue tags: -Needs tests, -LatinAmerica2015

Well, we have a working prototype for the test. Now we need a reviewer to provide suggestions on how to refactor/polish it in order to achieve the required quality for committing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Great work, thanks!

yched’s picture

yched commented

Yay, thanks @jedihe & @dalguete !

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed a0d5bc0 and pushed to 8.0.x. Thanks!

  • alexpott committed a0d5bc0 on 8.0.x 
    Issue #2424697 by jedihe, yched: ResponsiveImageFormatter throws an...

  • alexpott committed 574b30a on 8.0.x 
    Revert "Issue #2424697 by jedihe, yched: ResponsiveImageFormatter throws...
  • alexpott committed c7837f9 on 8.0.x
    Issue #2424697 by jedihe, dalguete, yched: ResponsiveImageFormatter...
yesct’s picture

yesct commented
Issue tags: +LatinAmerica2015

adding back the sprint tag.

dalguete’s picture

dalguete commented

Yay!!!... heh. I love to see this committed. Cool to work with you @jedihe and for the directions at DrupalCon @YesCT.

Status: Fixed » Closed (fixed)

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