Add \Drupal\link\LinkItemInterface::toLink method

I really like this feature. I'm just a bit worries about the potential BC break, mhhhh

Yeah that failure was a purely random one. Now that I look at this patch again we should ideally add some test coverage for it.
\Drupal\Tests\link\Kernel\LinkItemTest would be a perfect place for it. Sorry :)

Hi Daniel Wehner,

As suggested in #8,
I have added the Drupal\Tests\link\Kernel\LinkItemTest.
Please find the updated patch.

Thanks,

The idea makes sense.

Question: would toLink() make more sense than getLink()? getLink() makes me think it's giving me a property, but it's actually constructing a link object with the data contained by the field.

toLink sounds better indeed.

Let's do the rename then, and add the test coverage. #9 did not provide it.

It would also make sense to optionally allow setting a Link object as the LinkItem value. We have a value object, let's use it fully.

Added test for getLink method.

Hi there, this is basically a reroll of Sharique's #14.
Changed getLink() to toLink() as suggested in #11 and #12.
I notice that Entity has a toLink() as well, so that's better for consistency.

I think the test will fail, it did when I ran this locally.
I think that line 179 in LinkItemTest.php
$link = $entity->toLink();
... is referencing Entity toLink(); not LinkItem toLink();

@thlor thank you for setting that last patch to "needs review".

I think I have this working now ...

@Jigar.tanna there is no need to tag issues with "drupal core", I have seen you add this tag to a few issue today. Drupal.org issue are in projects. Since this one is in "Drupal core (3060)" that is what determines it deals with "drupal core".

By that way would be interested in your help on Outside In issues if you have the time and desire.

+++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
@@ -89,6 +90,12 @@ public function testLinkItem() {
+    $this->assertTrue($link->getUrl()->toString(), $url, 'Link has expected url.');
+    $this->assertTrue($link->getText(), $title, 'Link has expected title.');

Nitpick which can be fixed on the commit: The order for these for these parameters are: $expected_value, $actual_value
This gives you some warm and readable output when this test might ever break.

Thanks, Daniel. New patch attached.

Although, I notice that assertTrue() actually only has two parameters ... at least from what I see here in phpstorm ...

https://www.drupal.org/files/issues/2752187-1.png

https://www.drupal.org/files/issues/2752187-2.png

You are right about the message not existing in many phpunit assertions. These kind of messages don't add that many value, IMHO anyway.

Although, I notice that assertTrue() actually only has two parameters .

+++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
@@ -89,6 +90,12 @@ public function testLinkItem() {
+    $this->assertTrue($url, $link->getUrl()->toString(), 'Link has expected url.');
+    $this->assertTrue($title, $link->getText(), 'Link has expected title.');

This should use assertEquals instead :) There 3 parameters make more sense

Yes :). While I was at it, I removed the other deprecated methods in the test.

While I was at it, I removed the other deprecated methods in the test.

Hmm. I guess I shouldn't do that. Removing those changes, because they are not relevant to the current issue.

Hmm. I guess I shouldn't do that. Removing those changes, because they are not relevant to the current issue.

Exactly :) Please open up a new issue for that maybe. To be honest though it would be great to do those deprecation replacements in one big go. So one method but over the entirety of the codebase maybe.

Tested the patch #30 on 8.7 branch.
Tests passed.

Runtime: PHP 7.2.10 with Xdebug 2.6.1
Configuration: /data/app/core/phpunit.xml.dist

Testing
Test 'Drupal\Tests\link\Kernel\LinkItemTest::testLinkItem' started
Test 'Drupal\Tests\link\Kernel\LinkItemTest::testLinkItem' ended

Time: 1.68 minutes, Memory: 686.50MB

OK (1 test, 36 assertions)

This is a great addition, I could literally use this and the chained ::toRenderable on at least 5 client projects tomorrow!

But we need to advertise its availability with a change record.

Can someone create one heralding the new feature with much fanfare.

Thanks

Lee

What is the current status of this? Should I attempt to re-roll the patch with 8.9.x? I really need this and thought I was going crazy that I have a LinkItem and I couldn't find a way to get an actual, renderable link from it.

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 require as a guide.

Believe it or not it appears patch #30 still applies to 10.1.x cleanly. Just triggered the tests for this.

