Fix regression in the link widget where help text does not show

What was wrong with the patch from #2416987-61: Fix UI regression in the menu link form?

Mostly testing out the credit link on a patch I've already contributed.

@YesCT, in #4 I linked to a specific comment from [#9580553], which is comment #61 from that issue :)

Correcting non standard tag 'Need tests' to 'Needs tests'

1. The patch from #6 includes changes that are out of scope for this issue. Per YesCT in #2:

   next step is to strip out all the unrelated stuff from that patch

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -184,16 +192,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      if ($this->supportsExternalLinks() && $this->supportsInternalLinks()) {
   +        $element['uri']['#description'] .= $this->t('This can be an internal Drupal path such as %add-node or an external URL such as %drupal. Enter %front to link to the front page.', array('%front' => '<front>', '%add-node' => 'node/add', '%drupal' => 'http://drupal.org'));
   +      }
   +      elseif ($this->supportsExternalLinks() && !$this->supportsInternalLinks()) {
   +        $element['uri']['#description'] .= $this->t('This must be an external URL such as %drupal.', array('%drupal' => 'http://drupal.org'));
   +      }
   

   User facing strings of an example url should no longer use drupal.org, see #2418209: Replace user facing strings that use drupal.org as example of an external url.

I ran into this issue while building a Drupal 8 site. Had me wondering for a while :)

Tested the patch and it does not apply anymore. So here is a reroll, which also include these changes:

- Refactor all cases of testing for help texts into a loop (maybe address your concern #2). This also addresses concern #3 and #4.

Now, the code changes in this patch addresses the mentioned regression, but I was wondering about another thing (potentially another issue i guess):
- Why do we hide the description for internal links (and instead rely on the "helpful" prefix)? The help text that in the case of external/internal states that "Start typing the title of a piece of content" and this also applies when the setting is set to "only internal", only no-one gets this helpful hint. In fact, adding the prefix kind of makes you think that the input is required to be a internal URL, when it can in fact be the title of a node.

Thoughts?

Setting to needs review.

Yes, that was expected :)

I don't understand why this patch is so complicated.

The following cases seems to already be handled by default :
- cardinality > 1, all cases
- cardinality = 1, title enabled, all cases

We just need to handle
- cardinality = 1, title disabled, all cases

Please find attached a simple solution that fixes the issue (without tests, sorry).

The side effect of this fix is that, in the Menu UI, the MenuLinkContent 'link' base field description ('The location this menu link points to.') is appended to the field description. This leads to that help text displayed on the field : 'Start typing the title of a piece of content to select it. You can also enter an internal path such as /node/add or an external URL such as http://example.com. Enter to link to the front page.
The location this menu link points to.'

I think that the appended information improves the usability of the field so it might not be considered as a problem.

Let's see what the testbot have to say about that patch an discuss my tiny solution.

Better handling of empty descriptions (fixes tests)

Looks fine to me. Fixes the bug.

