Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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() :-)
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 2424697-ResponsiveImageFormatter_preview-11.patch | 2.56 KB | jedihe |
| #11 | 2424697-ResponsiveImageFormatter_preview-11-TEST-ONLY.patch | 1.43 KB | jedihe |
| #11 | interdiff.txt | 1.27 KB | jedihe |
| #1 | 2424697-ResponsiveImageFormatter_preview-1.patch | 1.12 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch 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...
Comment #2
Wim LeersFor test inspiration: see
\Drupal\image\Tests\ImageFieldDisplayTest::_testImageFieldFormatters(), specifically this bit:Should be added to
\Drupal\responsive_image\Tests\ResponsiveImageFieldDisplayTest().Comment #3
jedihe CreditAttribution: jedihe commentedComment #4
jedihe CreditAttribution: jedihe commentedWith 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:
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:
Fixed:
Comment #5
jedihe CreditAttribution: jedihe commentedComment #6
jedihe CreditAttribution: jedihe commentedComment #7
yched CreditAttribution: yched commented@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...)
Comment #8
yched CreditAttribution: yched commentedAlso, @jedihe++ for the inception-style screenshots :-p
Comment #9
jedihe CreditAttribution: 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.
Comment #10
jedihe CreditAttribution: jedihe commentedFinally 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.
Comment #11
jedihe CreditAttribution: jedihe commentedJust 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.
Comment #13
jedihe CreditAttribution: jedihe commentedWell, 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.
Comment #14
Wim LeersGreat work, thanks!
Comment #15
yched CreditAttribution: yched commentedYay, thanks @jedihe & @dalguete !
Comment #16
alexpottThis 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!
Comment #19
YesCT CreditAttribution: YesCT commentedadding back the sprint tag.
Comment #20
dalguete CreditAttribution: dalguete commentedYay!!!... heh. I love to see this committed. Cool to work with you @jedihe and for the directions at DrupalCon @YesCT.