Formatter 'URL to image' from ImageUrlFormatter shows PHP warning due to the newly introduced loading attribute on image field

Thank you!

Hmm not sure about this. ImageUrlFormatter is loading the settingsForm from the parent but not loading the default settings from the parent, so that discrepancy is what the root cause of this problem is.

See Base64ImageFormatter for what works: that inherits neither the default settings nor the settingsForm, so that doesn't break.

Don't know if this should either inherit both or should inherit neither, but I think the fact that it's doing one of the two is what causes this to break.

Tested the PR
Confirmed the issue without the patch
Confirmed the PR fixed the issue.
Attaching screenshots

I'm having the same issue on my site. Is there a patch for this at all?

There's a PR up.

@smustgrave I've never had any experience using the PR before. I've always just come to find patches that I applied and have been pretty comfortable with. I seem to be a bit confused about how I would apply this to my site? Its composer run with the drupal/recommended-project starter.

Gotcha

If you open the PR https://git.drupalcode.org/project/drupal/-/merge_requests/2416 you can append .diff or .patch to the end of the URL and you can past that into composer.json file and use it just like a patch

Perfect. Thanks for being helpful and teaching me something new :)

It was pointed out to me you shouldn't point directly to the URL but download the file instead. For security purposes.

I have only skimmed the IS and the comments. I notice that the changes made to the code in #5 have been tested but have not had a code review.

Setting to NR for a code review.

The change looks good, but the fact that just using this plugin should trigger this warning, it feels we have no test coverage for this plugin, and we should probably add it.

Took a shot at the test.

While testing with the original MR I was getting

Drupal\Core\Config\Schema\SchemaIncompleteException : Schema errors for core.entity_view_display.node.article.default with the following errors: core.entity_view_display.node.article.default:content.qymfsdps.settings.image_link missing schema, core.entity_view_display.node.article.default:content.qymfsdps.settings.image_loading missing schema

So maybe the default settings aren't needed because we aren't using those settings? Could be wrong.

Patch in #16 Applied Cleanly and working fine on local 9.4

@smustgrave, I have ideas about the schema validation error.

The schema for the image_url formatter is as follows:

field.formatter.settings.image_url:
  type: mapping
  label: 'Image URL formatter settings'
  mapping:
    image_style:
      type: string
      label: 'Image style'

When the parent::defaultSettings() are added, we get additional settings for image_link and image_loading. Those settings are not defined in the schema for the image_url formatter, so that is going to lead to schema validation errors.

So I would say definitely do not want to add the parent's default settings. And they aren't even needed as long as the ImageFormatter can account for empty values of image_loading and image_link, which I think has to be the goal here.

Probably at this point it no longer makes sense for ImageUrlFormatter to extend ImageFormatter. But untangling that would be a lot harder than simply making the ImageFormatter code a bit more defensive.

Hope that helps!

I've just tested patch #16, and it renders the image instead of just printing the URL/path.

Tested on Drupal 9.4.x and php 8.1

- Issue summary is clear and describes the problem
- was able to reproduce the issue
- The patch applies cleanly and fixes the issue
- Tests pass

Marking this as RTBC

Applied #16 Works

I believe 100% that the patch fixes the pho error. And the approach in the MR is 100% not what we want, as described in #18. But we can't set this to RTBC yet. We still need a fail test.

@smustgrave made an excellent attempt in #16 for a tests-only patch, but the tests passed. We as a community still need to get the fail test sorted out.

Thanks, @smustgrave! Fail test fails now, but I think it can be meaningfully improved.

1. I think testImageLoadingAttribute() is the wrong place for this since it doesn't really have anything to do with the loading setting. I would prefer to see this in _testImageFieldFormatters, which is in the same class. At the very end of that method there are some existing assertions related to the image_url formatter. That seems like the most logical place to put an additional assertion like this.

2. I think you can drop the assertion related to the absence of the php warning. If there is a warning, the test will fail before even getting to that assertion.

