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

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

CommentFileSizeAuthor
#108 required_link_text-2613924-108.patch523 bytesjohan.s
#107 required_link_text-2613924-107.patch557 bytesjohan.s
#100 interdiff-2613924-95-100.txt671 bytespguillard
#100 required_link_text-2613924-100.patch3.41 KBpguillard
#98 required_link_text-2613924-98.patch3.41 KBpguillard
#95 required_link_text-2613924-95.patch2.64 KBoknate
#90 required_link_text-2613924-90.patch22.06 KBoknate
#89 required_link_text-2613924-89.patch11.37 KBoknate
#86 required_link_text-2613924-84.patch11.39 KBoknate
#83 required_link_text-2613924-83.patch13.29 KBoknate
#79 required_link_text-2613924-78.patch11.74 KBMac_Weber
#76 required_link_text-2613924-76-tests-only.patch7.22 KBMac_Weber
#76 required_link_text-2613924-76.patch8.96 KBMac_Weber
#76 interdiff-74-76.txt4.94 KBMac_Weber
#74 link_text_is_being_marked_required-2613924-74.patch8.93 KBoknate
#73 link_text_is_being_marked_required-2613924-73.patch1.73 KBoknate
#72 link_text_is_being_marked_required-2613924-72.patch1.19 KBoknate
#69 link_text_is_being_marked_required-2613924-69.patch1.4 KBoknate
#65 link_text_is_being_marked_required-2613924-64.patch1.25 KBPrasun
#63 required_link_text-2613924-63.patch7.79 KBalozie
#52 required_link_text-2613924-52.patch8.55 KBMac_Weber
#46 interdiff-43to46.txt2.33 KBMac_Weber
#46 required_link_text-2613924-46.patch4.85 KBMac_Weber
#43 interdiff-2613924-39-43.txt723 bytesheykarthikwithu
#43 2613924-43.patch2.86 KBheykarthikwithu
#40 required_link_text-2613924-40.patch3.47 KBMac_Weber
#39 interdiff-2613924-30-39.txt894 bytesheykarthikwithu
#39 2613924-39.patch2.95 KBheykarthikwithu
#34 link_test.patch2.19 KBclaudiu.cristea
#30 interdiff-2613924-26-30.txt2.34 KBheykarthikwithu
#30 2613924-30.patch3.04 KBheykarthikwithu
#26 interdiff-2613924-23-26.txt5.23 KBheykarthikwithu
#26 2613924-26.patch3.14 KBheykarthikwithu
#23 interdiff-2613924-13-23.txt7.16 KBheykarthikwithu
#23 2613924-23.patch8.1 KBheykarthikwithu
#17 interdiff-2613924-13-16.txt6.13 KBheykarthikwithu
#16 interdiff-2613924-13-16.patch6.13 KBheykarthikwithu
#16 2613924-16.patch7.55 KBheykarthikwithu
#13 interdiff-2613924-11-13.txt701 bytesheykarthikwithu
#13 2613924-13.patch2.84 KBheykarthikwithu
#11 interdiff-2613924-7-11.txt2.01 KBaerozeppelin
#11 2613924-11.patch2.86 KBaerozeppelin
#7 interdiff-2613924-2-7.txt646 bytesheykarthikwithu
#7 2613924-7.patch1.58 KBheykarthikwithu
#2 i2613924.patch862 bytesattiks
Screenshot from 2015-11-12 11:03:10.png49.02 KBattiks
Screenshot from 2015-11-12 11:01:34.png12.36 KBattiks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks created an issue. See original summary.

attiks’s picture

Status: Active » Needs review
FileSize
862 bytes

Attached patch marks the link text field as required if both the parent and the link text are required.

Status: Needs review » Needs work

The last submitted patch, 2: i2613924.patch, failed testing.

marvin_B8’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Makes sense, just needs a test.

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu

Writing test case for this, first time :)

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
1.58 KB
646 bytes

