MediaThumbnailFormatter => Calling ImageFormatter::__construct() without the $file_url_generator argument is deprecated in drupal:9.3.0

Hey @heddn I think it's a good catch! I also think you forgot to document the new $file_url_generator variable on MediaThumbnailFormatter.php. I'm uploading a new patch with the doc to see it the patch now applies on 9.3.

I added a new tag for deprecated scenarios, I don't think it's a bug but let's see the opinion of other person.

Thanks!

I'm wondering why tests didn't fail. Is it possible that this formatter has no test coverage?

Hey @berdir, I think you're right, we have tests for OEmbedFormatter but none for MediaThumbnailFormatter, good catch!

I'll move this back to NW and add need tests tag here.

Thanks!!

Hey, I created a new test for MediaThumbnailFormatter, I'll move this back to needs review to see what you guys think.

Thanks!

Hey @heddn, on the module we have two field formatters, OEmbedFormatter and MediaThumbnailFormatter. The first one has a functional test to cover how the field would be rendered, my idea was to follow the same approach for MediaThumbnailFormatter.

#3264061: Remove deprecated functions from image module was committed so this should probably fail against 10.0.x.

Phpstan caught this locally. Looking forward to #3259355: Always do a full phpstan analysis on DrupalCI catching it first in CI though :-D

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php
@@ -97,7 +101,6 @@ protected function needsEntityLoad(EntityReferenceItem $item) {
   public function settingsForm(array $form, FormStateInterface $form_state) {
     $element = parent::settingsForm($form, $form_state);
-
     $link_types = [

👀

The fix makes sense. I didn't look too close at the test but I skim sort of makes sense and on 10.0.x the test fails without the fix but passes with it.

Looks like this will fix the following phpstan failure on full runs: https://www.drupal.org/pift-ci-job/2319582

Running PHPStan on *all* files.
 ------ -------------------------------------------------------------------------------- 
  Line   core/modules/media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php  
 ------ -------------------------------------------------------------------------------- 
  62     Method                                                                          
         Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter::__construct(           
         ) invoked with 9 parameters, 10 required.                                       
 ------ -----------------------------------------------

Committed 51c1ef6 and pushed to 10.0.x. Thanks!

Will cherry-picked to 9.4.x and 9.3.x after the freeze.

Exactly! Thanks for the speedy commit.

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php
@@ -55,11 +56,13 @@ class MediaThumbnailFormatter extends ImageFormatter {
    */
-  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, ImageStyleStorageInterface $image_style_storage, RendererInterface $renderer) {
-    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $current_user, $image_style_storage);
+  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, ImageStyleStorageInterface $image_style_storage, FileUrlGeneratorInterface $file_url_generator, RendererInterface $renderer) {
+    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $current_user, $image_style_storage, $file_url_generator);
     $this->renderer = $renderer;
   }
 

Thinking about it more, I think the 9.4.x/9.3.x backport should include a 'best effort' deprecation with the constructor - i.e. handle both the old and new constructor arguments and trigger a deprecation if it's called with the old arguments. No need for deprecation test coverage etc. Since the 10.0.x patch represents the 'final' state we can just leave it as is.

@catch, you mean something like this patch for 9.4.x/9.3.x backport?

FileUrlGeneratorInterface $file_url_generator = NULL, RendererInterface $renderer

Optional before required parameter? I think you'll have to remove the types from the two parameters in the signature and do some juggling if you're shifting arguments like this. It's not just the missing parameter but the parameter order that needs bc.

Yep both arguments would have to have no type and default to null, then instanceof checks in the constructor to see what we're getting.

Thanks @neclimdul and @catch, maybe something like this is enough here.

The added test is the same as the one committed to D10.
The parameters are added correctly.
For me it is RTBC.

Committed/pushed to 9.4.x and cherry-picked to 9.3.x, thanks!

This appears to have broken 9.4.x/9.3.x after commit, reverting for now.

See https://www.drupal.org/pift-ci-job/2324301 for an example of the test failures. They appear to be limited to PHP 7 environments.

Have a theory about what's going on.

1. +++ b/core/modules/media/tests/src/Functional/FieldFormatter/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,146 @@
   +    $this->assertSession()->responseContains($renderer->renderRoot($image));
   

   After this line the $image['#printed'] === TRUE

2. +++ b/core/modules/media/tests/src/Functional/FieldFormatter/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,146 @@
   +    $image['#url'] = $node->toUrl();
   +    $this->assertSession()->responseContains($renderer->renderRoot($image));
   

   This url change does nothing because render see's its already "printed" and does nothing giving an empty string.

Just reading the code everything seems pretty logical, but some that render array reference logic confused things. So this never tested what it thought but the question is why mink thinks asserting an empty string is OK in php8 and a failure in php7.

The fix is going to be just make a fresh image array. On top of printed, and the cached render information is already on the array and will be reused.

The answer to the mink question will blow your mind....
https://3v4l.org/VGB5j

@neclimdul, WOW, lol. How is that the newer behavior?

It's like Xeno's paradox of strings or something. Like... empty string appears at position zero of a string? Then it must also be at position one. And every position. Actually, there are infinitely many empty strings in a string, by that logic. (Or, well, finite, but at least as many as there are possible substrings. Still bizarre.)

Thank you so much for bringing this up @neclimdul! Looking deeply here, the test is not actually testing what I really wanted.

I've refactored the test, so sorry about fails.

I'm uploading two patches, one for 9.3/9.4 and the other updates 10.0

FYI: My idea was to test the following scenario:

1. Create a new content type with an image reference field
2. Change the display of the field and to use media_thumbnail_formatter
3. Create a new node
4. Check the node to see if the image is linked to the media or node

It's weird, testing locally here(branch 9.3.x, php7.4 and phpunit 8.5.21) the test pass, moving back to NW.

+++ b/core/modules/media/tests/src/Functional/FieldFormatter/MediaThumbnailFormatterTest.php
@@ -93,34 +90,21 @@ public function testRender() {
     $this->submitForm($edit, 'Save');
 
+    // Validate the image being loaded with the media reference.
+    $this->assertSession()->responseContains('<a href="/media/' . $mediaImage->id() . '/edit');
+

testbot is running in a subfolder, that's why this doesn't match. build the url with $mediaImage->toUrl('edit-form')

Thanks for the help @berdir! Let's see now.

1. +++ b/core/modules/media/tests/src/Functional/FieldFormatter/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,130 @@
   +    ]);
   +    $file->save();
   +    $mediaImage = Media::create([
   +      'bundle' => 'image',
   +      'name' => 'Test image',
   +      'field_media_image' => $file->id(),
   +    ]);
   +    $mediaImage->save();
   +
   +    // Save the article node.
   +    $edit = [
   +      'title[0][value]' => $this->randomMachineName(),
   +    ];
   +    $edit['field_media_reference[0][target_id]'] = $mediaImage->getName();
   +    $this->drupalGet('node/add/article');
   +    $this->submitForm($edit, 'Save');
   

   this is a somewhat strange mix of creating the media in the test code and the node through the UI, why not just create the node in the same way, then you don't need to do that preg match to find the node id. (Alternatively, use \Drupal\Tests\node\Traits\NodeCreationTrait::getNodeByTitle())

