Add resolvable_uri property to LinkItem for APIs

Thanks for filing this issue! 🙏

From MenuLinkContentHalJsonAnonTest:

Expected

 'link' => Array &22 (
        0 => Array &23 (
            'options' => Array &24 (
                'attributes' => Array &25 (
                    'class' => Array &26 (
                        0 => 'example-class'
                    )
                )
                'fragment' => 'a-fragment'
            )
            'title' => null
            'uri' => 'https://nl.wikipedia.org/wiki/Llama'
        )
    )

Actual

  'link' => Array &22 (
        0 => Array &23 (
            'options' => Array &24 (
                'attributes' => Array &25 (
                    'class' => Array &26 (
                        0 => 'example-class'
                    )
                )
                'fragment' => 'a-fragment'
            )
            'title' => null
            'uri' => 'https://nl.wikipedia.org/wiki/Llama'
            'url' => 'https://nl.wikipedia.org/wiki/Llama#a-fragment'
        )
    )

This means it's working! 🥳

If you update the existing tests' expectations, we'll have fewer failures.

Add a computed url property at \Drupal\link\Plugin\Field\FieldType\LinkItem::propertyDefinitions()

I do think adding a computed field to LinkItem is not the right approach here. \Drupal\file\Plugin\Field\FieldType\FileUriItem is already doing that.

This problem also exsits for \Drupal\file\Plugin\Field\FieldType\FileUriItem and \Drupal\Core\Field\Plugin\Field\FieldType\UriItem both have properties with \Drupal\Core\TypedData\Plugin\DataType\Uri DataType.

We should update ::getValue method of \Drupal\Core\TypedData\Plugin\DataType\Uri DataType so that if parent has the external true option/setting set then it should provide an external URL.

#7: yay, number of failures cut in half! 🥳

Like @justafish, I'm not following @jibran's comment.

