Field type plugin description is assumed to be an array

Opened an MR with a proposed fix. Maybe not the most beautiful and elegant, but it works. 😅

Should we just cast it to array, so a non array value becomes a single item list?

Was thinking about that, but a ul with 1 item is kinda crappy UI and semantics.

Note, this isn't going to cherry-pick to 10.2.x due to 📌 Make field selection a two-step form Fixed and a few other 11.x-only commits to this form. I'll wait to open a 10.2.x backport MR until this is further along and closer to commit for 11.x.

Hi!

I have adapted & implemented the changes done here (for 11.x) locally to a 10.2 (as FieldStorageAddForm is quite different) and fields with a 1-line description now appears properly (eg: reference fields or the ones of the address module with the patch in https://www.drupal.org/project/address/issues/3413017 🐛 Using a translatable string as a category for field type is deprecated in drupal:10.2 Needs work ), as you can see in both images uploaded.

Verified the MR applied working as expected.

Thanks for the reviews!

Tagging this as a regression in 10.2. the code in address.module used to work. Having core now assume the description must be an array is an unplanned / unhandled API change from previous versions.

Fix looks good to me.

Looks like ✨ Make field selection less overwhelming by introducing groups Fixed also missed updating docs on \Drupal\Core\Field\Annotation\FieldType::$description so we should do that here, otherwise we'll probably be in the same boat when we move that to an attribute.

We need a test here, We can use \Drupal\field_test\Plugin\Field\FieldType\TestItem as the guinea pig

@larowlan

I opened an issue about that

https://www.drupal.org/project/drupal/issues/3414368#comment-15422422 🐛 Incorrect types in variables used in Drupal\Core\Field\Annotation\FieldType Needs review

Is that sufficient? 😅 I realized the ER field descriptions are also missing in 11.x right now, not just address. Instead of a whole new test and using \Drupal\field_test\Plugin\Field\FieldType\TestItem, how about we ensure the default ER field descriptions appear? Fails locally on 11.x and passes on the MR branch.

I triggered the test-only job, let's see what happens: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/725176

Groovy, test-only failed exactly as expected:

There was 1 error:
1) Drupal\Tests\field\FunctionalJavascript\EntityReference\EntityReferenceAdminTest::testFieldAdminHandler
Behat\Mink\Exception\ResponseTextException: The text "An entity field containing an entity reference." was not found anywhere in the text of the current page.

Per brief Slack discussion in #bugsmash, both @mstrelan and @acbramley called my bluff and convinced me the previous commit isn't sustainable as test coverage for this bug. 😅 We'd need a bunch more comments around that assertion and why it's there, plus it's dependent on things that are likely to change, anyway. It'd be a lot better to have a dedicated test for this with both 1-line and array descriptions, and make sure both versions appear where expected.

Pushed commits for real test coverage. 😊 This is definitely better and more likely to survive going forward. Also rebased to latest 11.x while I was at it.

Locally, the test-only version fails against 11.x like so:

There was 1 error:

1) Drupal\Tests\field_ui\Functional\ManageFieldsTest::testAddField
Behat\Mink\Exception\ResponseTextException: The text "This one-line field description is important for testing" was not found anywhere in the text of the current page.

/.../drupal-11/vendor/behat/mink/src/WebAssert.php:811
/.../drupal-11/vendor/behat/mink/src/WebAssert.php:262
/.../drupal-11/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php:145
/.../drupal-11/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

I'll trigger the test-only job in GL-CI once the pipeline is far enough along to allow it.

Here's the test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/732794

There was 1 error:
1) Drupal\Tests\field_ui\Functional\ManageFieldsTest::testAddField
Behat\Mink\Exception\ResponseTextException: The text "This one-line field description is important for testing" was not found anywhere in the text of the current page.
/builds/issue/drupal-3415412/vendor/behat/mink/src/WebAssert.php:811
/builds/issue/drupal-3415412/vendor/behat/mink/src/WebAssert.php:262
/builds/issue/drupal-3415412/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php:145
/builds/issue/drupal-3415412/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 9, Assertions: 157, Errors: 1.

🎉

Already discussed this in slack and this is looking great, test coverage is perfect! Thanks @dww

Thanks! Yeah, the full pipeline is now done and happy, too: https://git.drupalcode.org/issue/drupal-3415412/-/pipelines/86342

larowlan → credited mstrelan → .

Crediting @gorkagr for triage etc and @mstrelan and @acbramley for comms in Slack with @dww

Fails on cspell with `muliple` unfortunately

Sorry about that. Fixed in the 11.x MR.

Full pipeline: https://git.drupalcode.org/issue/drupal-3415412/-/pipelines/86386 (all passed)
Test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/733182 (failed as expected)

Also, since this doesn't cleanly cherry pick to 10.2.x, opened a separate MR for that:
https://git.drupalcode.org/project/drupal/-/merge_requests/6424
Full pipeline: https://git.drupalcode.org/issue/drupal-3415412/-/pipelines/86392 (super weird, failed with a phpcs violation that I didn't touch with the changes in the branch).
Test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/733251 (failed as expected)

Okay, the 10.2.x MR should be happy now:

Full pipeline: https://git.drupalcode.org/issue/drupal-3415412/-/pipelines/86412 (all pass)
Test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/733522 (failed as expected)

In Slack, @larowlan told me to self-RTBC, so here goes.

larowlan → closed merge request !6217

larowlan → closed merge request !6424

Committed the appropriate MRs to 11.x and 10.2.x respectively

Thanks folks.

Yay, thank you.

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