From reading the comments sounds like this was all but done but never got a change record so sending to NW for that.

Moving to NW for the change record please. Can mark this once that happens.

Omg! I follow this issue since probably six years.
I just went ahead and created the change record.

I checked the code too and it also looks RTBC to me, so setting the state to that.

Thanks for proposing this addition!

As mentioned in #3306201: Add a title getter to LinkItemInterface, adding new methods to an interface is a BC break. However, since the interface has a 1:1 relationship with the LinkItem class, method additions are allowed in minor releases. The change record should mention this BC break. (See my addition to https://www.drupal.org/node/3330231 for comparison.)

1. +++ b/core/modules/link/src/LinkItemInterface.php
   @@ -33,6 +33,14 @@
   +   * Creates a Link object from the field data.
   +   *
   +   * @return \Drupal\Core\Link
   +   *   Returns a Link object.
   +   */
   +  public function toLink();
   
   +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
   @@ -168,6 +169,13 @@ public static function mainPropertyName() {
   +  public function toLink() {
   

   Since this is a new method we're adding, it should have a return typehint. Reference:

   https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s...

2. +++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
   @@ -89,6 +90,13 @@ public function testLinkItem() {
   +    // Test the toLink method.
   

   toLink() should have params in docs.

3. +++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
   @@ -89,6 +90,13 @@ public function testLinkItem() {
   +    $this->assertInstanceOf(Link::class, $link, 'Link implements interface.');
   +    $this->assertEquals($url, $link->getUrl()->toString(), 'Link has expected url.');
   +    $this->assertEquals($title, $link->getText(), 'Link has expected title.');
   

   We can drop the assertion messages from these three assertions; core best practice has moved on to omitting them since this issue was first filed. :)

As with the other issue, we can also file a followup issue for the linked_entity_reference module to implement this method as well.

Created a new change record here (since I don't have access to edit original): https://www.drupal.org/node/3458241

On the feedback in #50:

1. Typehint added
2. I'm not sure how to add params to the interface, it does not support any parameters? Or maybe I misunderstand
3. Updated assertions to remove messages

Hide patches to avoid confusion with MR (and since this type of action tends to get ignored, anyone coming across this, please do not add a new patch, see this doc page for instructions).

Haven't reviewed yet but could issue summary be updated to the standard issue template please

Thank you!

Updated issue summary

I have checked this issue and issue summary. By calling the newly added toLink() method, I was able to get the link object as expected. I also verified the added test coverage and did not find any additional tags for this issue. I have rebased the MR since it was significantly behind, and the pipeline is now green. Moving this MR for review. Kindly review.

CR looks like it needs slight tweaking.

Versions seems off
Maybe a before of how users are retrieving this info currently

with an after of how this is easier.

i am working for CR Update

Updated the CR according to the feedback based on the issue summary also. Moving this for NR.
let me know if anything else need to update.

MR looks good. But should we add test coverage new method at Drupal\Tests\link\Unit\LinkFormatterTest?

RE: #63 I am starting on that. Used existing Unit test class for DRY reasons as so much setup for each test method. Have added use statement for Link class, Created a mock for it and set an expects() for the new toLink method.

Thanks, RTBC.

I read the IS, comments, the MR and the change record.

I see this is tagged 'Needs change record updates'. That should be resolved before setting an issue to RTBC, thanks.

Continuing to triage, I changed the order of a 'use' statement in the MR. I re-arranged the CR so the most important part, the way things work with this change, is first. I then adding a remaining task to the issue summary. That is an update to the change records, to the tag can remain. I didn't find any unanswered questions. Oh, and I updated credit.

Thanks!

• Updated the Change Record to match the BC note from https://www.drupal.org/node/3330231 - I added in a strong text to separate it from the other strong texts to keep appearing as a separate point.
• I removed that as a remaining task from the issue summary.

CR looks fine to me.

I did rebase the MR as it was 600+ commits back, after a few re-tries it's still green.

Believe all feedback has been addressed for this one.

Amending attribution.

Left some comments on the MR - I think we need to be defensive here for the edge cases like when getUrl throws an exception or the field-item is empty.

I addressed the feedback.

Ran the test-only test:

PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.13
Configuration: /builds/issue/drupal-2752187/core/phpunit.xml.dist
E                                                                   1 / 1 (100%)
Time: 00:00.928, Memory: 8.00 MB
There was 1 error:
1) Drupal\Tests\link\Kernel\LinkItemTest::testLinkItem
Error: Call to undefined method Drupal\link\Plugin\Field\FieldType\LinkItem::toLink()
/builds/issue/drupal-2752187/core/modules/link/tests/src/Kernel/LinkItemTest.php:98
ERRORS!
Tests: 1, Assertions: 5, Errors: 1.
HTML output directory sites/simpletest/browser_output is not a writable directory.
PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.13
Configuration: /builds/issue/drupal-2752187/core/phpunit.xml.dist
EFF                                                                 3 / 3 (100%)
Time: 00:00.027, Memory: 8.00 MB
There was 1 error:
1) Drupal\Tests\link\Unit\LinkItemTest::testToLink
Error: Call to undefined method MockObject_LinkItem_c54c6190::toLink()
/builds/issue/drupal-2752187/core/modules/link/tests/src/Unit/LinkItemTest.php:47
--
There were 2 failures:
1) Drupal\Tests\link\Unit\LinkItemTest::testToLinkWithEmptyField
Failed asserting that exception of type "Error" matches expected exception "InvalidArgumentException". Message was: "Call to undefined method MockObject_LinkItem_0a9ce685::toLink()" at
/builds/issue/drupal-2752187/core/modules/link/tests/src/Unit/LinkItemTest.php:64
.
2) Drupal\Tests\link\Unit\LinkItemTest::testToLinkWithBadUrl
Failed asserting that exception of type "Error" matches expected exception "InvalidArgumentException". Message was: "Call to undefined method MockObject_LinkItem_c54c6190::toLink()" at
/builds/issue/drupal-2752187/core/modules/link/tests/src/Unit/LinkItemTest.php:90
.
ERRORS!
Tests: 3, Assertions: 2, Errors: 1, Failures: 2.

