Media image thumbnail incorrectly ends up as NULL when it should be an empty string

Hey together,

I have research a little bit on this and I am sure, that the patch in die issue fork solve this.
The related property here is thumbnail__alt (Database column in media_field_data which comes from the BaseFieldDefinition in the Media Entity itself.
This property/column stay's null if you upload an image to a media entity based on the image source plugin and left the alt field empty.
But the ImageItem field plugin already has a default value for alt which is an empty string.
see: https://git.drupalcode.org/issue/drupal-3319601/-/blob/3319601-media-ima...

For remote video media entities based on the oembed source plugin this issue doesn't come up because the default value is set on the updateThumbnail method in the Media entity.
see: https://git.drupalcode.org/issue/drupal-3319601/-/blob/3319601-media-ima...

So the issue is "only" related to media entities which are based on media source plugins that have $thumbnail_alt_metadata_attribute defined.

Feels like something that should probably have some kind of test coverage even though it's a small change.

Added test coverage in a distinct test file to assess specific scenario.

Please review, move NR

@pooja_sharma

Thank you a lot for pushing this forwars with a functional test. :-)
I have changed the test cases a bit to have to specific test case for given strings and a test case with no given alt attribute.
Both cases have to result in having a rendered alt attribute.

I am unsure if we have to add a specific alt = NULL testcase, because this is not intended by the used field API here. ImageItem have default values for it's properties which should not altered or?

Nice, the test coverage isn't triggering the bug it seems though, the test-only run https://git.drupalcode.org/issue/drupal-3319601/-/jobs/2003864 is green

Thanks for review, recent code changes lead to pass test case in every scenario which is not correct , so revert back changes.

Alt attribute field is bind with image media entity if alt attribute not attach with empty string then it 'll not be use of adding this specific test case as in manual test if we keep empty "attribute field" then alt tag not render on FE which leads to issue.

PLease review, moving to NR

mea culpa, I did make an understanding failure. Thought it is defaulting to the ImageItem default settings, which is not, because here is no field api involved in the test szenario. :-)
Thank you for pointing that out, did learn something new with testing! :)
Should be good to go.

Rebased seems working fine

Left a comment on the MR.

Addressed the mentioned suggestions & removed the newly created test file as test code moved to existing suggested test file.

Please review , moving NR

Left some comments on the MR

pooja_sharma → changed the visibility of the branch 3319601-media-image-thumbnail to hidden.

pooja_sharma → changed the visibility of the branch 3319601-media-image-thumbnail to active.

Applied suggestion & left comments as well.

Please review, moving NR

Will leave in review then for others but think the other test location makes more sense since the test there is checking the media content view while MediaSourceImageTest is not.

Applied the suggestions, Moved test coverage code from MediaSourceImageTest to MediaOverviewPageTest

Re-ran failure for the editor module and appeared to be random.

Believe all feedback for this one has been addressed.

Thanks for continuing to work on this!

I left some comments in the MR. I am also not sure that this is the best way to test this. I'll ask about that.

Addressed the feedback, Please review the MR, moving NR.

Added a review for proposing null coalescing operator. So the change will be just 1 character. :-P
Thank you @pooja_sharma for your fast interaction on this!

Feedback from #38

Yes thanks @pooja_sharma for keeping this one alive.

Addressed the requested changes, Please review, moving NR.

Believe feedback has been addressed.

Some nits about the test case.

Addressed the feedback, rebased the MR, apart from it nothing seems to be left.

Please review, moving NR.

Believe feedback around test has been addressed.

I didn't find any unanswered questions here. Updated the credit, though it should be check by the committer.

I made suggestions to comments in the test and applied them. They are both minor and I double checked them, so I am leaving this at RTBC

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch → closed merge request !8548

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