Problem/Motivation
The link text of the link field is not marked as being required, although the field is marked as required and the link text as well.
Proposed resolution
- Make the field display the asterisk for required field, when needed.
- Only in the case of the
URL
is not required, butLink text
is required we need to add therequired
class and mark via JS only. This is to avoid this kind of issue for users with JS disabled: #2634232: In case of only 'link text' is required, then remove the required mark if the user deletes the text entered for URL - For the case in which URL and
Link text
are both required then it must be added in the regular way, without JS.
Add tests for:
URL required: false; Link Text required: true*
get entity_test/add
check the asterisk mark (it should not exist for both)
post the form
check the asterisk mark (it should exist only for link text)
* in this case the form element should not be marked as required, except via JS. @see #57
URL required: true; Link Text required: false
get entity_test/add
check the asterisk mark (it should exist only for URL)
post the form
check the asterisk mark (it should exist only for URL)
URL required: true; Link Text required: true
get entity_test/add
check the asterisk mark (it should exist for both)
(no need for the POST test here, as we already got both asterisks)
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#108 | required_link_text-2613924-108.patch | 523 bytes | johan.s |
#107 | required_link_text-2613924-107.patch | 557 bytes | johan.s |
Comments
Comment #2
attiks CreditAttribution: attiks at Attiks commentedAttached patch marks the link text field as required if both the parent and the link text are required.
Comment #4
marvin_B8 CreditAttribution: marvin_B8 as a volunteer and at comm-press commentedComment #5
BerdirMakes sense, just needs a test.
Comment #6
heykarthikwithuWriting test case for this, first time :)
Comment #7
heykarthikwithuAdded a patch for this.
Comment #9
lokapujyaIsn't this just adding text to the Link Text field? That would break the test, because now the "Link Text field is required." text won't show up, but the assertion is expecting that text.
Comment #10
lokapujyaThe existing test expects the "link text" to be required based on whether the URL is empty. It seems like the test should not care about the URL, but only be concerned with whether the "link text" is required or not.
Comment #11
aerozeppelin CreditAttribution: aerozeppelin commentedTest for #2
Comment #12
lokapujyaVery minor. Don't need a variable, can just pass empty array.
Comment #13
heykarthikwithu1. changed the variable to an empty array.
Comment #14
claudiu.cristeaThis needs a docblock.
entity_create()
is deprecated. use instead:Same here, use
FieldConfig::create()
Use modern
[]
instead ofarray()
.entity_get_form_display
is deprecated. UseEntityFormDisplay::load()
instead and chain with other methods.$edit
makes no sense here. Just put[]
directly inside method call.Comment #15
heykarthikwithuWorking on.
Comment #16
heykarthikwithuAs mentioned in #14
1. added a doc block for the test function.
2. entity_create of 'field_storage_config' is been replaced with FieldStorageConfig::create.
3. entity_create of 'field_config' is replaced with FieldConfig::create.
4. used [] instead of array().
5. entity_get_form_display is replaced with EntityFormDisplay::load.
6. added [], in the function call.
And, changes mentioned in above mentioned 2, 3, 5 points.
These changes are been made in the entire test file.
Comment #17
heykarthikwithuComment #19
heykarthikwithuThis was because of wrong interdiff file extention, so moving back to review.
Comment #21
heykarthikwithuworking on this.
Comment #22
heykarthikwithuComment #23
heykarthikwithuAs mentioned in #14
1. added a doc block for the test function.
2. entity_create of 'field_storage_config' is been replaced with FieldStorageConfig::create.
3. entity_create of 'field_config' is replaced with FieldConfig::create.
4. used [] instead of array().
5. entity_get_form_display is replaced with EntityFormDisplay::load.
6. added [], in the function call.
And, changes mentioned in above mentioned 2, 3, 5 points.
These changes are been made in the entire test file.
Note: ignoring #16 comment patch.
Comment #24
claudiu.cristeaThank you, nice patch.
However
No, please. Revert them back we need only to stick to the point of this issue.
"(...) summary lines must start with a third person singular present tense verb, and they must explain what the function does." - from https://www.drupal.org/coding-standards/docs#functions.
So we need to change this accordingly. Maybe "Tests if the link text is required."?
Comment #25
heykarthikwithuWorking on.
Comment #26
heykarthikwithuChanges done as mentioned in #24
1. reverted back the other changes in #23, to stick to the point of this issue.
2. changed the function doc block text "Tests if the link text is required.".
Comment #27
heykarthikwithuComment #28
claudiu.cristeaNice @heykarthikwithu, we're almost there. Just few nits:
We are not using $this->fieldStorage and $this->field later, right? Then we can omit them and chain the the methods in both cases:
...same for FieldConfig. And, yes, we need to replace
with
so that FieldConfig works
These lines can be merged. Just put the path as first argument in the 2nd, instead of NULL.
Let's use here also the modern
[]
array representation.Otherwise looks good and clean.
Comment #29
heykarthikwithuworking on.
Comment #30
heykarthikwithuChanges as mentioned in #28.
1. unused $this->fieldStorage and $this->field, are removed.
2. merged 'entity_test/add', into drupalPostForm.
3. used modern [] in the function.
Comment #31
claudiu.cristeaNice! RTBC if passes tests. Thank you.
Comment #32
catchWhat happens currently if you save without link text? Does it validate it as required or let it go through?
Thinking about existing content if the latter.
Comment #33
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@catch, currently, the first time the node add form is viewed the field does not have the
*
. After trying to save it displays the*
.After patching, the
*
is displayed only if both the link field AND the text are required. If only the link text is required, then the*
is not displayed before the first time the user tries to save a link without text. I think this is a good behavior because it only requires the link text in case the user input the link address.Marking it as RTBC again.
Comment #34
claudiu.cristea@catch, this patch proves that the validation is in place right now, in HEAD. The only problem is with the lack of "required" mark when visiting the page. That said, I think the current patch is OK but the test not. Because we need to test, not on POST (where the asterisk gets displayed when the validation fail) but on simple GET. On GET is where the form doesn't show the asterisk.
Comment #35
claudiu.cristeaBased on #34, let's change the POST to a GET.
Remove them. This already works.
According to coding standards there's need for an empty line here.
Comment #36
heykarthikwithumerging the LinkWidget.php changes with the test patch.
Comment #37
heykarthikwithunot merging this now, putting back in earlier status.
Comment #38
claudiu.cristeaComment #39
heykarthikwithuAs per #35.
1.
Added this.
2.
Removed the lines.
3. Added a line space.
Comment #40
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedI just changed the order of the used classes to alphabetical order. So far, looks good.
Changing priority back to normal, as it does not allow saving the required field empty before patching. For the user it is just a cosmetic change, yet the new test is a important change.
Comment #41
dawehner+1
Comment #42
claudiu.cristea@Mac_Weber, You cannot RTBC your own patch :)
Also, #39 didn't follow #35.1. @heykarthikwithu, as I recommended there but also in IRC, we need to switch from a POST request to a GET request. We simply want only to show the form without posting. So ->drupalGet(). Why? See #34 why. Because the validation works and also the asterisk is displayed in case of validation failure. But it's not displayed when first time showing the form (aka GET).
Don't need to alphabetically arrange the methods. Adding the new method to the end is OK.
Comment #43
heykarthikwithuComment #44
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedTests on #43 are for the case in which both URL and Link Text are required. I think we also need a POST test in case of only the Link Text is required.
Comment #45
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedComment #46
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedAdded test for URL not required, but Link Text is required.
Comment #47
claudiu.cristeaFirst, we don't need to create a test only for that. If we need some additional scenario we simply add few new lines to
testRequiredLinkText()
. But if we go with test also the POST then we have to follow this path, to test all cases:Comment #48
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@claudiu.cristea what do you think about this:
URL required: false; Link Text required: true
get entity_test/add
check the asterisk mark (it should not exist for both)
post the form
check the asterisk mark (it should exist only for link text)
URL required: true; Link Text required: false
get entity_test/add
check the asterisk mark (it should exist only for URL)
post the form
check the asterisk mark (it should exist only for URL)
URL required: true; Link Text required: true
get entity_test/add
check the asterisk mark (it should exist for both)
(no need for the POST test here, as we already got both asterisks)
Comment #49
claudiu.cristeaThis makes no sense. Is it possible to have such a case? It's possible to have a link with only text but nor URL? Can you manually test that?
Comment #50
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@claudiu.cristea Yes, in this case the case I said at #33.
As you can see at the second image at the issue summary, there are 2 settings for 'required'. The first one the text is
Required field
which is a checkbox that makes the URL required.The second one is the radio boxes for the 'Link text'.
If you do not check the checkbox "Required field" and choose Link text to be required, then it will be required only if you type something at the URL. The asterisk will be shown only after POST with some URL and empty Link text.
Comment #51
claudiu.cristeaOK, let's see a test covering all 3 scenarios. We can put the widget settings and assertions inside a 3 items iteration, so we don't duplicate the code. Anyway, we need to go back to a single test.
Comment #52
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedAdded the tests.
As the settings and messages differ that much then just a small part could be wrapped in a loop.
Comment #53
claudiu.cristeaPlease provide an interdiff every time.
Well, this became to complex.
We don't need 3 fields. We only create one field and then we iterate. Inside an iteration, we change the settings and make the tests.
Move it outside iteration.
Keep only this but remove the config 'required' and settings[title]. Those are variables and will be set inside iteration.
Remove.
Merge all these together like in #43.
Wrap this and everything below in a foreach(). Foreach should take an array of variable things which you'll need to create first:
Comment each case to understand the scenario.
Remove the test for URL field, that is covered elsewhere. Just test the link title.
Don't add assertion messages. Those are obsolete. We rely on good comments that you'll have to provide in $cases array.
Inside foreach() you'll need the following:
Comment #54
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@claudiu.cristea because the settings for each test change so much, then getting these settings in an array to later iterate over it in a loop would not make much difference. The loop would also require more time to run the test as it would test each field setting separated, which means more POST/GET requests than this approach.
The best way I see to make the code readable is to separate the 3 tests into different functions. It would take the same amount of POST/GET transactions as the loop, but much easier for other people to read the code. I'm open to other ideas.
By the way, while testing manually this patch I've found a new related bug: #2634232: In case of only 'link text' is required, then remove the required mark if the user deletes the text entered for URL
Comment #55
claudiu.cristea@Mac_Weber, It will be more compact with the iteration and will have a high degree of code reusing. Don't create tests for each scenario. Eventually you can externalise that array in a protected method.
Comment #56
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedBecause of #2634232: In case of only 'link text' is required, then remove the required mark if the user deletes the text entered for URL
probably we should not mark the 'Link text' with the class
required
in case of URL is not required. By having this class it would not allow form form validation in case the user gives up inserting an URL and deletes what he/she has typed there.Having the
required
mark and class in this case would need some kind of JS to remove this class. Using JS would not work for users with JS disabled and I am not sure if we want it for a core installation.Comment #57
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedAfter a chat with @alexpott at IRC we got the a new idea of progressive enhancement.
To make sure users without JS will get full functionality we remove the "required" class from
Link text
and do the validation anyway if there is text input for URL, just as it is being done at the the patch here: #2634232: In case of only 'link text' is required, then remove the required mark if the user deletes the text entered for URLIn order to improve UX for users with JS we add the
required
mark and class via JS whenever the user input text for URL.Comment #58
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedComment #59
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedComment #62
guldi CreditAttribution: guldi commentedIs there an update on this?
Comment #63
alozie CreditAttribution: alozie commentedAttached is an update for the patch provided in comment #43 by heykarthikwithu. This only makes the patch compatible with the latest 8.2.x branch. It does not address issues raised in subsequent comments, nor change the patch's code otherwise.
Comment #65
Prasun CreditAttribution: Prasun commentedComment #66
Prasun CreditAttribution: Prasun commentedlink_text_is_being_marked_required-2613924-64.patch is perfectly working in drupal 8.4.x-dev and 8.3.x.
Comment #67
sudhanshug CreditAttribution: sudhanshug as a volunteer and at Google Summer of Code commentedQueuing for tests
Comment #69
oknatetesting for drupal 8.4
Comment #70
sudhanshug CreditAttribution: sudhanshug as a volunteer and at Google Summer of Code commentedComment #72
oknateI have reworked the patch and verified it against tests locally.
Conditions where title field should show asterisk.
1) Field is required and title subfield is required, fieldset, uri and title should all show red asterisk
2) Field is not required and title subfield is required, no field should show red asterisk, but on submitting, if only the uri is filled out, it should reload form with red asterisk and form error on the title subfield.
Comment #73
oknateAdding a new patch that uses #states to require link text when uri is not empty on the front end.
Comment #74
oknateAdding in the functional test from @alozie from https://www.drupal.org/files/issues/required_link_text-2613924-63.patch
Comment #75
oknateComment #76
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedPatch looks good. There are a few uses of deprecated functions:
Deprecated. Use
$this->assertEquals()
Deprecated. Use
$this->assertSession()->pageTextNotContains()
Comment #77
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedComment #79
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedI just realized one missing thing in the last patches: according to #57 the form should not require the field in case of JS disabled. The validation will take care of it and the 'required' mark should only be added via JS in this specific case.
In addition, the last patch uses
t()
in tests. We are not using it anymore in tests.New patch with these fixes attached. Other tests might break on the testbot because I changed one interface error string.
Comment #80
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedThis string should be changed to something like "Tests if the link text is validated as required and it does not have the required class."
I also opened a follow-up issue: #2879293: Make Link URI required if there is Link Text input
Comment #81
karan_kural CreditAttribution: karan_kural commented.
Comment #82
Wim LeersWhy change from
@name
to@title
?And why even change the string at all?
This is a string change!
Let's use
===
, and let's not wrap it in parentheses unnecessarily.These if-conditions can be merged. Also:
===
again.s/uri/URI/
s/front-end/front end/
This looks like a regression.
Comment #83
oknateignore this one, I accidentally included local files
Comment #84
oknateComment #86
oknateApplying Wem Leers feedback
Comment #87
oknateComment #89
oknateApplying Wem Leers feedback, with fix in test case to remove extra text
Comment #90
oknateUpdate to last patch, this time replacing deprecated functions in other parts of core/modules/link/tests/src/Functional/LinkFieldTest.php
Comment #91
oknateComment #92
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@oknate replacing the other deprecated entries is out of scope of this issue. We better replace all the old ones in a single (and new) issue, including all files in the module.
@Wim Leers
Because the variable is named title, I think it would be confusing using
uri
/url
withname
.Because the
title
field will only be required if there is a URL input, so a more meaningful message.Right.
They would pass column 80, that's why I had chosen to split them.
Right.
Right.
Not a regression. This happens when the field itself is not marked as required and the
link text
is marked as required.. In this case, the link field will be marked as required only when the user input URI data, then this is the expected behavior here. See the issue summary and #57:Comment #93
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedComment #95
oknateHere's a new version of the patch that fixes an issue I was noticing where the selector wasn't working for field fields within paragraphs, for example, "field_paragraph_field[0][subform][field_link_field_on_paragraph]"
Comment #96
claudiu.cristeaComment #98
pguillard CreditAttribution: pguillard commentedThis patch is fixing the failing test.
Then, I would say RTBC.
Comment #100
pguillard CreditAttribution: pguillard commentedOoops, sorry
Comment #101
darvanenInstalled on a fresh standard profile and tested both as a base field on a node and within a paragraph.
Patch looks good to me, setting RTBC.
I imagine once a core maintainer gets a look at it they'll do a thorough code review and pick up anything I've missed.
Comment #102
larowlanAs per https://www.drupal.org/core/d8-allowed-changes this isn't allowed during 8.4.x (patch releases) because it changes a translatable string.
It therefore has to go into the next minor release.
Adding review credits.
Comment #104
larowlanCommitted as 3410d78 and pushed to 8.5.x
Comment #106
larowlanThis broke base fields with non required title
#2937889: Regression: LinkWidget no longer supports base fields with optional title
Comment #107
johan.s CreditAttribution: johan.s commentedWe add asterisk for the uri field when title is filled because the validation is only in when the form is submit.
Comment #108
johan.s CreditAttribution: johan.s commentedUpdate the path file git apply patch.
Comment #109
mpp CreditAttribution: mpp at District09 for District09 commentedHi Johan, welcome on drupal.org and thank you for the patch. As this ticket is already closed, I suggest you open a new one and relate to this issue.
You'll also have to replace the tabs with whitespace characters. See https://www.drupal.org/docs/develop/standards/coding-standards#indenting
Comment #110
banoodle CreditAttribution: banoodle at Kanopi Studios commentedIt looks like this issue was closed automatically due to inactivity, but as of 8.7.10, this problem is still occurring.
I tried applying patch #107 but it wouldn't apply.
Patch #108 applied cleanly, but didn't resolve the problem.
I've created a new issue here: https://www.drupal.org/project/drupal/issues/3100979
Comment #111
stefan.kornI think #3100979: Link text isn't marked as required (followup to 2613924) is not about what johan.s described in #107. So I created a new issue for what affects #107: #3203656: Make uri required on the front-end when title filled-in