ImageFieldTestBase::createImageField() accepts a $field_settings parameter that is supposed to contain both the settings, and the 'description' for the field, next to each other in the same set of array keys.

That makes no sense, and would clash if there was a setting called 'description'.
Not sure that helper method even needs to bother with setting a field description ?

CommentFileSizeAuthor
#5 remove-description.patch2.39 KBgábor hojtsy
#2 remove-description.patch2.39 KBgábor hojtsy
#2 move-description.patch2.88 KBgábor hojtsy

Comments

yched’s picture

yched commented
Issue tags: +Novice

Good novice task :-)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Title: Fix ImageFieldTestBase::createImageField() » ImageFieldTestBase::createImageField() takes a description in field settings errorneously
Status: Active » Needs review
StatusFileSize
new move-description.patch2.88 KB
new remove-description.patch2.39 KB

Wanted to make sure to attend to this followup, so even though marked as Novice, here it is :) One version to move the argument (but only found one use). And one version to remove it (which as you said should hopefully be even better).

tibbsa’s picture

tibbsa commented

At the moment, there is no testing purpose for keeping the description, I don't believe. But for the sake of future extensibility, making it apparent that this is a "possibility" is probably not a bad thing. I'd vote for "moving" rather than "removing".

yched’s picture

yched commented

Thanks @Gabor :-)
No big deal, but I'd vote for "remove". There's little point in keeping the description IMO.
If a specific test needs to specifically test something with the description (should be rare, the "description" is UI-only...), it can always set it itself. This is only a helper method for brevity, it's only normal that it doesn't have all the verbosity and features of calling the actual "create" functions itself.

+ FileFieldTestBase has a similar createFileField() that doesn't take a description, so... consistency++ :-)

So, RTBC for me for the "remove patch".

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new remove-description.patch2.39 KB

Reuploading for easier committer review then.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

:-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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 49b1c1d and pushed to 8.0.x. Thanks!

  • alexpott committed 49b1c1d on 8.0.x 
    Issue #2376899 by Gábor Hojtsy: ImageFieldTestBase::createImageField()...

Status: Fixed » Closed (fixed)

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