Can we add some quick tests, though? You can still base it on the "test-only" patch in #18 I guess.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -244,6 +244,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+        if (!empty($element['#description'])) {
+          if (empty($element['uri']['#description'])) {
+            $element['uri']['#description'] = $element['#description'];
+          }
+          else {
+            $element['uri']['#description'] .= '<br />' . $element['#description'];

It would be great to have some quick comment why we have this logic here.

Attached you can find a test only, which should fail, and a test + patch.

Test is the code from #18, added 1 verbose command to see which variant is being tested.

Patch is taken from #24, added comment as asked in #26

Thanks @StryKaizer for your work!

Tests pass.
Coding standards are OK.
And finally, labels are where they're supposed to be \o/

Before

After

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -244,6 +244,17 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+          else {
+            $element['uri']['#description'] .= '<br />' . $element['#description'];
+          }

There does not seem to be any test for this. Since adding html together is one of losing safeness with need to confirm that we're not introducing an escaping bug here.

There does not seem to be any test for this. Since adding html together is one of losing safeness with need to confirm that we're not introducing an escaping bug here.

+++ b/core/modules/link/src/Tests/LinkFieldUITest.php
@@ -67,31 +71,84 @@ function testFieldUI() {
+          // Also assert that the description we made is here, no matter what
+          // the cardinality or link setting.
+          $this->assertRaw($description);
+        }

I might misunderstand your comment, but this should cover the case you are asking for, no?

Granted, we do not test _not_ adding a custom description, so here is a quick patch adding that as well. Since you mention " Since adding html together is one of losing safeness with need to confirm that we're not introducing an escaping bug here." do you want to add an assertion that we are not seeing the
tag as text also? (for the record, it does not introduce an escaping bug, i just tested manually)

Manually tested. Looking good.

Re-read the comment in #30 and I think I might have misunderstood the concern, so I'll just add a quick test for that. Also, want to replace the random data in there (see #2571183: Deprecate random() usage in tests to avoid random test failures for more info)

- Replaced some random data.
- Fixed some lines that were 81 characters :p
- Added html in the descriptions, to test this does not get escaped (hope that was what you were after, @alexpott?)

I made a new round of manual testing and that still looks good!
Thank you @eiriksm.

Tested on simplytest with a link field on the article content type and editing a shortcut link.

Link field:

Shortcut:

Whenn I compare that shortcut screen with a Drupal 8 install without the patch the description "The location this shortcut points to." is not shown in the latter.

I *think* this is rtbc.

Committers want to look at #30, #31 and #37

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -243,6 +243,17 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+            $element['uri']['#description'] .= '<br />' . $element['#description'];

Why the <br /> tag?

Also it's not clear to me whether the test covers the case where the uri description and field description are concatenated.

Seems to have been added in #31.

To first comment on the <br>, it was first introduced in #22. I guess the reason was not explicitly defined, but I assume the main point is that we want to differentiate between "core help text" and "user entered help text". As an example, look at this image:

If the text "Here is my link help text" would be just after the "core help text", that would appear strange. In my opinion, anyway.

Would it be preferable if we added these in different render arrays, making them paragraphs instead of concatenating them with a <br>? Or was your point that you would prefer it to not be there, and instead be concatenated with no markup between?

Now, regarding test coverage for the above mentioned. It is not really tested that the concatenated text is on the page. It is, however, tested that both the user entered and the "core help text" is on the page. Although I guess an extra assertion would not hurt. :)

Let's just first decide how to proceed with the "<br> problem". Any thoughts from anyone there? Keep it? Remove it? Change it?

@eiriksm You're totally right. I introduced that <br> because I thought it was less confusing than without.
I don't know at all if it's better than paragraphs but I'm afraid that paragraphs default style would make it bad looking.

So do we style the <p>, add documentation to the <br> or just set it back to RTBC?

I'm for adding a comment in the PHP to explain why this was added.

I'm not de the right guy to write comments.
I can put it in the code and make the patch file but I'm not confident enough in english to write it :)

OK, here is an updated patch.

- Added a comment in both the test and the widget why we add the BR tag.
- Fixed a typo.
- Simplified the test code a little.
- Added assertions for when the BR tag should be added.

