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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,020 pass(es). View
#11 2424697-ResponsiveImageFormatter_preview-11-TEST-ONLY.patch1.43 KBjedihe
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,912 pass(es), 2 fail(s), and 0 exception(s). View
#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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,641 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

yched’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,641 pass(es). View

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

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

Issue tags: +LatinAmerica2015
jedihe’s picture

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

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

Issue summary: View changes
yched’s picture

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

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

jedihe’s picture

: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

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

Status: Needs work » Needs review
FileSize
1.27 KB
1.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,912 pass(es), 2 fail(s), and 0 exception(s). View
2.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,020 pass(es). View

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

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

Status: Needs review » Reviewed & tested by the community

Great work, thanks!

yched’s picture

Yay, thanks @jedihe & @dalguete !

alexpott’s picture

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

Issue tags: +LatinAmerica2015

adding back the sprint tag.

dalguete’s picture

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.