Added a patch for this.

Status: Needs review » Needs work

The last submitted patch, 7: 2613924-7.patch, failed testing.

lokapujya’s picture

+++ b/core/modules/link/src/Tests/LinkFieldTest.php
@@ -275,6 +275,8 @@ function testLinkTitle() {
           // Verify that the link text is required, if the URL is non-empty.
           $edit = array(
             "{$field_name}[0][uri]" => 'http://www.example.com',
+            // if the URL is non-empty, text field should also not be empty.
+            "{$field_name}[0][title]" => 'Example',
           );
           $this->drupalPostForm(NULL, $edit, t('Save'));
           $this->assertText(t('@name field is required.', array('@name' => t('Link text'))));

Isn'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.

lokapujya’s picture

The 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.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
2.01 KB

Test for #2

lokapujya’s picture

+++ b/core/modules/link/src/Tests/LinkFieldTest.php
@@ -593,4 +593,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
+    $edit = [];
+    $this->drupalPostForm(NULL, $edit, t('Save'));

Very minor. Don't need a variable, can just pass empty array.

heykarthikwithu’s picture

1. changed the variable to an empty array.

claudiu.cristea’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +593,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +  function testRequiredLinkText() {
    

    This needs a docblock.

  2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +593,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->fieldStorage = entity_create('field_storage_config', array(
    

    entity_create() is deprecated. use instead:

    $this->fieldStorage = FieldStorageConfig::create([
      ...
    ]);
    
  3. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +593,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->field = entity_create('field_config', array(
    

    Same here, use FieldConfig::create()

  4. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +593,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +      'settings' => array(
    

    Use modern [] instead of array().

  5. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +593,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    entity_get_form_display('entity_test', 'entity_test', 'default')
    

    entity_get_form_display is deprecated. Use EntityFormDisplay::load() instead and chain with other methods.

  6. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +593,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $edit = [];
    +    $this->drupalPostForm(NULL, $edit, t('Save'));
    

    $edit makes no sense here. Just put [] directly inside method call.

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu

Working on.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
7.55 KB
6.13 KB

As 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.

heykarthikwithu’s picture

FileSize
6.13 KB

Status: Needs review » Needs work

The last submitted patch, 16: interdiff-2613924-13-16.patch, failed testing.

heykarthikwithu’s picture

Status: Needs work » Needs review

This was because of wrong interdiff file extention, so moving back to review.

The last submitted patch, 16: 2613924-16.patch, failed testing.

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu

working on this.

heykarthikwithu’s picture

Status: Needs review » Needs work
heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
8.1 KB
7.16 KB

As 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.

claudiu.cristea’s picture

Status: Needs review » Needs work

Thank you, nice patch.

However

And, changes mentioned in above mentioned 2, 3, 5 points. These changes are been made in the entire test file.

No, please. Revert them back we need only to stick to the point of this issue.


+++ b/core/modules/link/src/Tests/LinkFieldTest.php
@@ -593,39 +596,42 @@
+  /**
+   * Check link text is required.
+   */

"(...) 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."?

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu

Working on.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
3.14 KB
5.23 KB

Changes 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.".

heykarthikwithu’s picture

claudiu.cristea’s picture

Status: Needs review » Needs work

Nice @heykarthikwithu, we're almost there. Just few nits:

  1. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,46 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->fieldStorage = FieldStorageConfig::create([
    ...
    +    $this->field = FieldConfig::create([
    

    We are not using $this->fieldStorage and $this->field later, right? Then we can omit them and chain the the methods in both cases:

    FieldStorageConfig([
      ...
    ])->save();
    

    ...same for FieldConfig. And, yes, we need to replace

    +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,46 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +      'field_storage' => $this->fieldStorage,
    

    with

    'field_name' => $field_name,
    'entity_type' => 'entity_test'
    

    so that FieldConfig works

  2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,46 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->drupalGet('entity_test/add');
    +    $this->drupalPostForm(NULL, [], t('Save'));
    

    These lines can be merged. Just put the path as first argument in the 2nd, instead of NULL.

  3. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,46 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +      ->setComponent($field_name, array(
    ...
    +    $result = $this->xpath('//label[contains(@class, :class) and contains(text(), :text)]', array(':class' => 'form-required', ':text' => 'Link text'));
    ...
    +    $this->assertText(t('@url field is required.', array('@url' => t('URL'))));
    +    $this->assertText(t('@name field is required.', array('@name' => t('Link text'))));
    

    Let's use here also the modern [] array representation.

Otherwise looks good and clean.

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu

working on.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
3.04 KB
2.34 KB

Changes 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.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Nice! RTBC if passes tests. Thank you.

catch’s picture

Status: Reviewed & tested by the community » Needs review

What 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.

Mac_Weber’s picture

Status: Needs review » Reviewed & tested by the community

@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.

claudiu.cristea’s picture

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

@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.

claudiu.cristea’s picture

  1. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->drupalPostForm('entity_test/add', [], t('Save'));
    

    Based on #34, let's change the POST to a GET.

  2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->assertText(t('@url field is required.', ['@url' => t('URL')]));
    +    $this->assertText(t('@name field is required.', ['@name' => t('Link text')]));
    

    Remove them. This already works.

  3. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,44 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +  }
     }
    

    According to coding standards there's need for an empty line here.

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu
Status: Needs review » Needs work

merging the LinkWidget.php changes with the test patch.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review

not merging this now, putting back in earlier status.

claudiu.cristea’s picture

Status: Needs review » Needs work
heykarthikwithu’s picture

Status: Needs work » Needs review
FileSize
2.95 KB
894 bytes

As per #35.
1.

+    $edit = ["{$field_name}[0][uri]" => 'http://www.example.com'];
+    $this->drupalPostForm('entity_test/add', $edit, t('Save'));

Added this.
2.

-    $this->assertText(t('@url field is required.', ['@url' => t('URL')]));
-    $this->assertText(t('@name field is required.', ['@name' => t('Link text')]));

Removed the lines.
3. Added a line space.

Mac_Weber’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community
FileSize
3.47 KB

I 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.

dawehner’s picture

+1

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work

@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.

heykarthikwithu’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
723 bytes
Mac_Weber’s picture

Status: Needs review » Needs work

Tests 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.

Mac_Weber’s picture

Assigned: Unassigned » Mac_Weber
Mac_Weber’s picture

Assigned: Mac_Weber » Unassigned
Status: Needs work » Needs review
FileSize
4.85 KB
2.33 KB

Added test for URL not required, but Link Text is required.

claudiu.cristea’s picture

First, 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:

  1. url required: false
  2. get entity_test/add
  3. check the asterisk mark
  4. post the form
  5. check the asterisk mark
  6. url required: true
  7. get entity_test/add
  8. check the asterisk mark
  9. post the form
  10. check the asterisk mark
Mac_Weber’s picture

Status: Needs review » Needs work

@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)

claudiu.cristea’s picture

URL required: false; Link Text required: true

This 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?

Mac_Weber’s picture

@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.

claudiu.cristea’s picture

OK, 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.

Mac_Weber’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.55 KB

Added the tests.

As the settings and messages differ that much then just a small part could be wrapped in a loop.

claudiu.cristea’s picture

Status: Needs review » Needs work

Please provide an interdiff every time.

Well, this became to complex.

  1. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $field_both_required = Unicode::strtolower($this->randomMachineName());
    +    $field_url_only = Unicode::strtolower($this->randomMachineName());
    +    $field_text_only = Unicode::strtolower($this->randomMachineName());
    

    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.

  2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +      FieldStorageConfig::create([
    

    Move it outside iteration.

  3. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    FieldConfig::create([
    ...
    +        'title' => DRUPAL_REQUIRED,
    

    Keep only this but remove the config 'required' and settings[title]. Those are variables and will be set inside iteration.

  4. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    // Creates a field with only URL set as required.
    +    FieldConfig::create([
    ...
    +    // Creates a field with only link text set as required.
    +    FieldConfig::create([
    

    Remove.

  5. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +
    +    // Sets the form display.
    +    $component_settings = [
    +      'type' => 'link_default',
    +      'settings' => [
    +        'placeholder_url' => 'http://example.com',
    +        'placeholder_title' => 'Enter the text for this link',
    +      ],
    +    ];
    +    EntityFormDisplay::load('entity_test.entity_test.default')
    +      ->setComponent($field_both_required, $component_settings)
    +      ->setComponent($field_url_only, $component_settings)
    +      ->setComponent($field_text_only, $component_settings)
    +      ->save();
    

    Merge all these together like in #43.

  6. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
    @@ -593,4 +596,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    // Display creation form.
    +    $this->drupalGet('entity_test/add');
    

    Wrap this and everything below in a foreach(). Foreach should take an array of variable things which you'll need to create first:

    $cases = [
      // Explain this case..
      [...1st case params...],
      // Explain this case..
      [...2nd case params...],
      // Explain this case..
      [...3rd case params...],
    ];
    

    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:

    1. Load the FieldConfig entity an set the 'required' property to [TRUE|FALSE]. This is the first variable that needs to be added on each $cases item. Set the settings[title] setting to [DRUPAL_REQUIRED|DRUPAL_OPTIONAL]. This is the 2nd parameter from any item in $cases. Save the FieldConfig.
    2. GET the form.
    3. Check that link title asterisk [exists|not exists] (this is the 3rd variable parameter on each $cases item)
    4. POST the form. If the POST $edit is variable, then you can add also the $edit as an item on each $cases items.
    5. Check again the asterisk mark.
Mac_Weber’s picture

@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

claudiu.cristea’s picture

@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.

Mac_Weber’s picture

Because 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.

Mac_Weber’s picture

Issue summary: View changes

After 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 URL

In order to improve UX for users with JS we add the required mark and class via JS whenever the user input text for URL.

Mac_Weber’s picture

Issue summary: View changes
Mac_Weber’s picture

Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

guldi’s picture

Is there an update on this?

alozie’s picture

Attached 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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Prasun’s picture

Prasun’s picture

link_text_is_being_marked_required-2613924-64.patch is perfectly working in drupal 8.4.x-dev and 8.3.x.

sudhanshug’s picture

Status: Needs work » Needs review

Queuing for tests

Status: Needs review » Needs work

The last submitted patch, 65: link_text_is_being_marked_required-2613924-64.patch, failed testing.

oknate’s picture

sudhanshug’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 69: link_text_is_being_marked_required-2613924-69.patch, failed testing.

oknate’s picture

I 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.

oknate’s picture

Adding a new patch that uses #states to require link text when uri is not empty on the front end.

$element['title']['#states']['required'] = [
  ':input[name="' . $field_name . '[' . $delta . '][uri]"]' => ['filled' => TRUE]
];
oknate’s picture

oknate’s picture

Status: Needs work » Needs review
Mac_Weber’s picture

Patch looks good. There are a few uses of deprecated functions:

  1. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
    @@ -737,4 +738,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->assertEqual(count($result), 1, "URL is marked as required when both the URL and link field are set to required.");
    

    Deprecated. Use $this->assertEquals()

  2. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
    @@ -737,4 +738,128 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    +    $this->assertNoText(t('@name field is required.', array('@name' => t('Link text'))));
    

    Deprecated. Use $this->assertSession()->pageTextNotContains()

Mac_Weber’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 76: required_link_text-2613924-76-tests-only.patch, failed testing.

Mac_Weber’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.74 KB

I 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.

Mac_Weber’s picture

+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -737,4 +738,144 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
+    // Tests if the link text is not marked as required after posting a form
+    // with data only for URL for the field $field_url_only.

This 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

karan_kural’s picture

Status: Needs review » Reviewed & tested by the community

.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +String change in 8.4.0
  1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -151,10 +151,9 @@ public static function validateUriElement($element, FormStateInterface $form_sta
    +      $form_state->setError($element['title'], t('@title field is required if there is @uri input.', ['@title' => $element['title']['#title'], '@uri' => $element['uri']['#title']]));
    

    Why change from @name to @title?

    And why even change the string at all?

    This is a string change!

  2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -218,12 +217,24 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      '#required' => ($this->getFieldSetting('title') == DRUPAL_REQUIRED && $element['#required']),
    

    Let's use ===, and let's not wrap it in parentheses unnecessarily.

  3. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -218,12 +217,24 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +    if (!$this->isDefaultValueWidget($form_state)) {
    +      if ($this->getFieldSetting('title') == DRUPAL_REQUIRED) {
    

    These if-conditions can be merged. Also: === again.

  4. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -218,12 +217,24 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +        // Make title required on the front-end when uri filled-in.
    

    s/uri/URI/
    s/front-end/front end/

  5. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
    @@ -282,14 +283,14 @@ public function testLinkTitle() {
    -          $this->assertText(t('@name field is required.', ['@name' => t('Link text')]));
    +          $this->assertText('Link text field is required if there is URL input');
    

    This looks like a regression.

oknate’s picture

ignore this one, I accidentally included local files

oknate’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 83: required_link_text-2613924-83.patch, failed testing.

oknate’s picture

Applying Wem Leers feedback

oknate’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 86: required_link_text-2613924-84.patch, failed testing.

oknate’s picture

Applying Wem Leers feedback, with fix in test case to remove extra text

oknate’s picture

Update to last patch, this time replacing deprecated functions in other parts of core/modules/link/tests/src/Functional/LinkFieldTest.php

oknate’s picture

Status: Needs work » Needs review
Mac_Weber’s picture

Status: Needs review » Needs work

@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

  1. Why change from @name to @title?

    Because the variable is named title, I think it would be confusing using uri/url with name.

    And why even change the string at all?

  2. Because the title field will only be required if there is a URL input, so a more meaningful message.

  3. Let's use ===, and let's not wrap it in parentheses unnecessarily.

    Right.

  4. These if-conditions can be merged.

    They would pass column 80, that's why I had chosen to split them.

    Also: === again.

    Right.

  5. s/uri/URI/
    s/front-end/front end/

    Right.

  6. 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:

    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

Mac_Weber’s picture

Issue summary: View changes

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

oknate’s picture

Here'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]"

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 95: required_link_text-2613924-95.patch, failed testing. View results

pguillard’s picture

Status: Needs work » Needs review
FileSize
3.41 KB

This patch is fixing the failing test.
Then, I would say RTBC.

Status: Needs review » Needs work

The last submitted patch, 98: required_link_text-2613924-98.patch, failed testing. View results

pguillard’s picture

Status: Needs work » Needs review
FileSize
3.41 KB
671 bytes

Ooops, sorry

darvanen’s picture

Status: Needs review » Reviewed & tested by the community

Installed 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.

larowlan’s picture

Version: 8.4.x-dev » 8.5.x-dev
Issue tags: -String change in 8.4.0

As 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.

  • larowlan committed 3410d78 on 8.5.x
    Issue #2613924 by heykarthikwithu, oknate, Mac_Weber, pguillard, attiks...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 3410d78 and pushed to 8.5.x

Status: Fixed » Closed (fixed)

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

larowlan’s picture

johan.s’s picture

FileSize
557 bytes

We add asterisk for the uri field when title is filled because the validation is only in when the form is submit.

johan.s’s picture

Update the path file git apply patch.

mpp’s picture

Hi 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

banoodle’s picture

It 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

stefan.korn’s picture

I 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