My suggestion is to not add URL field at all and use uri property to provide URL when \Drupal\Core\TypedData\Plugin\DataType\Uri is serialized.
Something like this in \Drupal\Core\TypedData\Plugin\DataType\Uri

  /**
   * {@inheritdoc}
   */
  public function getValue() {
    $item = $this->getParent();
    $options = $item->option; // incase it is LinkItem
    $options = $item->{($this->definition->getSetting('options'))} // incase of UriItem
    $absolute = $options['absolute'] ?? FALSE;
    if ($absolute) {
      // Create URL here and return.
    }
    return $this->value;

We can also remove URL field from FileUriItemand implement __get and __set to porvide BC here.

Updated the patch.

• Added \Drupal\link\Plugin\DataType\LinkUrlComputed::setValue() method along with implmenting CacheableDependencyInterface.
• Also added \Drupal\link\Plugin\Field\FieldType\LinkItem::onChange() to keep the field values in sync with computed property just ERItem.
• Add test for all this.

Reroll.

This looks great! I only have nits:

1. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
   @@ -172,6 +180,23 @@ public function getUrl() {
   +    if ($property_name == 'url') {
   

   Nit: ===

2. +++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
   @@ -172,6 +174,31 @@ public function testLinkItem() {
   +    $this->assertEquals($url, $entity->field_test->url);
   +    $this->assertEqual($entity->field_test->uri, $parsed_url['path']);
   

   Why sometimes assertEqual(), sometimes assertEquals()? 🤔

Somehow I have a problem with this portion of code

+    $properties['url'] = DataDefinition::create('link_url')
+      ->setLabel(t('URL'))
+      ->setDescription(t('The processed URL for this link that can e.g. be used in in href attributes.'))
+      ->setComputed(TRUE)
+      ->setInternal(FALSE)
+      ->setReadOnly(TRUE);
+

It seems to override what twig do so in my twig template if I call a field.0.url.routename it returns null when before I had a route name like entity.node.canonical.
Changing it to

+    $properties['_url'] = DataDefinition::create('link_url')
+      ->setLabel(t('URL'))
+      ->setDescription(t('The processed URL for this link that can e.g. be used in in href attributes.'))
+      ->setComputed(TRUE)
+      ->setInternal(FALSE)
+      ->setReadOnly(TRUE);
+

fixed the problem.
Sorry I have not much time to dig more

So field.0.url is calling \Drupal\link\Plugin\Field\FieldType\LinkItem::getUrl()?

Just tested this patch on my 8.8 setup. Works like a charm

#22: can you reproduce that problem with just Drupal core? Or do you have some custom Twig-enhancing modules installed? Because I don't see how field.0.url.routename can do anything with just Drupal core?

can you reproduce that problem with just Drupal core?

Yes, because field.0.url.routename translates to $field->get(0)->getUrl()->getRouteName().

Indeed, field.0.url.routename can translate to $field->get(0)->getUrl()->getRouteName(). This has been explained here: https://twig.symfony.com/doc/3.x/templates.html#variables. In the template the code could be changed to {{ field.0.getUrl.routename }} which should work with the patch.

#27: But it sounds like you would consider this a BC break, right?

I'm on the fence about this. Based on https://www.drupal.org/core/d8-frontend-bc-policy#render-arrays, I probably wouldn't consider this as a BC break, but I would still provide a change record to warn people about this change. I think it would be good to get an opinion from a release manager on this.

The patch #19 works as expected, but it would be nice, to have an option to get always absolute URLs instead of relative.

This one is the same as 19, just removed the git binary part at the very beginning of the patch.
It was failing:

  - Installing drupal/core (8.8.5): Loading from cache
  - Applying patches for drupal/core
    https://www.drupal.org/files/issues/2020-02-29/3066751-19.patch (3066751 - `link` fields in REST, JSON:API and GraphQL cannot be rendered by client)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-02-29/3066751-19.patch

patch -p1 < ~/Downloads/3066751-19.patch 
File core/modules/jsonapi/tests/src/Functional/MenuLinkContentTest.php: git binary diffs are not supported.
patching file core/modules/jsonapi/tests/src/Functional/ShortcutTest.php
patching file core/modules/link/src/Plugin/DataType/LinkUrlComputed.php
patching file core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
patching file core/modules/link/tests/src/Kernel/LinkItemTest.php
patching file core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php
patching file core/modules/shortcut/tests/src/Functional/Rest/ShortcutResourceTestBase.php

Here is an updated patch that can be used in conjunction with the patch from issue #2915792: MenuLinkContentAccessControlHandler does not allow "view" access without admin permission, making these entities inaccessible via REST, JSON API and GraphQL and entity reference fields.

My composer.json has the patches in this order.

"2915792": "https://www.drupal.org/files/issues/2020-05-13/2915792-47.patch",
"3066751": "https://www.drupal.org/files/issues/2020-05-19/3066751-32-with_issue_2915792.patch"

Rerolled #19 and addressed #21.

This will fix some fails.

This fix remaining fails.

Code looks good to me, thorough tests and great commentary. Just up in the air if we need a test-only patch or not. Otherwise RTBC from me!

I'll add my comment from slack here.

This patch adds new URL property to LinkItem and test-only-patch will error out without that change so it doesn't make sense to have a test only patch.

The one thing you can ask about it is missing test coverage of a new feature which I think is covered by this code block.

+++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
@@ -172,6 +177,38 @@ public function testLinkItem() {
+    // Test setting up computed property also sets up other values.
+    $entity = EntityTest::create();
+    $url = 'https://www.drupal.org?test_param=test_value#top';
+    $parsed_url = UrlHelper::parse($url);
+    $entity->field_test->url = $url;
+    $this->assertEquals($parsed_url['path'], $entity->field_test->uri);
+    $this->assertEquals([
+      'query' => $parsed_url['query'],
+      'fragment' => $parsed_url['fragment'],
+      'external' => TRUE,
+    ], $entity->field_test->first()->getUrl()->getOptions());
+    $entity->name->value = $this->randomMachineName();
+    $entity->save();
+
+    // Verify that the field value is changed.
+    $id = $entity->id();
+    $entity = EntityTest::load($id);
+    $this->assertInstanceOf(FieldItemListInterface::class, $entity->field_test);
+    $this->assertInstanceOf(FieldItemInterface::class, $entity->field_test[0]);
+    $this->assertEquals($url, $entity->field_test->url);
+    $this->assertEquals($entity->field_test->uri, $parsed_url['path']);
+    $this->assertEquals($entity->field_test[0]->uri, $parsed_url['path']);
+    $this->assertEquals($entity->field_test->options['query'], $parsed_url['query']);
+    $this->assertEquals($entity->field_test->options['fragment'], $parsed_url['fragment']);
+
+    // Check that if we only set url the default values for uri, title, and
+    // options are also initialized.
+    $entity->field_test = ['url' => '/node/add'];
+    $this->assertEqual($entity->field_test->uri, 'internal:/node/add');
+    $this->assertEqual($entity->field_test->url, '/node/add');
+    $this->assertNull($entity->field_test->title);

so IMO we are good here from testing POV.

As per #39, also reviewed by @Sam152

Re-rolled

@Krzysztof Domański patch #38 still applies cleanly, no need to reroll.

The change record doesn't really address #22/ #27 and there's not been the request release manager about the potential BC break. I think the fact that someone has managed to have this problem and report it on the issue before we've even committed it should give us concern.

Will this change break {% set bundle_attributes = bundle.add_linkgetOption('attributes') ?: {} %} in core/themes/claro/templates/entity-add-list.html.twig?

Looking at http://codcontrib.hank.vps-private.net/search?text=.url.&filename= we've got:
http://codcontrib.hank.vps-private.net/node/29411413
http://codcontrib.hank.vps-private.net/node/29424660
http://codcontrib.hank.vps-private.net/node/29427291
http://codcontrib.hank.vps-private.net/node/29454278
On the first page.

This looks disruptive in its current form. Perhaps we need another name? :(

It is disheartening to see #45 after a month long RTBC. 🤐

@jibran sorry about that. As I'm sure you're aware the rtbc queue has been very long for a while but we're finally getting in under control.

The comments referred to in #45 were posted more than two months before the issue was RTBCed, and were not addressed. When there are unaddressed comments on an issue (even if they were rejected for a good reason), and there is not a clear resolution, this requires a lot of additional time for committers to read through and figure out whether they were valid or not, which slows down progress in the RTBC queue overall. Anyone involved in the issue can ensure that all comments are addressed (and ideally mentioned in the issue summary if they're significant) whether the issue is RTBC or not

Thanks for the reply @alexpott. I really appreciate all the work you and the rest of committer team is doing to bring the RTBC queue down.

I'm going to put the issue in NR so that release managers can review.

@jibran looking at the number of affected templates I think we need another plan - or at least to try to come up with another option.

Patch #38 appears to have solved the issue for me. Thank you!

This is a version of patch #38, applied on top of the patch for #2915792: MenuLinkContentAccessControlHandler does not allow "view" access without admin permission, making these entities inaccessible via REST, JSON API and GraphQL and entity reference fields

Fixed a test and added referenced patch number to patch file

Reroll

Ignore above, forgot the LinkUrlComputed.php

Re-roll patch created for 9.3.x.

Fixed fail tests, Please have a look.

Fixed some cs issues.

Is this is a duplicate of #2942851: Link enhancer to add Aliased URL for internal links?

@Vacilando Fyi, I tested #2942851: Link enhancer to add Aliased URL for internal links and it didn't add url for paragraphs (only nodes) but this core patch added it to paragraph fields.

Example...

Before:

"field_para_link": {
"uri": "entity:node/8",
"title": "View All Products KP",
"options": []
},

After:

"field_para_link": {
"uri": "entity:node/8",
"url": "/dea/products",
"title": "View All Products KP",
"options": []
},

This patch is working very well for me on 9.3.5.

Thanks!

Unfortunately, this is not ready we need to address #22 as stated in #45.

How about we change url to full_url?

Here is a patch addressing #22 using #67 suggestion. I also fixed some CS on the patch code so it don't introduce new phpcs errors on the core.

Thanks for the patch. Let's update the change record and IS before we finish reviews and get this RTBC.

Removing "Needs release manager review" tag as we have addressed the points raised in #45.

IS updated.
I also updated the change record, but didn't remove the related tag as I'm not sure the change record is 100% correct now. I'll wait for a second review on the CR (https://www.drupal.org/node/3143736).

I'm a bit confused about switching things to full_url which makes me think it's an absolute (with domain) link but it can be absolute or relative. What is the reasoning with calling it full_url instead of url?

+++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
@@ -150,7 +150,7 @@
-    $this->assertEquals($entity->field_test->url, '/node/add');
+    $this->assertEquals($entity->field_test->full_url, '/node/add');

@Kristen Pol
The name change from url to full_url was to address the problem raised by #22

It seems to override what twig do so in my twig template if I call a field.0.url.routename it returns null

and confirmed by #26 and #27

It doesn't need to be full_url but has to be something different from just url. I just used the name suggestion from @jibran on #67. :)

I tried the patch on #62 and #68 but it didn't work for me. We're running Drupal 9.3.13. I will have to admit I'm new to using the JSON API to retrieve data.

My array inside the field_link inside a paragraph is still the following (when I console.log):

field_link:
	options: []
	title: "Request Changes"
	uri: "internal:#"

How are other people getting the value from field links? Do they work in fields on nodes and not on paragraphs?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Making the patch compatible with latest Drupal 10.3 version.

Hiding patches for clarity

MR should be pointed at 11.x as the current development branch
MR also appears to have some test failures

Previously tagged for CR updates which appear to be needed as well.

Thanks

#79 is not applying for me on 10.3.1 NVM- #79 applies for me on 10.3.1 when I use depth 3

Same patch as #79 with depth 2.

This is a patch that is intended for those who need to support both "url" and "full_url" while decoupled front ends make this change.

I see from the comments that naming the property full_url wasn't necessarily a popular choice. I'm not fond of it either. It isn't descriptive or meaningful.

A resolvable, or dereferenceable, URI is a URI that actually points to a functioning website.

I renamed the property from full_url to resolvable_uri. I wouldn't mind resolvable_url, but I decided to stay with "uri" to keep the association with the original uri property. Feel free to provide feedback or other suggestions.

I recreated the MR for Drupal 11. The original MR was hidden and a new one was created because usually changing the MR version breaks everything. Recreation is often the easiest way forward. I had to fix an issue with the JSON:API ShortcutTest. There's a comment on the MR about it.

The issue summary and change record have been updated.

Left a comment on the MR.

I responded to the feedback.

I also updated the issue summary with information about the ability to set the field item's value by assigning the resolvable_uri property.

I appreciate the work everyone is doing on this, and I don't want to be a complainer- but the repeated name changes are excrutiating. When you have dozens of front end applications relying on "url" getting them to change to "full_url" and redeploy their applications was painful. Now we have everybody using "full_url" and we are moving once again to "resolvable_uri." This is not a trivial change if you have a large number of production apps already relying on full_url. I am not seeing why resolvable_uri is an improvement.

Patch for those who need to support full_url and resolvable_uri for some time (P2).

Believe feedback for this one has been addressed. Not sure about full_url vs resolvable_uri but do agree didn't seem like full_url was heavily agreed upon. See no issue with @dcam proposal

The "Needs release manager review" tag was removed in #69, but I did not see a release manager sign off, so adding the tag back.