+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -464,6 +464,18 @@ public function testImageLoadingAttribute(): void {
+    $this->assertSession()->responseNotContains('Warning: Trying to access array offset on value of type null in Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter->settingsSummary()');

You should be able to just assert that some expected settings summary text is present.

sure. Made those updates.

@smustgrave, I'm channeling my inner committer and the test is not quite there. I'm feeling like a should have been clearer and more expansive on a couple things in my previous comment.

+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -230,6 +230,17 @@ public function _testImageFieldFormatters($scheme) {
+    // Test to make sure null error is not appearing.

More accurately we're testing that the settings summary is correct for the image_url formatter. I don't think we want this comment to be so specific to the error in this issue. Can we change to something like "Test the settings summary."?

+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -230,6 +230,17 @@ public function _testImageFieldFormatters($scheme) {
+    $this->drupalGet("admin/structure/types/manage/" . $node->getType() . "/display");

After getting to the manage display page, we should assert that the settings summary appears as expected, because that's at the heart of what is being tested here. I think the expected summary would be "image_style: Thumbnail (100×100)". Can we assert that this text is present?

+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -20,8 +20,8 @@ class ImageFieldDisplayTest extends ImageFieldTestBase {
   use TestFileCreationTrait {
-    getTestFiles as drupalGetTestFiles;
-    compareFiles as drupalCompareFiles;
+    TestFileCreationTrait::getTestFiles as drupalGetTestFiles;
+    TestFileCreationTrait::compareFiles as drupalCompareFiles;
   }

I also forgot to comment on this in my previous notes. This looks out of scope and we should revert this change. Even positive changes like this tend to hold up commits when they are out of scope.

Sorry again that my previous comment was not clearer. I swear I'm not trying to make extra work for you!

No I appreciate all the rapid feedback!

Almost done, @smustgrave!

There are two lines at the end of you patch that are duplicated a few lines above. Can you remove these lines?

+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -230,6 +233,20 @@ public function _testImageFieldFormatters($scheme) {
+    $expected_url = \Drupal::service('file_url_generator')->transformRelative(ImageStyle::load('thumbnail')->buildUrl($image_uri));
+    $this->assertEquals($expected_url, $node->{$field_name}->view($display_options)[0]['#markup']);

Then I'll flip the switch to RTBC! Thanks for all the hard work getting this across the finish line.

Sure!

Those fails all look unrelated, but not random. I think head is broken.

Can we get the test only patch too, @smustgrave? This version of the test has not failed yet here. In #28 it failed because of the logged out state. We need to run it and show that it fails because of the settings summary bug.

I also just noticed we're targeting the wrong branch. I wouldn't expect any re-rolling to be required though.

Back to NW to get the latest test only patch.

Re uploaded the full patch too

Alright! That fail test triggers the exact error described in the IS:

1) Drupal\Tests\image\Functional\ImageFieldDisplayTest::testImageFieldFormattersPublic
Exception: Warning: Trying to access array offset on value of type null
Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter->settingsSummary()

The patch fixes it.

The patch has been used by the community.

RTBC!

thanks for all the help!

Let's open a follow-up to make ImageUrlFormatter extend directly from ImageFormatterBase instead of ImageFormatter. It looks like the new test coverage will still be relevant once we've done that though too which is great.

Adding tag for #40.

Opened https://www.drupal.org/project/drupal/issues/3306066

Isn't the config schema for image_loading also incorrect given that the value is not marked as nullable?

can you elaborate more please?

Thanks for the review @lauriii

I only find three instances in core of nullable: so it does not appear to be in common usage within our schema. I had never heard of this before.

Locally, I added nullable: true to image_loading and the fail test still fails. To me that indicates that the bug here is unrelated to image_loading being specified as nullable. If the schema should be updated, it should probably be done in a separate issue.

I'm going to throw this back to RTBC.

Patch #32 worked for me on 9.4.5

Confirming that #35 fixed the issue for me as well on two sites, both on 9.4.5. Thanks!

Looking at #3306066: Make ImageUrlFormatter extend directly from ImageFormatterBase I think we should fold it back into this issue. It might mean we don't need to change any logic except for what gets subclassed - test coverage will show whether or not that's the case.

Moved over changes from https://www.drupal.org/project/drupal/issues/3306066 and closed it out.

Not sure why it retested #49 so please ignore that.

Here is the patch against 9.4.5 where only two below notices are coming.

Notice: Trying to access array offset on value of type null in /mnt/www/html/veoliad801dev4/docroot/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php on line 210

Notice: Trying to access array offset on value of type null in /mnt/www/html/veoliad801dev4/docroot/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php on line 167

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -164,7 +164,7 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
       '#type' => 'radios',
-      '#default_value' => $image_loading['attribute'],
+      '#default_value' => !empty($image_loading['attribute']) ? $image_loading['attribute'] : '',
       '#options' => $loading_attribute_options,
       '#description' => $this->t('Select the loading attribute for images. <a href=":link">Learn more about the loading attribute for images.</a>', [
         ':link' => 'https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes',
@@ -206,9 +206,11 @@ public function settingsSummary() {

@@ -206,9 +206,11 @@ public function settingsSummary() {
     }
 
     $image_loading = $this->getSetting('image_loading');
-    $summary[] = $this->t('Image loading: @attribute', [
-      '@attribute' => $image_loading['attribute'],
-    ]);
+    if (!empty($image_loading)) {
+      $summary[] = $this->t('Image loading: @attribute', [
+        '@attribute' => $image_loading['attribute'],
+      ]);
+    }
 

Can these hunks be dropped from the patch if the url formatter no longer needs them?

Patch No. #51 tested and applied successfully on drupal 9.4.x version.

Still needs review and an answer to #55.

Hi, @gajendra_sharma! Thanks for jumping in here.

We as a community need to be careful about setting an issue to RTBC. That's the sign to core committers that the issue is ready for their attention. Core committers have very little precious time, and we don't want them to waste any of it reviewing issues that aren't actually ready.

If you want to update the patch #54 to address the comments in #55, that would be welcome. And any new patch should (almost) always be accompanied by an interdiff.

If you need help with contribution, the #contribute channel in Drupal Slack is a great resource. And this issue has been tagged by the Bug Smash Initiative, so the #bugsmash channel would be a good place to turn as well.

Cheers!

Moving back to 9.5.x not sure why it was moved to 9.4

#51 should be used going forward for changes. before breaking changes start getting mixed in. Hiding #54 and #56

Looking into #55

Removing those changes mentioned in #55 didn't cause any issues.

#61 looks like I hoped it would - no longer needs the special casing.

Committed and pushed b8ad2d90b4 to 10.1.x and 3cdcdbc45f to 10.0.x and a3c8bb27f1 to 9.5.x. Thanks!

diff --git a/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
index 71bf26bc3a..78ea4061eb 100644
--- a/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
@@ -113,8 +113,8 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
       '#empty_option' => t('None (original image)'),
       '#options' => $image_styles,
       '#description' => $description_link->toRenderable() + [
-          '#access' => $this->currentUser->hasPermission('administer image styles'),
-        ],
+        '#access' => $this->currentUser->hasPermission('administer image styles'),
+      ],
     ];
 
     return $element;

Fixed coding standards on commit.

diff --git a/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
index 78ea4061eb..dc4557949c 100644
--- a/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
@@ -107,10 +107,10 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
       Url::fromRoute('entity.image_style.collection')
     );
     $element['image_style'] = [
-      '#title' => t('Image style'),
+      '#title' => $this->t('Image style'),
       '#type' => 'select',
       '#default_value' => $this->getSetting('image_style'),
-      '#empty_option' => t('None (original image)'),
+      '#empty_option' => $this->t('None (original image)'),
       '#options' => $image_styles,
       '#description' => $description_link->toRenderable() + [
         '#access' => $this->currentUser->hasPermission('administer image styles'),
@@ -133,10 +133,10 @@ public function settingsSummary() {
     // their styles in code.
     $image_style_setting = $this->getSetting('image_style');
     if (isset($image_styles[$image_style_setting])) {
-      $summary[] = t('Image style: @style', ['@style' => $image_styles[$image_style_setting]]);
+      $summary[] = $this->t('Image style: @style', ['@style' => $image_styles[$image_style_setting]]);
     }
     else {
-      $summary[] = t('Original image');
+      $summary[] = $this->t('Original image');
     }
 
     return array_merge($summary, parent::settingsSummary());

Also fixing use of t() in plugins... shoudl be $this->t()