2. +++ b/core/modules/media/tests/src/Functional/FieldFormatter/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,130 @@
   +    // Validate image being loaded with the content on the link.
   +    $this->assertSession()->responseContains('<a href="/node/' . $nid);
   +  }
   

   there is a second assert here, that one might be failing now?

Thanks again for the responses @berdir, regarding #40:

1. I had to create the media and upload a file in order to able to see the file on the reference field, this field is using the thumbnail formatter, but on this patch I used your suggestion and now I'm using the NodeCreationTrait::getNodeByTitle() instead of the preg match. If you have any other ideas about how to improve this test(without creating the media) please let me know.
2. Yep, this one was failing the test, let's see it now.

FYI: Once again I'm uploading two patches(D9 and D10) along with two interdiffs one for D9 and one for D10.

Ok, I've uploaded the patch with the same name in my test issue, let's see now with a different name.

This change should probably be minor-only since fixing the deprecation requires reordering the constructor arguments.

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php
@@ -97,7 +109,6 @@ protected function needsEntityLoad(EntityReferenceItem $item) {
     $element = parent::settingsForm($form, $form_state);
-
     $link_types = [

Removing this blank line is out of scope. Otherwise the test change and deprecation fix look correct to me.

Updated patch #43 by addressing #46, please review it.

Thanks!

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php
@@ -55,11 +56,21 @@ class MediaThumbnailFormatter extends ImageFormatter {
+      @trigger_error('Calling ' . __METHOD__ . '() without the $file_url_generator argument is deprecated in drupal:9.3.0 and $file_url_generator argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3255887', E_USER_DEPRECATED);
...
+      @trigger_error('Calling ' . __METHOD__ . '() without the $renderer argument is deprecated in drupal:9.3.0 and $renderer argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3255887', E_USER_DEPRECATED);

Oops, I forgot that these lines need to be updated to 9.5.0 instead of 9.3.0 now.

Updated patch #47 as per mentioned in #48, please review it.

Thanks again - #49 is good for 9.x and #43 for 10.x.

Committed/pushed/cherry-picked to 10.1.x/10.0.x and 9.5.x respectively, thanks!

Hi @catch,

the committed changes avoid applying an old patch for https://www.drupal.org/project/drupal/issues/2961404 so I have to create an updated patch for that issue. But I'd like to know if there is any chance to fix that issue because there are a lot of related issues with missing language handling in MediaThumbnailFormatter and ImageFormatter that are still active.

Cheers Ralf