Problem/Motivation

Noticed in #2416987: Fix UI regression in the menu link form
there is help text for when a link can be both internal and external.
there is text prefixed when internal only
but there is no help text for external only.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because help text is missing.
Issue priority Normal because it is isolated to just link widgets.
Prioritized changes Prioritized because it is a bug fix.
Disruption Not disruptive. Adds a translatable string (those are unfrozen anyway), causes no BC breaks.

Allowed because it is a prioritized change and impact is greater than disruption.

Proposed resolution

add help text to the uri form element for the link widget when it is external only

Here are the screenshot for issue:

This is the image for the field options


And the before image

and the after image

CommentFileSizeAuthor
#66 interdiff.2421021.62.66.txt1.59 KBYesCT
#66 2421021.66.patch4.58 KBYesCT
#66 2421021.66.testsonly.patch3.39 KBYesCT
#62 interdiff.2421021.60.61.txt2.46 KBYesCT
#62 2421021.61.patch4.7 KBYesCT
#62 2421021.61.testsonly.patch3.51 KBYesCT
#60 interdiff.2421021.57.60.txt855 bytesYesCT
#60 2421021.60.patch4.01 KBYesCT
#60 2421021.60.testsonly.patch2.83 KBYesCT
#57 interdiff.2421021.56.57.txt3.1 KBYesCT
#57 2421021.57.patch3.83 KBYesCT
#57 2421021.57.testsonly.patch2.64 KBYesCT
#56 interdiff.2421021.54.56.txt430 bytesYesCT
#56 2421021.56.patch3.58 KBYesCT
#54 interdiff.2421021.46.54.txt0 bytesYesCT
#54 2421021.54.patch3.4 KBYesCT
#46 interdiff.2421021.40.46.txt1.58 KBYesCT
#46 2421021.46.patch4.17 KBYesCT
#46 2421021.46.tests-only.patch2.98 KBYesCT
#43 2421021.40.patch4.72 KBYesCT
#41 2421021.41.patch4.86 KBmartin107
#41 interdiff-40-41.txt585 bytesmartin107
#40 interdiff-38-40.txt586 bytesmartin107
#40 2421021.40.patch4.72 KBmartin107
#38 interdiff.2421021.36.38.txt2.79 KBYesCT
#38 2421021.38.patch4.15 KBYesCT
#38 2421021.38.tests-only.patch2.97 KBYesCT
#1 externalonly-2421021.1.patch1.03 KBYesCT
#4 externalonly-2421021-4.patch1.03 KBshashikant_chauhan
#5 2421021-5.patch1.02 KBamateescu
#9 2421021-6.patch1.61 KBlhuacho
#11 Options.png7.33 KBtremix
#11 Before.png7.35 KBtremix
#11 After.png45.78 KBtremix
#12 2421021-5.1.patch1.09 KBlhuacho
#12 2421021-6.patch1.61 KBlhuacho
#12 interdiff-2421021-5-6.txt1.79 KBlhuacho
#14 externalonly-2421021-14.patch1.19 KBshashikant_chauhan
#14 interdiff-6-14.txt722 bytesshashikant_chauhan
#19 externalonly-2421021-19.patch1.19 KBYesCT
#19 interdiff.2421021.14.19.txt1.22 KBYesCT
#20 before-external.png198.74 KBYesCT
#20 external-with.png222.47 KBYesCT
#25 2421021.25.tests-only.patch1.57 KBYesCT
#27 2421021.27.tests-only.patch1.79 KBYesCT
#27 interdiff.2421021.25.27.txt986 bytesYesCT
#29 2421021.29.tests-only.patch2.92 KBYesCT
#29 interdiff.2421021.27.29.txt2.46 KBYesCT
#30 2421021.30.tests-only.patch2.98 KBYesCT
#30 interdiff.2421021.29.30.txt810 bytesYesCT
#36 2421021.36.tests-only.patch2.87 KBYesCT
#36 2421021-36.patch4.06 KBYesCT
#36 interdiff.2421021.30.36.txt1.87 KBYesCT
#36 html_input_name.png317.52 KBYesCT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Status: Active » Needs review
FileSize
1.03 KB

Status: Needs review » Needs work

The last submitted patch, 1: externalonly-2421021.1.patch, failed testing.

YesCT’s picture

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -144,6 +144,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      $element['uri']['#description'] .= $this->t('This must be an external URL such as %drupal.', array('%drupal' => 'http://example.com'));