I think this indicates the Unit test is working?

Seems feedback has been addressed with the addition of

if ($this->isEmpty()) {
      throw new \InvalidArgumentException('The field item is empty and cannot be converted to a link.');
    }

Test coverage for new exception added in testBadUrl()

I was looking at whether we could use this in core... and I was thinking something along the lines of:

We should file a follow-up that will eventually allow is get rid of a preprocess hook in at the same time...

• \Drupal\link\Hook\LinkThemeHooks::preprocessLinkFormatterLinkSeparate should trigger a deprecation is $variables['link'] is not set
• \Drupal\link\Hook\LinkThemeHooks::theme link should be added to the variables
• \Drupal\link\Plugin\Field\FieldFormatter\LinkSeparateFormatter::viewElements add link to the render array.

BUT!!!!!!

Then I noticed the following code in \Drupal\link\Plugin\Field\FieldFormatter\LinkSeparateFormatter::viewElements

      if (!empty($settings['trim_length'])) {
        $link_title = $link_title !== NULL ? Unicode::truncate($link_title, $settings['trim_length'], FALSE, TRUE) : NULL;
        $url_title = Unicode::truncate($url_title, $settings['trim_length'], FALSE, TRUE);
      }

Which makes this plan unworkable.

Given this and the fact that I can't work out where else to use this in core I wonder if adding this is actually necessary and whether other considers like trimming are always going to prevent it's usefulness?

Setting back to needs review to get an answer to that question.

Moving to NW as the MR appears to need a manual rebase, least gitlab isn't doing it right.

Also for an answer to #77, maybe sub-maintainer?

As the subsystem maintainer I don't have an answer to #77. I don't know that anyone had a plan to use this in Core. Reading back through the comments, it sounds like this was something that developers of downstream code wanted. No one offered use cases. But it seemed simple enough that I thought it might be a worthwhile addition.

I rebased the MR. I'm setting the status back to Needs Review.

Because this issue has opposing opinions from two framework managers (#37 and #77) I am tagging this for framework manager review.

from my point of view this is the companion to LinkItemInterface::getUrl() - I have a lot of custom code that uses getUrl and getTitle with a new Link so a utility to combine those is a useful addition in my opinion

Most of our projects have this in a trait EntityWithLinkFieldTrait that we add on a per bundle class case.

I think this is totally fine for @dcam as the subsystem maintainer to be the tie breaker.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.