+++ b/core/modules/link/src/Tests/LinkFieldUITest.php
@@ -126,32 +131,24 @@ function testFieldUI() {
+              // If the link type was not internal, and the cardinalyty was 1,

typo: cardinality

Looks good to me other than that.

oops. Oh well, typo fixed.

I tested #48 on simplytest.
Things are appearing fine. Screenshots are attached.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -243,6 +243,21 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+            // If we have the description of the type of field together with
+            // the user provided description, we want to make a distinction
+            // between "core help text" and "user entered help text". To make
+            // this distinction more clear, we separate them with a line break.
+            $element['uri']['#description'] .= '<br />' . $element['#description'];

This is concatenating markup which will lose safe-ness. This works because #description is XSS admin filtered. Not sure what is the right thing to do here. Setting back to needs review to get more people than just me thinking about this.

How about this? An inline template for the description?

Looks good by testing on simplytest.me
Moving it to RTBC

+1 RTBC for the online template, good idea @eiriksm

Is the br tag necessary? Why not just a space?

@catch
As per me it will look good and add some more user friendliness.
For ex: See 2421001-51-1.png in #52

In Link 3 field, if we don't separate link description in new line it will be hard to identify link description and link help text. Same holds true for this snapshot as well.

@mohit that's what I'm saying though. If the description and help text are actually semantically different, then they should not be within the same HTML tag. The <br > tag is not visible to screen readers for example.

@catch
How about moving both to separate div like

$element['uri']['#description'] = [
              '#type' => 'inline_template',
              '#template' => '<div class="uri-core-description">{{ core_description }}</div><div class="uri-user-description">{{ user_description }}</div>',
              '#context' => [
                'core_description' => $element['uri']['#description'],
                'user_description' => $element['#description'],
              ],
            ];

https://api.drupal.org/api/drupal/core%21modules%21block%21src%21Control...

<div> are not read by screen readers. Most of the time, they don't vocalize <p> either (and they would need an extra layer of theming to avoid too big margins at this place).
The best option would be to add visually hidden labels to these elements but I'm not sure we can find something really relevant.

I also think we should put the user defined description first because screen readers users often skip content they already know. So, if that starts with the exact same words as everywhere else on the website, they'll tend to skip it entirely and miss the user entered part.

Nice work on getting tests going for this and resolving the regression.

Toying with link.module component vs maybe form api as this doesn't really touch the menu system. Please correct this if it's wrong.

I'm reviewing this to see if I can't get this back to RTBC.

I'd prefer to move this markup to the template(as always).

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -243,6 +243,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +        // By default the field description is added to the title field. Since
   +        // the title field is disabled, we add the description, if given, to the
   +        // uri element instead.
   

   Why do we add the description to the uri? Was it the only thing to add it to? Maybe there is a more appropriate place for this?

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -243,6 +243,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +            // between "core help text" and "user entered help text". To make
   +            // this distinction more clear, we separate them with a line break.
   

   This may be harder but it would be nice to get this variable into the template to make it easier to theme. Even if a break tag is the solution, it will make it much easier to override.(I hope)

Ping me on irc #drupal-contribute if you have any questions.

The core committers (@xjm, @alexpott, @catch, @effulgentsia, @Cottser) and theme maintainers (@lauriii, @joelpittet) discussed this issue today and agreed that this issue does not need to be major priority. However, we did accidentally remove the ability to add this text to these fields, and that can be considered a bug.

The screenshots in the summary explain the problem, but this issue would be easier to understand with specific steps to reproduce in the summary. (I see the issue has test coverage already in the patch, although I did not review the changes thoroughly.)

@joelpittet also noted that using an inline template and joining two strings with a <br /> tag are non-ideal (though the inline template is at least not introducing any escaping bugs, so it is an improvement over earlier patches for that reason.) If possible, we should use actual templates for markup, and/or more semantic markup. (For example, maybe it should be an unordered list?) It's worth considering whether this is the best way to ensure both description texts are available.

We should consider whether the issue might be better targeted for 8.2.x, because it is making a user-facing change and adding new UI text. (Some module could already be altering to add text back in addition, and this patch could result in that text being repeated twice, for example.) I've moved it to 8.2.x for now.

Finally, it looks like the feedback from @DuaelFr, @catch, and @joelpittet all still needs to be addressed as well.

Thanks everyone for your work so far on this bug!

Per #61, let's add the specific steps to reproduce to the summary.

Added steps to reproduce and improved remaining tasks.

Here is a new patch where I choosed to use an item_list to show both descriptions.

To anwser @joelpittet in #59.1: no, we sadly don't have any better option. In the case we are talking about, the only element we have is that 'uri' element. That's only when we get both uri and text that we have a surrounding container.

@YesCT thanks for the review :)
I wasn't able to find any usefull existing assertion to test strings order.
This new patch removes the part where we try to test the html structure of that description.

Green! \o/

Patch doesn't apply any more.

Assigning to me. I'll first make a patch with a straight reroll to latest 8.4.x, and then a second one to address the remarks from #72.

Straight reroll.

Setting to needs review for the bot. The tests have been ported to BrowserTestBase in the meantime so there might be subtle changes causing failures now.