oops. I forgot to take out the concatenations.

.=
should be just
=

shashikant_chauhan’s picture

Status: Needs work » Needs review
FileSize
1.03 KB

Removed the concatenations

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.02 KB

The patch looks mostly ok to me. Just changed that %drupal parameter to %url :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2421021-5.patch, failed testing.

Status: Needs work » Needs review

amateescu queued 5: 2421021-5.patch for re-testing.

doakym’s picture

Issue tags: +LatinAmerica2015

Path #6 fail because head was broken.

I see that this change affects the UI so i im goin to take before an after screenshots.

lhuacho’s picture

FileSize
1.61 KB

This a reroll from patch5.
File LinkWidget.php produce an error and doesnt apply cleanly.

also, with patch 5 the internal format link does not work, only external format link shows help text.

This patch fixes the offset issue and the internal link help text.

doakym’s picture

Patch #6 passed simples test, now we need screenshots.

tremix’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
7.33 KB
7.35 KB
45.78 KB

I tested this also in a my local environment, we created a content type "Link" and we added fields with the three options: internal, external and both internal and external links allowed. Before the patch the help text is not shown, after the patch all three help texts are shown.

lhuacho’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.09 KB
1.61 KB
1.79 KB

We've realized that didn't upload an interdiff. So here is the number #5 patch rerolled and the interdiff along with the #6 patch again just for the testbot

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -220,12 +220,18 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      $element['uri']['#description'] = $this->t('This must be an internal URL such as %url.', array('%url' => '/node/nid'));

We do not need this. Because the UI is clear.
The text before the form makes it clear it is an internal URL, since it is the site prefix there.

We do not want to have helptext which does not add any information.

shashikant_chauhan’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
722 bytes

Removed internal url description.

Status: Needs review » Needs work

The last submitted patch, 14: externalonly-2421021-14.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

(bot hiccup)

YesCT’s picture

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -227,6 +227,11 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    // If the field is configured to allow only external links,
+    // show a useful description.

I'll fix this right now.

"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below)."

https://www.drupal.org/node/1354#drupal

YesCT’s picture

YesCT’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
198.74 KB
222.47 KB

updated screenshots.
also beta summary evaluation.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great :)

alimac’s picture

I read the patch and it does this issue calls for: add help text for external link only.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Awesomesauce.

Now let's get a test so we don't ever break this again.

YesCT’s picture

ok. I'm working on one now.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

just partial work. will fail.... the wrong way.

Status: Needs review » Needs work

The last submitted patch, 25: 2421021.25.tests-only.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
986 bytes

better. has a user with permission to create a node of the made content type.
this one passes. (but it checks for the help text for when both internal and external are allowed, which was correct in head). so that is ok.

next, make the test add link field that is external only, and test for the new help text for that.

Status: Needs review » Needs work

The last submitted patch, 27: 2421021.27.tests-only.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
2.46 KB

oh, that breadcrumb thing.

I'll fix that eventually.

