Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
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
Comment | File | Size | Author |
---|---|---|---|
#66 | interdiff.2421021.62.66.txt | 1.59 KB | YesCT |
#66 | 2421021.66.patch | 4.58 KB | YesCT |
#66 | 2421021.66.testsonly.patch | 3.39 KB | YesCT |
#62 | interdiff.2421021.60.61.txt | 2.46 KB | YesCT |
#62 | 2421021.61.patch | 4.7 KB | YesCT |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #3
YesCT CreditAttribution: YesCT commentedoops. I forgot to take out the concatenations.
.=
should be just
=
Comment #4
shashikant_chauhan CreditAttribution: shashikant_chauhan commentedRemoved the concatenations
Comment #5
amateescu CreditAttribution: amateescu commentedThe patch looks mostly ok to me. Just changed that %drupal parameter to %url :)
Comment #8
doakym CreditAttribution: doakym commentedPath #6 fail because head was broken.
I see that this change affects the UI so i im goin to take before an after screenshots.
Comment #9
lhuacho CreditAttribution: lhuacho commentedThis 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.
Comment #10
doakym CreditAttribution: doakym commentedPatch #6 passed simples test, now we need screenshots.
Comment #11
tremix CreditAttribution: tremix commentedI 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.
Comment #12
lhuacho CreditAttribution: lhuacho commentedWe'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
Comment #13
YesCT CreditAttribution: YesCT commentedWe 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.
Comment #14
shashikant_chauhan CreditAttribution: shashikant_chauhan commentedRemoved internal url description.
Comment #17
YesCT CreditAttribution: YesCT commented(bot hiccup)
Comment #18
YesCT CreditAttribution: YesCT commentedI'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
Comment #19
YesCT CreditAttribution: YesCT commentedComment #20
YesCT CreditAttribution: YesCT commentedupdated screenshots.
also beta summary evaluation.
Comment #21
amateescu CreditAttribution: amateescu commentedLooks great :)
Comment #22
alimac CreditAttribution: alimac commentedI read the patch and it does this issue calls for: add help text for external link only.
Comment #23
webchickAwesomesauce.
Now let's get a test so we don't ever break this again.
Comment #24
YesCT CreditAttribution: YesCT commentedok. I'm working on one now.
Comment #25
YesCT CreditAttribution: YesCT commentedjust partial work. will fail.... the wrong way.
Comment #27
YesCT CreditAttribution: YesCT commentedbetter. 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.
Comment #29
YesCT CreditAttribution: YesCT commentedoh, 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. :(
Comment #30
YesCT CreditAttribution: YesCT commentedfor the breadcrumb error.
Comment #33
YesCT CreditAttribution: YesCT commentedI'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.
Comment #34
YesCT CreditAttribution: YesCT commentedthis is the bit that I think is wrong.
Comment #35
jibranThis should be
['field[settings][link_type]' => LinkItemInterface::LINK_EXTERNAL]
Comment #36
YesCT CreditAttribution: YesCT commentedso, @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
Comment #38
YesCT CreditAttribution: YesCT commentedfixed some nits. (help text is two words, and clarified some comments)
ok, I think this is ready for review.
Comment #40
martin107 CreditAttribution: martin107 commentedFix 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.
Comment #41
martin107 CreditAttribution: martin107 commentedSorry one more thing ....
In the past I have found reading regression test @see annotations tags useful in cases like this.
Comment #42
YesCT CreditAttribution: YesCT commentedThanks! #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
Comment #43
YesCT CreditAttribution: YesCT commentedre-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)
Comment #44
martin107 CreditAttribution: martin107 commentedFair enough.
Comment #45
YesCT CreditAttribution: YesCT commentedoh, 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.
Comment #46
YesCT CreditAttribution: YesCT commentedComment #48
filijonka CreditAttribution: filijonka commentedthis looks ok for me
Comment #49
amateescu CreditAttribution: amateescu commentedCan we add these new tests to the existing
Drupal\link\Tests\LinkFieldUITest
class?Comment #50
webchickComment #51
YesCT CreditAttribution: YesCT commented@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.
Comment #52
amateescu CreditAttribution: amateescu commentedThese 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? :)Comment #53
YesCT CreditAttribution: YesCT commentedok. I will do that now.
Comment #54
YesCT CreditAttribution: YesCT commentedjust moving the tests to the other file.
another patch coming.
Comment #56
YesCT CreditAttribution: YesCT commentedoops. forgot the use that I need in the new part of specifying the link type.
Comment #57
YesCT CreditAttribution: YesCT commentedthis 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)
Comment #60
YesCT CreditAttribution: YesCT commentedthis 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.
Comment #62
YesCT CreditAttribution: YesCT commentedthis makes and admin and content user and saves them for reuse.
Comment #65
YesCT CreditAttribution: YesCT commentedoh, right. cause the permission is per content type. ....
Comment #66
YesCT CreditAttribution: YesCT commentedComment #67
YesCT CreditAttribution: YesCT commentedComment #69
neclimdulLooks good to me.
Comment #70
webchickExcellent, thanks a lot. Really happy to see this fixed.
Committed and pushed to 8.0.x. Thanks!
Comment #72
YesCT CreditAttribution: YesCT commentedthanks!
now on to #2421001: Fix regression in the link widget where help text does not show which should add tests for all the cases.