Changing link title configuration leads to displaying old values

Applied patch #2 successfully

Steps for reproducing the issue mentioned in the issue description is easy to follow.

Before patch

Even if we edit the URL, old link text with new url will be shown.

After patch

When the link text is disabled and a content with link text is edited, old link text won't be displayed and the updated url will be shown

I don't think this is technically a bug, but I think we should still resolve it.

We need a test here demonstrating the issue and that it is fixed

Looks like a test has been added, removing tag

I have applied the #6 patch on 9.2.x dev version and it works fine .
Before Patch
After Patch .RTBC

Re-rolled

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -233,7 +233,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#default_value' => $this->getFieldSetting('title') === DRUPAL_DISABLED ? NULL : (isset($items[$delta]->title) ? $items[$delta]->title : NULL),
   

   (isset($items[$delta]->title) ? $items[$delta]->title : NULL)
   could use the null coalescing operator, ??.

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +365,23 @@ public function testLinkTitle() {
   +    // Verify that if you change the settings to disable the title then link is
   +    // rendered using the URL as text and not keeping the old title.
   

   This could be simpler.

   Verify that if the title field is disabled the link is rendered using the URL as text.

3. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +365,23 @@ public function testLinkTitle() {
   +    $this->drupalPostForm(NULL, $edit, t('Save'));
   +    $this->assertText(t('entity_test @id has been updated.', ['@id' => $id]));
   

   drupalPostForm and assertText have been deprecated.

@quietone , I have updated the patch as per the inputs given by you.

Please review.

@vakulrai, thanks.

This time I applied that patch locally. And seeing it my editor I noticed more items.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -233,7 +233,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#default_value' => $this->getFieldSetting('title') === DRUPAL_DISABLED ? NULL : $items[$delta]->title ?? NULL,
   ...
          '#access' => $this->getFieldSetting('title') != DRUPAL_DISABLED,
          '#required' => $this->getFieldSetting('title') === DRUPAL_REQUIRED && $element['#required'],
   

   I didn't notice this before but there are now 3 instances of $this->getFieldSetting('title'). I think it is now time to put that in a variable and then use the variable here, say, $title = $this->getFieldSetting('title'). One advantage is that it will reduce the length of the line being added which, at least for me, will make it easier to read.

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -282,7 +282,7 @@ public function testLinkTitle() {
   +    $assert_session = $this->assertSession();
   

   No need to do this here for the one line added many lines below in this test.

3. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +365,23 @@ public function testLinkTitle() {
   +    // Verify that if the title field is disabled the link is rendered
   +    // using the URL as text.
   

   Wrapping is wrong here.

4. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +365,23 @@ public function testLinkTitle() {
   +    $this->field = FieldConfig::loadByName('entity_test', 'entity_test', $field_name);
   

   The field config has not changed so does not need to be loaded again.

5. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +365,23 @@ public function testLinkTitle() {
   +    $this->submitForm($edit, t('Save'));
   

   Remove use of t(), it is only used in tests when specifically testing translations.

6. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +365,23 @@ public function testLinkTitle() {
   +    $assert_session->pageTextContains(t('entity_test @id has been updated.', ['@id' => $id]));
   

   change to using $this->assertSession()

@quietone , I have changed the patch as per #14, Please review.

Thanks

Uploaded wrong interdiff in # 15, please consider this one.

Getting there. :-)

Some out of scope changes have crept in.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -202,7 +202,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -      //    in https://www.drupal.org/node/2423093.
   +      // in https://www.drupal.org/node/2423093.
   

   Out of scope change.

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -228,15 +228,15 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $title = $this->getFieldSetting('title');
   

   The variable I suggested $title doesn't make much sense because this is a setting. The name should be descriptive, maybe $title_settings? Of course, you may think of something better.

3. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -282,7 +282,6 @@ public function testLinkTitle() {
   -
   

   Out of scope change.

4. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +364,22 @@ public function testLinkTitle() {
   +    // Verify that if the title field is disabled.
   +    // The link is rendered using URL as text.
   

   Not wrapped correctly. More details about this standard is in the Drupal API documentation standards (general) sectiion.

5. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +364,22 @@ public function testLinkTitle() {
   +    $this->assertSession()->pageTextContains(t('entity_test @id has been updated.', ['@id' => $id]));
   

   As mentioned in #15.5 t() is not used in tests, unless specifically testing translations. This should be a string of the text expected. Refer to the other assertions in the test.

We also need a fail patch for this. Remember to upload that first then the success path.

@quietone updated as per #18, and i had a question regarding failing patch, where do we modify code for that case, under tests folders?

Attached before and after patch screenshot for reference.
Patch #19 seems fine.

@Neslee Canil Pinto, nice question. A fail patch is a patch that contains the test file(s) only. In this case, when the testbot runs the tests with a fail patch testLinkTitle will fail and that gives us evidence that the test is actually testing something we want to change. And then when the 'success' patch runs we have evidence that the fix does, in fact, fix the thing being tested. The fail patch is typically named 3165999-commentNumber-fail.patch. As an example, see https://www.drupal.org/project/drupal/issues/3109767#comment-14093095. It is important to upload the fail patch first and then the success patch. That way the testbot finishes with the success patch and updates the Status based on the success patch.

#18.4 needs to be fixed, the comment should be wrapped at 80 columns.

This time I manually tested the patch and can confirm it works. And I added the complete template to the IS and added remaining tasks.

@quietone , I have followed the flow you have stated in #21, hence reuploading the patch with updated fix for #18.4 and fail patch followed by success patch .I have also made changes to the comment text, which follows 80 line syntax.

Please review.

@vakulrai, thanks.

Everything in #18 and #21 have been addressed. I didn't find anything else that needs to be done when reading through the issue.

@Gauravmahlawat, I'm removing your issue-credit for attaching screenshots, as they'd already been added twice above.
Crediting @quietone for reviews that strongly shaped the final patch

Looks good, Needs Review for point 1 below - tl;dr I don't think we need to null coalesce $item[$delta]->title, I think TypedData API will have our back there

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -228,15 +228,15 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#default_value' => $title_settings === DRUPAL_DISABLED ? NULL : $items[$delta]->title ?? NULL,
          '#maxlength' => 255,
   

   It would be good to add some additional brackets here for readability

   e.g.

   $title_settings === DRUPAL_DISABLED ? NULL : ($items[$delta]->title ?? NULL)
   

   But now that I write that, I see we're null coalescing to ... null

   Is there a scenario where $items[$delta]->title would raise an error? Given that is going through the Typed data API, I think that it would return NULL regardless and shouldn't raise and error.

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -365,6 +365,22 @@ public function testLinkTitle() {
   +    $expected_link = (string) Link::fromTextAndUrl($value, Url::fromUri($value))->toString();
   

   reading this your first thought is wait, why cast the result of ->toString to a string, and then I realised that ->toString returns a GeneratedLink 🤦‍♂️ not the best naming

Needs reroll and #26 needs to be addressed.

Reroll added for patch #22.

You rerolled the tests only patch..

And by skipping the actual patch didn't address #26 issues.

Please read the ticket and check the code before uploading a patch. Helps keep things clear. Would recommend reading up on some documentation on drupal.org tons of good stuff that really helped me!

Hi @smustgrave,
The file

core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php

already has #22 patch changes for 9.5.x-dev. So I skipped it and #26 suggestion was not address.

Thanks

Can you double check that? I'm not seeing it. And if it were already addressed then this ticket is a duplicate

Skipping #30 because it just rerolled the tests only patch from #22.
I confirmed the full changes in #22 were not included as mentioned in #32 so no worries.

Tried addressing #26 comments
26.1 = Added the brackets but seems more like a comment and discussion
26.2 = Also seems like a comment more than a requested change.

Verified the patch and tested on 9.5.x-dev. Patch applied successfully.

Test Steps:
1. Add a field link to the content type article with the setting Required for Allow link text
2. Create a content and put a URL/Title in this link e.g. : https://www.example.com / Example Domain
3. Save and view the content, you will see Example Domain
4. Change the configuration of the link field to set the title as Disabled
5. Edit your content and replace the URL by https://www.google.com
6. Save and view the content, you will see https://www.google.com

Test Result:
When the Allow link text setting of a link field is changed from Optional/Required to Disabled after content has been saved and the content is edited again > Then the content displays the new title value

Before Patch:

After Patch:

RTBC +1

Thank you for reviewing @sonam.chaturvedi if you feel this good can you move to RTBC please.

Applied patch #34 on 9.5.x and it fixes the issue.

Before patch:

After patch:

Thank you for testing if you think the issue is fixed please mark it as such

Patch #34 Applied successfully on Drupal 9.4.x and Drupal 9.5.x.

Patch #34 is working fine.
see in #35, #37 and #39 comments.
problem solved after applying the #34 patch.
changed to RTBC.

@smustgrave Regarding #36 and #38, manual testing is only one step before RTBC.

From what I can tell no code review or other core gate checks have happened since #34.

I'm moving this back to needs review for this reason.

THIS ISSUE DOES NOT NEED MORE MANUAL TESTING. (It needs code review and core gate checks. Thanks.)

Not sure about the nested ternary, but that's just a preference and in this case moving it out of the array doesn't necessarily help. Otherwise looks good.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -243,14 +243,15 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $title_settings = $this->getFieldSetting('title');
   

   a nitpick so small you need an electron microscope to see it, but I think this should be $title_setting, not $title_settings

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -243,14 +243,15 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#default_value' => $title_settings === DRUPAL_DISABLED ? NULL : ($items[$delta]->title ?? NULL),
   

   #26.1 still hasn't been addressed here, and was also flagged by @catch as iffy

Addressed #45.1

Tested and verified the last patch #46. It was successfully applied on drupal 9.5.x. The above patch changed as per commented on #45. The above patch is not applicable for drupal version 10.1.x.

Needs work for #45.2

@Rinku Jacob 13, Before testing an issue it is good practice to read the comments. The work requested in #45 is not complete which means this patch is not ready for testing.

Hi @quietone , sorry for the mistake i have done. I didn't noticed comment on 45.2. According to comment on 26.1 we can change

'#default_value' => $title_setting === DRUPAL_DISABLED ? NULL : ($items[$delta]->title ?? NULL),

to

'#default_value' => $title_setting === DRUPAL_DISABLED ? NULL : ($items[$delta]->title),

because it shouldn't raise an error. I think this changes we need to do here. If it's wrong please excuse me. If it is correct i will create a patch based on that.

#46 seemed to be making additional changes then 45.1

Addressed both points. Just moved the ternary check above to make it easier to read.

Applied patch #50 successfully and it fixed the issue. Also addressed both points mentioned in #45.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Manually tested the patch on 10.1.x-dev and it resolves the issue. The IS looks good. Before/after screenshots for this iteration were added in #52.

+    $this->assertSession()->pageTextContains('entity_test ' . $id . ' has been updated.');

I wondered if this should use FormattableMarkup, but given that \d+ matches only integer values, I think it can be ommitted in this case. Didn't find any other issues. So I'd say the code looks good.

Bottom line: I think the issue is RTBC.

Random failure

Random

Think it's random.

Random failure

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -244,14 +244,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    if ($title_setting !== DRUPAL_DISABLED && isset($items[$delta]) && isset($items[$delta]->title)) {

ubernit: do we need two calls to isset here?

I think php will allow just the second one

Fine to self-rtbc after that change, please ping me to look again at that point.

Hi @larowlan,
I have removed the isset($items[$delta]) as per #63 comment.

Thanks!

Looks like failing linting, thanks

Updated patch #64 by fixing it, please review it.

Thanks!

Removing credit for Ahmad Smhan for the empty commit

#64 appeared to be adding out of scope of changes

#66 seems good. Since this was previously RTBC and nitpicky change, think safe to remark it.

Updating issue credits

I was about to commit this and whilst going through my internal checklist realised that we're making a widget/form level change here but we're not doing anything to address API level changes too.

Which leads me to believe that the fix probably belongs in LinkItem instead of in the widget.

So can we undo our changes to the widget here and instead change LinkItem::setValue to set the title to NULL when the title option is set to Disabled

We can use \Drupal\Tests\link\Kernel\LinkItemTest::testLinkItem to extend test coverage for that change.

We can retain the test changes we've made in this issue, but we need to undo the changes to the widget.

Sorry for not realising this sooner, I realise we've been around and around a few times on this one.

Per @larowlan's comment in #69 I moved the changes to LinkItem::setValue().

Feedback appears to be addressed.

This is going to silently delete old data which if a site is experimenting with field settings might possibly want to be kept around. Including in migrations judging by the test changes.

What about checking the field setting in the formatter instead?

What about checking the field setting in the formatter instead?

You'd have to implement the setting check in every formatter. There's no guarantee contrib formatters would comply or that compliance would happen quickly. That could result in an issue where a link is displayed with different formatters in different contexts where it's rendered with and without a title.

I'm also concerned about the title subfield getting disabled and then re-enabled at some point much later on. During that time span links may be changed, but old hidden title values would be retained and could get out-of-date. That thought kind of drives home the point that we're going down a rabbit hole of edge cases. Regardless, it's a concern.

I don't know that there's a perfect solution.

@catch thoughts on #76?

@catch what do you think about landing in 12 only? As dcam mentioned may not have a perfect solution but this seems like an improvement.

Apologies, I know this one's gone around a couple times, but I think we can address #75 by overriding the magic getter instead of setting the value to NULL in setValue(). (For D12, we could also use property hooks once we figure out what we're going to do about code style sniffs, but that's a follow up.)

Something like

  public function __get($name) {
      if ($name === 'title' && $this->getSetting('title') === LinkTitleVisibility::Disabled->value) {
        return NULL;
    }
    return parent::__get($name);
  }

Edited to add:
Actually, maybe not. I think this might cause the empty value to be set in storage on a re-save.

...this might cause the empty value to be set in storage on a re-save.

Yes, it does. I tested it by removing the current changes to LinkItem and adding the magic getter from #80. The title was set to NULL upon re-saving the entity while the title field was disabled.

I'm restoring the RTBC status.