This is trying to make a link that is external only.
but I dont know how to set the $field_edit array correctly.... so it is just the same, both internal and external. :(

YesCT’s picture

for the breadcrumb error.

The last submitted patch, 29: 2421021.29.tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: 2421021.30.tests-only.patch, failed testing.

YesCT’s picture

I'm done for the day. Someone else can try and set the link type to external if they want to.
Note, LinkFieldTest looks like it might, but does not use the fieldUIAddNewField() on fieldUiTestTrait that I was hoping to use here.

YesCT’s picture

+++ b/core/modules/link/src/Tests/LinkFieldUiHelptextTest.php
@@ -0,0 +1,86 @@
+    $field_edit = array(
+      'settings' => array(
+        'link_type' => LinkItemInterface::LINK_EXTERNAL,
+      ),
+    );
+    $this->fieldUIAddNewField($type_path, $field_name, $label, 'link', array(), $field_edit);

this is the bit that I think is wrong.

jibran’s picture

+++ b/core/modules/link/src/Tests/LinkFieldUiHelptextTest.php
@@ -0,0 +1,86 @@
+    $field_edit = array(
+      'settings' => array(
+        'link_type' => LinkItemInterface::LINK_EXTERNAL,
+      ),

This should be ['field[settings][link_type]' => LinkItemInterface::LINK_EXTERNAL]

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
317.52 KB
1.87 KB
4.06 KB
2.87 KB

so, @jibran explained to me that this must be the name of the input field (not a nested array of whatevers).

admin/structure/types/manage/article/fields/node.article.field_another_link

The last submitted patch, 36: 2421021.36.tests-only.patch, failed testing.

YesCT’s picture

fixed some nits. (help text is two words, and clarified some comments)

ok, I think this is ready for review.

The last submitted patch, 38: 2421021.38.tests-only.patch, failed testing.

martin107’s picture

FileSize
4.72 KB
586 bytes

Fix and test look good to me.

To someone on a deadline, a consistency fix like this going to potentially save so much pain - This issue is worth its weight in gold.

I am correcting two small nits.

martin107’s picture

FileSize
585 bytes
4.86 KB

Sorry one more thing ....

In the past I have found reading regression test @see annotations tags useful in cases like this.

YesCT’s picture

Thanks! #40 is good.

I dont think we need #41.
We dont reference issues when we fix things, we just @todo issues that are still open that need to yet be done.
(We can use git blame to find out which issue added or changed a test.)

And, function one line summaries standard says:
starts with third person verb and is less than 80 chars.

https://www.drupal.org/node/1354#functions

YesCT’s picture

FileSize
4.72 KB

re-uploading #40 (so it is the most recent patch, for clarity and also in case this gets rtbc, the testbot retests the most recent patch)

martin107’s picture

Fair enough.

YesCT’s picture

Status: Needs review » Needs work

oh, crumbs. I missed that #40 changed LinkFieldUITest, and this issue is not about that class, but adding a new one for helptext tests.

but, the functions in the help text tests should be public.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
4.17 KB
1.58 KB

The last submitted patch, 46: 2421021.46.tests-only.patch, failed testing.

filijonka’s picture

Status: Needs review » Reviewed & tested by the community

this looks ok for me

amateescu’s picture

Can we add these new tests to the existing Drupal\link\Tests\LinkFieldUITest class?

webchick’s picture

Status: Reviewed & tested by the community » Needs review
YesCT’s picture

@amateescu well, I'm planning to add a bunch more in #2421001: Fix regression in the link widget where help text does not show
for the various cardenality combinations and also something for the internal hint that is before the input field.
So, I was thinking having this separate group would make sense.

When we have a bunch, also, I was thinking they might have their own setUp to take care of some of the duplicate code.

but I can add them to LinkFieldUITest here, and move them later if we decide to.

amateescu’s picture

These UI tests are usually called integration tests so I would recommend to rename the current class to LinkFieldIntegrationTest and, yes, stick everything related to UI tests in it, even the bunch you plan on adding in #2421001: Fix regression in the link widget where help text does not show.

We also have to keep in mind that these "web tests" are not cheap, they're actually very expensive and slow to run, and every test*() method is doing a fresh Drupal installation. You can also think of it like: why would I do a new Drupal installation just to change a setting in Field UI and go another page to test it? :)

YesCT’s picture

ok. I will do that now.

YesCT’s picture

just moving the tests to the other file.

another patch coming.

Status: Needs review » Needs work

The last submitted patch, 54: 2421021.54.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
430 bytes

oops. forgot the use that I need in the new part of specifying the link type.

YesCT’s picture

this puts the tests into one test, taking into account @amateescu feedback in #52 about not wanting a new install for each test.
This is now ready for a full review. (if it is green)

Status: Needs review » Needs work

The last submitted patch, 57: 2421021.57.patch, failed testing.

The last submitted patch, 57: 2421021.57.testsonly.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
4.01 KB
855 bytes

this fixes the fails. logs in as an new admin to make the second content type/field.

I'm not sure though if I should save the previous admin and user and log in as them again.

The last submitted patch, 60: 2421021.60.testsonly.patch, failed testing.

YesCT’s picture

this makes and admin and content user and saves them for reuse.

The last submitted patch, 62: 2421021.61.testsonly.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 62: 2421021.61.patch, failed testing.

YesCT’s picture

oh, right. cause the permission is per content type. ....

YesCT’s picture

YesCT’s picture

Status: Needs work » Needs review

The last submitted patch, 66: 2421021.66.testsonly.patch, failed testing.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent, thanks a lot. Really happy to see this fixed.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed d16f1db on 8.0.x
    Issue #2421021 by YesCT, lhuacho, martin107, shashikant_chauhan,...
YesCT’s picture

thanks!

now on to #2421001: Fix regression in the link widget where help text does not show which should add tests for all the cases.

Status: Fixed » Closed (fixed)

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