Leaving assigned to me to review + address remarks from #65.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -239,6 +239,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +        // By default the field description is added to the title field. Since
   
   +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -52,9 +52,13 @@ function testFieldUI() {
   +    $this->fieldUIAddNewField($type_path, $field_name, $label, 'link', array(), $field_edit);
   

   We can use [] instead of array() to make this line more hipster.

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -62,31 +66,82 @@ function testFieldUI() {
   +            // Output variation being tested for debugging purpose.
   +            $this->verbose("Now testing - cardinality: $cardinality - link_type:
   +            $link_type, title_setting: $title_setting, description_enabled:
   +            $description_enabled");
   

   Let's remove the debugging cruft.

3. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -62,31 +66,82 @@ function testFieldUI() {
   +            // Log in an admin to set up a content type for this test.
   +            $this->drupalLogin($this->adminUser);
   +
   +            // Add a new content type.
   +            $type = $this->drupalCreateContentType();
   +            $type_path = 'admin/structure/types/manage/' . $type->id();
   +            $add_path = 'node/add/' . $type->id();
   ...
   +            // and link field settings.
   

   Relogging and creating content types are expensive operations. Since these actions are done in the inner loop of the test a lot of time is spent doing this. A bit later in the loop a new test user is also created and logged in for every iteration.

   I think we can save a lot of cycles if we only log in with the admin user once to create the test fields, and then relog once as the test user to verify the expected output.

   I also think creating one single content type with multiple fields would be sufficient. If we give the fields unique machine names we can check that the expected messages are associated with the expected fields.

1. Used the hipster array syntax. :)
2. Removed debugging cruft.
3. Refactored to only log in once as admin & once as user and only create one new content type. Had to remove the test for absence of certain text because, now that all the fields are on the same content type, the text is actually there (but I think the test is still valid).

(and removed Needs Re-roll tag)

Great improvement in speed!

[pieter@thinkarch core ((b2c3a31...) $%)]$ SIMPLETEST_BASE_URL=http://drupal.dev/drupal ../vendor/bin/phpunit modules/link/tests/src/Functional/LinkFieldUITest.php 
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

.

Time: 1.59 minutes, Memory: 4.00Mb

OK (1 test, 176 assertions)
[pieter@thinkarch core (2421001 $%)]$ git checkout 2421001-80 
Switched to branch '2421001-80'
[pieter@thinkarch core (2421001-80 $%)]$ SIMPLETEST_BASE_URL=http://drupal.dev/drupal ../vendor/bin/phpunit modules/link/tests/src/Functional/LinkFieldUITest.php 
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

.

Time: 1.03 minutes, Memory: 4.00Mb

OK (1 test, 38 assertions)

From 1.59 minutes to 1.03 minutes, it's almost twice as fast!

We can probably shave off some more seconds if we create the fields using the field API instead of through the interface. Since we are not actually testing the field UI there's no need to use the UI for this.

Refactored to only log in once as admin & once as user and only create one new content type. Had to remove the test for absence of certain text because, now that all the fields are on the same content type, the text is actually there (but I think the test is still valid).

I think we can keep this part of the test if we only look inside the field container for checking the absence of the text, instead of checking the entire response page.

Used the field API to create the test fields. I also restored the tests for the absence of non-configured help texts by using XPath to pinpoint exact fields in the document.

[pieter@thinkarch core (2421001 $%)]$ SIMPLETEST_BASE_URL=http://drupal.dev/drupal ../vendor/bin/phpunit modules/link/tests/src/Functional/LinkFieldUITest.php 
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

.

Time: 31.02 seconds, Memory: 4.00Mb

OK (1 test, 65 assertions)

The test runs again almost twice as fast, now down to 31 seconds from the original 1 minute 59 seconds!

And here is the patch :D

Rerolled against latest HEAD.

Having the description appear after the link text field is a bit confusing IMHO. It is confusing because it appears like it's describing the link text as opposed to the URL itself. I would suggest that it appear after the URL field, before the drupal-supplied text about the requirements (internal/external link). Here is a mock-up of what I'm suggesting:

Reroll

I've checked used the reproduction instructions to test geertvd's patch from comment #87.

Before:

After:

The help text has been added to the field's "help" area.

I'll go through the other variants in the summary before changing the status.

OK. I've run through the the variants mentioned in the summary. Get ready for a load of screenshots.

	Before	After
Menu
Shortcut
File field Cardinality: 1 Link text: disabled Url type: internal
File field Cardinality: 1 Link text: disabled Url type: external
File field Cardinality: 1 Link text: disabled Url type: both
File field Cardinality: 1 Link text: required Url type: internal
File field Cardinality: 1 Link text: required Url type: external
File field Cardinality: 1 Link text: required Url type: both
File field Cardinality: Unlimited Link text: disabled Url type: internal
File field Cardinality: Unlimited Link text: disabled Url type: external
File field Cardinality: Unlimited Link text: disabled Url type: both
File field Cardinality: Unlimited Link text: required Url type: internal
File field Cardinality: Unlimited Link text: required Url type: external
File field Cardinality: Unlimited Link text: required Url type: both

The help text is now displayed on menu and shortcut link fields as well as link fields where the cardinality is 1 and the link text option is disabled.

It appears that the problem doesn't exist on other setting combinations on link field.

I've also looked at the code. All appears to be OK.

I've now changed the status to RTBC.

Closed #2839940: Help Text does not override Link text 'This must be ...' as a duplicate issue.

I skeptically tested with an unordered list within the help text, but that actually looks ok.

Closing duplicate.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -241,6 +241,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+                // User description first to avoid screen reader users to skip.

Minor but this comment is a bit cryptic/terse to me, can this be expanded upon, please?

(leaving at RTBC)

Edit: Also minor, there is one coding standards violation, missing space after foreach: https://www.drupal.org/pift-ci-job/728472

That comment isn't particularly relevant to screen readers IMO - I think it's more accurate to say the custom help text is the most relevant part for everyone.

The link URL input has an aria-describedby attribute, which points to the div.class wrapper around the entire list of descriptions. That is, the list constitutes a single description as far as assistive tech is concerned.

This patch addresses #96 by:

• updating the comment about why user-specified description comes first
• fixing the missing space after foreach keyword.

Returning to RTBC glory

Putting back to 8.4.x due to it being a bug fix.

#97 no longer applies because #2864088: Convert web tests to browser tests for shortcut module was commited, so I just rerolled the patch.

I finally had a chance to sit down and review this patch. I was a bit hesitant about the proposed resolution, but after more than a month I've not thought of a better way to do this, and it definitely resolves the issue. The presentation to the user is also fine; it was just the bit of mucking with the semantics of the element that gave me pause. And @andrewmacpherson has already reviewed and it sounds like we are okay on the accessibility front now.

The very thorough manual testing demonstrates how well the fix works; thank you so much for providing the screenshots.

There's one thing I think we need to change with the tests (which are really thorough which is great to see).

+++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
@@ -62,31 +69,152 @@ public function testFieldUI() {
+    // Test the displaying of help texts for the link field.
+    $test_cases = [];
+
+    // There are many combinations of field settings: where the description
+    // should show: variation on internal, external, both; cardinality (where
+    // the fieldset is hidden or used); and link text shown (required or
+    // optional) or disabled. There are two descriptions: field and URL help
+    // text.
+    $cardinalities = [1, 2];
+    $title_settings = [
+      DRUPAL_DISABLED,
+      DRUPAL_OPTIONAL,
+    ];
+    $link_types = [
+      LinkItemInterface::LINK_EXTERNAL,
+      LinkItemInterface::LINK_GENERIC,
+      LinkItemInterface::LINK_INTERNAL,
+    ];
+
+    // Test all variations of link types on all cardinalities.
+    foreach ($cardinalities as $cardinality) {
+      foreach ($link_types as $link_type) {
+        // Now, test this with both the title enabled and disabled.
+        foreach ($title_settings as $title_setting) {
+          // Test both empty and non-empty labels.
+          foreach ([TRUE, FALSE] as $label_provided) {

So, this sort of thing is what data providers are made for. The good news is that LinkFieldUITest is already a BTB test, so we can use one! So I think we can refactor the test to use a data provider (really they're the first thing that got me excited about PHPUnit a couple years ago), and then we are good to go.

This isn't actually a string change -- the strings are already defined -- it's just a small UI change and a change to a render array. Typically those changes are minor-only, so the branch change in #99 isn't really correct.

However, since it's a clear UX and a11y bugfix with the only disruption being to the link element's render array, I feel comfortable backporting this during the release candidate phase if needed.

Very sorry for the long wait time; I know this one didn't get the feedback it deserved during the alpha and beta phases.

Ok, new patch using a Data Provider, but I have doubts:

1.     // With the old patch: 
       Testing Drupal\Tests\link\Functional\LinkFieldUITest
       .
       Time: 14.7 seconds, Memory: 8.00MB
   
        OK (1 test, 65 assertions)
   
        // With the new patch:
        Testing Drupal\Tests\link\Functional\LinkFieldUITest
        ........................
        Time: 3.95 minutes, Memory: 8.00MB
   
        OK (24 tests, 352 assertions)
     

   Using the data provider executes the whole test per test_case, making it way slower than before, In my computer it was ~15 seconds with the previous patch and now it takes ~4 minutes, is this expected and ok?

2.     Testing Drupal\Tests\link\Functional\LinkFieldUITest
       F
   
       Time: 3.38 seconds, Memory: 8.00MB
   
       There was 1 failure:
   
        1) Warning
        The data provider specified for Drupal\Tests\link\Functional\LinkFieldUITest::testFieldUI is invalid.
        Use of undefined constant DRUPAL_DISABLED - assumed 'DRUPAL_DISABLED'
       

   It seems that the data providers are executed before to include all the files, so it fails whenever I use a constant and that is why in my patch I didn't use them in the data provider, this makes me wonder if this is ok or is there a good way to use constants in a Data Provider?

- David

I have tested the patch (cleared cache etc) but the problem with it is both the default help text and manually entered help text are visible now. Attaching screenshots.

@Sinan ErdemThank you for your review!
The thing is that showing the default help text is needed because we cannot ask the site builder to know what this text is and to include its content in they own help text. What you are describing is precisely what we are trying to introduce with this patch, so it's perfectly working ;)

I didnt know that, sorry.

Anyway, IMO, the help text should be easily overridable, because this default help text aims mostly site builders. Normal users would need better, site-specific descriptions. In my case, I want to write that "The link should resemble https://www.linkedin.com/in/YOURNAME" which already contains the information that the link is external.

@gnuget yeah setup() is run for each item in the dataprovider, so doing this in a BTB test takes a HUGE hit on speed. So unless you need a fresh install for each item, it's probably better not to use it.

But I agree with xjm that it's a great pattern to use, but how about we sorta follow that pattern but skip the setup? Patch does something like that.

Thanks for your help, Lendude.

I just restored the constants in this patch and removed an unused variable.

I agree it's not a good idea to use data providers in functional tests due to the enormous slowdown they cause. In unit tests this is not a problem, and also not in kernel tests if there is only little data to test, but if we are doing functional tests with a lot of data we shouldn't use them. Adding multiple minutes to every single test run on the bot is a poor tradeoff for the luxury of being able to use this new feature.

It is an accepted pattern in core to run multiple slow tests in a single setup without data providers to speed up the test runs, see for example EntityTranslationTest and MenuRouterTest.

Reviewed the patch, this is getting very close to being perfect again, found some nitpicks:

1. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -31,6 +35,27 @@ class LinkFieldUITest extends BrowserTestBase {
   +  /**
   +   * A user that should see the help texts.
   +   *
   +   * @var \Drupal\user\Entity\User
   +   */
   +  protected $helpTextUser;
   

   Maybe move this property underneath the $adminUser property to keep them together.

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -39,22 +64,94 @@ protected function setUp() {
        $this->drupalLogin($this->adminUser);
   

   This call to $this->drupalLogin() is not needed, at the start of the test the user is already logged in.

3. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -39,22 +64,94 @@ protected function setUp() {
   +    $this->helpTextUser = $this->drupalCreateUser(['create ' . $this->secondContentType->id() . ' content']);
   

   Very minor remark, I would move this up a few lines so it sits underneath the other user that is created.

4. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -39,22 +64,94 @@ protected function setUp() {
   +            yield [
   +              $cardinality,
   +              $link_type,
   +              $title_setting,
   +              $label_provided ? $label : '',
   +            ];
   

   This yield is very cool!

5. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -39,22 +64,94 @@ protected function setUp() {
   +   * @param int $cardinality
   +   * @param $link_type
   +   * @param int $title
   +   * @param string $label
   

   This documentation is incomplete. The descriptions are missing, as well as the type hint of the second argument.

6. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -62,31 +159,94 @@ public function testFieldUI() {
   +    // Also assert that the description we made is here, no matter what the
   +    // cardinality or link setting.
   

   I would rewrite this "no matter what" to "regardless of"

New patch, this should address all the points mentioned in #111

I just have a doubt about the $link_type type hint, that argument is an hexadecimal number (see LinkItemInterface) but accord with the Drupal's documentation

Data types can be primitive types (int, string, etc.), complex PHP built-in types (array, object, resource), or PHP classes.

But a hexadecimal number is not a primitive type accord with the php's documentation.

And I think is not a PHP built-in type nor a PHP class... so I'm not so sure what put there, for now, I put int, I can change it if necessary.

patch attached.

Thanks for the reviews!

Patch looks perfect now, thanks for the quick fix!

But a hexadecimal number is not a primitive type accord with the php's documentation.

And I think is not a PHP built-in type nor a PHP class... so I'm not so sure what put there, for now, I put int, I can change it if necessary.

That's completely correct!

An integer is any "whole" number without a fraction. It doesn't matter in which numeric system it has been defined. When a variable is assigned an integer (or a boolean) PHP stores it internally as a "long" data type, inside a "zval" object. It doesn't matter whether the original value was decimal, hexadecimal, binary or octal. In the end it's just a number that is stored in 64 bits of memory.

Here is how you can see how PHP handles hexadecimal values:

$ php -a
Interactive shell

php > var_dump(gettype(0x0f));
string(7) "integer"

php > var_dump(0x08 === 8);
bool(true)

If you are interested in how PHP deals with numbers I can highly recommend this blog post: Internal value representation in PHP 7.

Bummer about the test run time. I thought browser tests were significantly faster than web tests... oh well.

1. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -45,16 +72,89 @@ protected function setUp() {
   -    $label = $this->randomMachineName();
   -    $field_name = Unicode::strtolower($label);
   -    $this->fieldUIAddNewField($type_path, $field_name, $label, 'link');
   +    $field_label = $this->randomMachineName();
   
   @@ -62,31 +162,94 @@ public function testFieldUI() {
   +      $this->assertNoFieldContainsRawText($field_name, $expected_help_texts[LinkItemInterface::LINK_EXTERNAL]);
   +      $this->assertNoFieldContainsRawText($field_name, $expected_help_texts[LinkItemInterface::LINK_GENERIC]);
   

   So, the randomMachineName() exists in HEAD. However, we're now adding an assertNoSomething(). That's a combination that can cause random test failures when the random-thing matches the assert-no-thing.

   In this case, none of the texts we're asserting the absence of are eight characters long, so I think it's okay for this to go in as-is, and I don't want to delay this issue further since I already did so with the data provider distraction. However, for future reference, we should be avoid random usage because of the risk of random failures. Edit: That's discussed in #2571183: Deprecate random() usage in tests to avoid random test failures. However, another concern related to that...

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -62,31 +162,94 @@ public function testFieldUI() {
   +  protected function assertFieldContainsRawText($field_name, $text) {
   +    $this->assertTrue((bool) preg_match('/' . preg_quote($text, '/') . '/ui', $this->getFieldHtml($field_name)));
   +  }
   ...
   +  protected function assertNoFieldContainsRawText($field_name, $text) {
   +    $this->assertFalse((bool) preg_match('/' . preg_quote($text, '/') . '/ui', $this->getFieldHtml($field_name)));
   +  }
   ...
   +  protected function getFieldHtml($field_name) {
   +    $css_id = Html::cleanCssIdentifier("edit-$field_name-wrapper");
   +    return $this->xpath("//div[@id='$css_id']")[0]->getHtml();
      }
   

   This is a whole lot of abstraction, plus an xpath, plus coupling the test to CSS and markup that may change in the future. It's also fragile... presumably these texts should exist nowhere on the page, but if we change the markup and use the negative assertions so specifically tied to xpath, they will give false positives if/when the markup changes in the future.

   It looks like this was added in #82 by @pfrenssen:

   Used the field API to create the test fields. I also restored the tests for the absence of non-configured help texts by using XPath to pinpoint exact fields in the document.

   But that's not clear to me why it merits all the extra code nor the risk of false positives in the future.

This issue does not need further manual testing unless the solution also changes; all the recent changes are only to automated tests. :) Thanks!

I think testing both the presence and absence of the different messages is worthwhile since we have options to show them or not. If we only test if they are present when they should then we might have a bug in the future where the wrong messages are showing up and we won't know about it.

I agree fully on the random naming, but matching on the field wrappers is fine I think. The base field templates are not likely to change in core because it would break the custom theming and JS of all websites that use forms. The Form system itself uses XPath and the "edit-" prefix in their tests to identify fields and field wrappers, so we should be pretty OK. See for example the tests in \Drupal\Tests\system\Functional\Form\ElementTest, they are very similar.

Would you be OK if we hardcode unique field names in the test instead of using random names? I think this would also mitigate the risk of tests breaking due to collisions with CSS IDs that may be added in the markup in the future.

Updated the patch to use predictable machine names, to reduce the chance of random failures. Also updated the XPath call to use an argument for the dynamic part.

I just had a final look at the interdiff and I noticed that we are testing if the field is contained in a <div> element. I somehow missed this before. This is brittle and likely to change to a more specific HTML tag in the future. We should only check the ID.

This looks RTBC to me.

Does it need something else?

Thanks for your help pfrenssen.

The patch from #118 looks RTBC'able to me. After an extensive review, I have two nitpicks, fixed in the attached patch, though:

1. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -45,16 +71,91 @@ protected function setUp() {
   +    $field_label = $this->randomMachineName();
   

   As @xjm told above, we should not use random strings to avoid random failures. As I had to work on the patch anyway (see below), I changed this to a predictable string.

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
   @@ -62,31 +163,94 @@ public function testFieldUI() {
   +    if (!empty($test_case['label'])) {
   +      $this->assertFieldContainsRawText($field_name, $test_case['label']);
   +    }
   

   $test_case is never ever set so I removed this dead code to avoid confusion.

If the bot goes green again, one should mark this issue RTBC, I think.

Thanks!

Reviewed the patch and even more so the discussion above, which was plentiful to say the least. This issue is a whos-who of Drupal :) All the concerns raised recently have been resolved and this approach had signoff and positive feedback from subsystem maintainers above as well.

I found this little issue with the latest changes though:

+++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
@@ -208,11 +208,6 @@ public function runFieldUIItem($cardinality, $link_type, $title, $label, $field_
     }
-    // Also assert that the description we made is here, regardless of the
-    // cardinality or link setting.
-    if (!empty($test_case['label'])) {
-      $this->assertFieldContainsRawText($field_name, $test_case['label']);
-    }

This seems to be a refactoring artifact, the method does have a $label argument and the method is invoked on multiple test cases. So this was meant to be $label apparently.

Thanks a lot, Gábor.

New patch attached.

@Gábor Hojtsy nice catch!

Feedback has been addressed, back to RTBC.

I did not identify this before, failed as I wanted to commit it. Fixed before commit.

PHPCS: core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php passed

FILE: ...mmits/core/modules/link/tests/src/Functional/LinkFieldUITest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 74 | ERROR | [x] Expected 1 space after FOREACH keyword; 0 found
----------------------------------------------------------------------

Thanks all for working on this and trying different approaches!

@Gábor Hojtsy Why is this reverted on 8.4?