Use link field for shortcut entity

Yeah, the link field is as fun as it is broken :)

Most of the brokenness you found was actually because you forgot to put a 'link__url' column in the schema ;) Another part of it was coming from the change in #2054011-70: Implement built-in support for internal URLs, because massageFormValues() doesn't run on programatic updates. I had to revert that hunk and write a proper fix.

- Translatability, I guess we need to integrate the link field type with entity translation sync so that we can make the title translatable?
- Or should we not use the title of the link field?

I'd prefer to not use the title property of the link field, mostly because I see it as a main property of the entity.

- Currently set to type general, using internal only would simplify a lot and pretty sure we want that, but didn't want to think about that yet, just applying patterns that I found elsewhere..

shortcut.module goes to great lengths to ensure that the links are internal only (@see shortcut_valid_link()), so we definitely want the internal links option.

- unserialize is ugly, but I have no idea how we can have passing tests right now, probably because we only use it as configurable field, which gets serialization handled properly?

Yup :/

Here's an updated patch that will fix most of the errors. The patch above didn't apply, so the interdiffs for shortcut.install and ShortcutFormController are not reliable.

That said, we should have only two relevant failures left:
- shortcut supported an empty path as the url by converting it automatically to the front page. We need to either bake that into the link field or drop this feature.
- the link field doesn't support aliases atm, that's being fixed in #2231413: Link field does not accept a valid path alias

Removed some debug leftovers and updated the label for this field.

I ran that test locally and it seems there's still a problem. I'll try to figure out a better solution because, unfortunately, moving that code to massageFormValues() is not the right fix..

Not sure if we have a test for deleting shortcuts but a manual test worked.

Ok, so the logic of the fix was correct, just the implementation was wrong. We should be left only with #2231413: Link field does not accept a valid path alias now.

1. 'url' is the main property because all the other properties can be derived from it.
2. Right, fixed.
3. A method that returns the render array for a shortcut sounds useful indeed, but it has to be tied to its link field (e.g. getLinkRenderArray()) since the entity type is fieldable and we don't want to duplicate the viewbuilder's job.. It might be useful to have this method on the LinkItem field type class too, so maybe we need to introduce an interface for it, but in a separate issue?

4. Do we want to keep the test but use ? Might need some special casing somewhere...

   I assume you meant <front> there, and yes, that's a good point. Fixed and it didn't need any special casing.

About menu links, I have no idea anymore how those will look like in D8, see #2178723: [meta-2] Finalize module-facing API of declaring menu links and the fourth child issue, so I prefer to not speculate on it at this point..

Sadly, we still depend on #2231413: Link field does not accept a valid path alias :(

Just re-rolled.

This should be better. Unfortunately, #2231413: Link field does not accept a valid path alias fixed the path alias problem at the wrong level...

Meh!

a) It's already removed in the patch, see LinkWidget::massageFormValues() :)
b) Sure, somethis like this?

updated issue summary to reflect this is nolonger blocked on #2231413: Link field does not accept a valid path alias

tried to identify remaining tasks. (please correct them, if I missed something)

I think ultimike's #2242779: Link Module - Add an optionless link test test (maybe just as a new method to the existing test) should be merged into this because of the change in link module is untested.

I looked at the patch from #2242779: Link Module - Add an optionless link test and the new test seems to be a verbatim copy of Drupal\link\Tests\LinkItemTest with some assertions commented out. What exactly is there to bring into this patch?

In addition to not doing some assertions,
it also commented out, in the test, setting of some options.

I think in that issue, we were hoping it would fail, to show that something needed to be fixed.

Ok, so we don't have anything to bring in here?

The test that chx referred to in comment 29 isn't done yet - my thinking is that we need a test that saves a new link using $entity->save() (not Form API) and then tests the rendering of the link (not just testing entity_load()).

-mike

No longer applies.

Straight reroll. A deleted file from the patch was renamed in another patch.

This is fine; probably needs a followup for testing the link cleanup; but that shouldnt be holding up a major task.

+++ b/core/lib/Drupal/Core/Url.php
@@ -122,6 +122,7 @@ public static function createFromPath($path) {
+        $path = \Drupal::service('path.alias_manager.cached')->getSystemPath($path);

In the vast majority of cases, this should already be using the system path. Should check $this->options['alias'] first. Otherwise this will add a lot of unnecessary queries.

If the caller doesn't know this, it should do the check, not Url itself.

Didn't review past that line.

<?php
$path = \Drupal::service('path.alias_manager.cached')->getSystemPath($path);
?>

Without that line, creating a shortcut to an alias fails testing.

39: 2235457-39.patch queued for re-testing.

getSystemPath() changed to getPathByAlias() in Core/Url.php.

Fixed the failure + added some documentation.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -122,6 +122,7 @@ public static function createFromPath($path) {
   +        $path = \Drupal::service('path.alias_manager.cached')->getPathByAlias($path);
            $result = \Drupal::service('router')->match('/' . $path);
   

   It is kind of sad that we have to do that, but yeah path alias handling is done on the request level. Should we maybe document that here?

2. +++ b/core/modules/shortcut/shortcut.install
   @@ -46,13 +43,27 @@ function shortcut_schema() {
   +      'link__url' => array(
   +        'description' => 'The URL of the link.',
   +        'type' => 'varchar',
   +        'length' => 2048,
   +        'not null' => FALSE,
   +      ),
   +      'link__route_name' => array(
   

   Is there really a need to store the URL, given that shortcuts will always link to internal paths.

3. +++ b/core/modules/shortcut/src/Tests/ShortcutTestBase.php
   @@ -57,7 +57,10 @@ function setUp() {
   +          'url' => 'node/add',
   
   @@ -65,7 +68,10 @@ function setUp() {
   +          'url' => 'admin/content',
   

   As written before, it would be nice to not require both.

#46: did you forget to upload the patch? :)

@dawehner, you forgot to upload the patch AFAICT. :)

Refactored the code a bit so we don't need to store or use the 'url' property when we're dealing with internal paths only, but, sadly, this means we're now blocked on #2232321: Limit validation errors on confirm form.

@catch: We don't have any $this->options there because Url::createFromPath() is a factory method that receives only one argument, a path string.

I did see the performance problem when I added that line but we don't really have a good way to fix it, because making the caller always ensure it's sending a system path would make it quite easy to overlook (like we (I) did when adding internal path support to the link field).

One way of making it better (perf wise) would be to try and resolve the alias only if the initial match didn't yield any result. That would mean something like this, query wise:

- if a system path was passed in => the initial match finds it => 1 query
- if an invalid alias was passed in => initial match fails, resolving the alias fails => 2 queries
- if a valid alias was passed in => initial match fails, alias is resolved, second match finds it => 3 queries

That's quite a pessimistic scenario so any other ideas are welcome :)

As suggested by @Berdir in #2232321: Limit validation errors on confirm form, let's just merge that patch in here.

Just a straight re-roll to try bring this issue back to life.

I was going to take a look at this but after the amount of effort it took to re-roll i'm just posting the patch to see what else might have broken. I'd be happy if someone a little more qualified in this area could pick the issue up.

Git!

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -200,17 +200,30 @@ public function validateTitle(&$element, FormStateInterface $form_state, $form)
   -        $url = \Drupal::pathValidator()->getUrlIfValid($value['url']);
   -        if (!$url) {
   -          return $values;
   -        }
   +        try {
   +          $parsed_url = UrlHelper::parse($value['url']);
    
   -        $value += $url->toArray();
   +          // If internal links are supported, look up whether the given value is
   +          // a path alias and store the system path instead.
   +          if ($this->supportsInternalLinks() && !UrlHelper::isExternal($value['url'])) {
   +            $parsed_url['path'] = \Drupal::service('path.alias_manager')->getPathByAlias($parsed_url['path']);
   +          }
    
   -        // Reset the URL value to contain only the path.
   -        if (!$url->isExternal() && $this->supportsInternalLinks()) {
   -          $value['url'] = substr($url->toString(), strlen(\Drupal::request()->getBasePath() . '/'));
   +          $url = Url::createFromPath($parsed_url['path']);
   +          $url->setOption('query', $parsed_url['query']);
   +          $url->setOption('fragment', $parsed_url['fragment']);
   +          $url->setOption('attributes', $value['attributes']);
   +
   +          $value += $url->toArray();
   +        }
   +        catch (NotFoundHttpException $e) {
   +          // Nothing to do here, LinkTypeConstraintValidator emits errors.
            }
   +        catch (MatchingRouteNotFoundException $e) {
   +          // Nothing to do here, LinkTypeConstraintValidator emits errors.
   +        }
   +
   

   Sorry but these changes are certainly wrong. We explicit introduced the path validator to handle the usecases here. In case it doesn't it has to be adapted.

2. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   @@ -107,37 +101,22 @@ public function setRouteName($route_name) {
   +  public function getUrl() {
   +    return new Url($this->link->route_name, (array) $this->link->route_parameters, (array) $this->link->options);
   

   Did we considered to add a getUrl method directly on the link item?

Rerolled, let's see how much fails.

Let's reroll and get it installed.

Let's see.

Some work ...

A bit less ...

With an interdiff ...

Number of times @dawehner clear the cache in tests to make em green is to damn high.

The amount of wasted time without doing patch reviews is crazy.

Let's fix some of the failures.

Mostly reviewed the link.module part for now.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -203,7 +203,7 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
   -    if (!parse_url($uri, PHP_URL_SCHEME)) {
   +    if ($uri != 'base://'  && !parse_url($uri, PHP_URL_SCHEME)) {
   

   Nothing I'm familiar with, but this could warrant a comment explaining why 'base://' deserves special consideration ?

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationTestBase.php
   @@ -176,6 +176,8 @@ protected function setupTestFields() {
   +    \Drupal::service('plugin.manager.field.widget')->clearCachedDefinitions();
   
   @@ -183,12 +185,14 @@ protected function setupTestFields() {
   +    \Drupal::service('plugin.manager.field.widget')->clearCachedDefinitions();
   ...
   +    \Drupal::service('plugin.manager.field.widget')->clearCachedDefinitions();
   

   It feels very weird that we need to clear the discovered widget plugins - and especially that often ?

   What causes the set of available widgets to change here ?

3. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
   @@ -164,9 +165,36 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +    $parsed_url = UrlHelper::parse(trim($this->url));
   +    $this->url = $parsed_url['path'];
   +    $this->options = ($this->options) ?: array();
   +
   +    // The link 'query' and 'fragment' parts need to be split and updated only
   +    // if they exist (e.g. when the 'url' property is new or updated).
   +    $options = array();
   +    if (!empty($parsed_url['query'])) {
   +      $options['query'] = $parsed_url['query'];
   +    }
   +    if (!empty($parsed_url['fragment'])) {
   +      $options['fragment'] = $parsed_url['fragment'];
   +    }
   +    $this->options = $options + $this->options;
   

   Hmm - such code is totally correct in the widget's massageFormValues(), but doesn't feel right here in LinkItem::preSave() :
   I don't think we want to support "$link_item->url can be either a raw url, or the 'path' part of a parsed url with ->options containing the parsed options", it should be one or the other ?
   (and thus it has to be the latter, since this is what the widget produces and what gets stored and loaded)

   I don't think allowing polymorphism on "what is found in an Item properties" is a good thing, since it means you de facto don't know what's in there and what you can do with the values at runtime.

   If needed, we can always add a $link_item->setPath($raw_url) as a helper for DX, that immediately expands into the single canonical shape for the url and options properties.

4. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
   @@ -164,9 +165,36 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +    if ($this->isExternal()) {
   +      return $this->url === NULL || $this->url === '';
   +    }
   +    else {
   +      return $this->route_name === NULL || $this->route_name === '';
   +    }
   

   '0' is moot for either url or root_name, right ?
   couldn't it be just
   return empty($this->route_name) && empty($this->url); ?

5. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
   @@ -183,4 +211,19 @@ public function isExternal() {
   +  public function setValue($values, $notify = TRUE) {
   +    // Unserialize the values.
   +    // @todo The storage controller should take care of this.
   

   It should indeed - I thought we had support for 'serialized' columns in the field schemas (I think the image field type also has a serialized column (sorry, can't check atm), would be worth checking how it's done there ?)
   And if not, we should totally open a followup and link it there.

6. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -200,18 +205,40 @@ public function validateTitle(&$element, FormStateInterface $form_state, $form)
   +          // If internal links are supported, look up whether the given value is
   +          // a path alias and store the system path instead.
   +          if ($this->supportsInternalLinks() && !UrlHelper::isExternal($value['url'])) {
   +            $parsed_url['path'] = \Drupal::service('path.alias_manager')->getPathByAlias($parsed_url['path']);
   +          }
   

   Hm - that, on the other hand, might belong to LinkItem::preSave() ?

   Or is accepting aliases as input is only a feature of the widget, and is not supported on runtime LinkItem value ? Yeah, could make sense I guess.

   Anyway - again, it's a question of having clear and unambiguous semantics for the properties in a runtime LinkItem object.

Thanks @yched for the review, though this is for now just a reroll.

I just want to engage the bot on this. Need to still address @yched points.

Let's do just the reroll.

needs a beta evaluation (especially since this is a task)

At least related #2315773: Create a menu link field type/widget/formatter

does this issue close #2277753: Shortcuts should handle missing routes elegantly ?

We should possibly change course based on #2407505: [meta] Finalize the menu links (and other user-entered paths) system

At least, for shorcut we want to store the route info, for link field just the user-entered path

If we just want route info, then do we want a PathElement based widget?

Takes path, converts to route/route name pair?

There's been more discussion on #2407505: [meta] Finalize the menu links (and other user-entered paths) system.

Once the link field is applied to menu links, it makes sense to apply it to shortcuts too. This reverses the path vs. route storage decision but means we have the same storage model and expectations everywhere.

Tagging D8 upgrade path since this will require a schema change. Undecided on whether this is critical or not so leaving at major for now. If we don't treat it as critical, we need to discuss whether we'd go ahead with it after the upgrade path is supported with a hook_update_N() - mostly trying to avoid that though.

Once the link field is applied to menu links, it makes sense to apply it to shortcuts too. This reverses the path vs. route storage decision but means we have the same storage model and expectations everywhere.

And sanity is restored! Nothing more I could wish for :)

I think this now blocks #2403847: Shortcut content entity validation misses form validation logic which is critical - so does that in turn make this critical? or do we have enough criticals tracking this space.

I think this now blocks #2403847: Shortcut content entity validation misses form validation logic which is critical - so does that in turn make this critical? or do we have enough criticals tracking this space.

Enough yes, but afaik that's the way it works :/

Updated summary.

Let's wait until #2411143: Change LinkItem schema to store URIs rather than URLs/paths/routes is in ... these potentially fixes a lot of the problems we had earlier here with various route related issues.

Unpostpone it.

Currently working on a "reroll", let's see how far we can get.

Here is a first version.

One question I came up with: Where should the title of a shortcut be stored ... does that belong onto the link or not.

Fixed the failures.

Attempting to fix one of the failures, just so that I understand this better.

This will not fix the last failure but I am just changing a test and I thought it best to do that independently. I will attach the patch with the fix shortly after this. I will set it to Needs Review with that patch.

This should fix the last two failures, but it is not done yet. The fix uses the Url::getInternalPath() method which is deprecated. The trouble is I can't find any substitute for this method except of inlining the function code (which might be wasteful in a test, especially if there is a ready substitute method).

We could also let this go in and let the code that removes the deprecated calls handle this. It is already used in over a dozen places.

Any suggestions?

Ah, all green.

Going up the comments, I see that the last review comments appeared in #85, but I am not sure if that is still valid. @yched, can you check again please? I know that at least one of the review comments were addressed in one of the patches since then.

Awesome,
but the Url::getInternalPath() is needs to be removed as deprecated function. So this can be achieved in similar way.

\$shortcut->getUrl()->getPathFromRoute($shortcut->getRouteName(), $shortcut->getRouteParameters()), '/'))\

Can be used as substitute.

@RavindraSingh: Can you give an interdiff for your patch? Is that the only line you have changed?

@RavindraSingh: I did mention about this deprecated method in #110. I agree that it should be removed but I don't know if it is a good idea to block a critical on it. If we can fix it, great! But it doesn't seem that simple, not without adding many lines for this trivial use case.

Besides, there are lots and lots of places where this method is used. This could just be another instance to change when we deal with that issue. I am not convinced that the snippet you have given is actually the best way to substitute this. I have put in all the places where this method is used in #2413701-1: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs.

I would suggest that if we can quickly figure out a good substitute, we change it. Otherwise, we worry about the substitute in #2413701: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs and not block this critical. I still suggest the patch in #110. If everyone agrees, I will reupload it.

I was trying out the snippet in #112 and realized that the method getPathFromRoute() is also deprecated. This means that if we can't use getInternalPath(), we can't use getPathFromRoute() either.

I am reuploading the patch from #110 to keep things clear until we find a better substitute. Else, I still suggest in dealing with that in #2413701: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs.

Let's keep it simple :)

Hahaha... Never realized those were public. :)

@hussainweb
Really good catch in https://www.drupal.org/files/issues/interdiff-107-109_0.txt

This patch is absolutely beautiful. If I were emotionally attached to Shortcut module, I'd cry!

I only found nitpicks, hence keeping at NR.

1. +++ b/core/modules/content_translation/src/Tests/ContentTranslationTestBase.php
   @@ -176,18 +176,22 @@ protected function setupTestFields() {
   +    \Drupal::service('plugin.manager.field.widget')->clearCachedDefinitions();
   +
        entity_create('field_storage_config', array(
          'field_name' => $this->fieldName,
          'type' => 'string',
          'entity_type' => $this->entityTypeId,
          'cardinality' => 1,
        ))->save();
   +    \Drupal::service('plugin.manager.field.widget')->clearCachedDefinitions();
        entity_create('field_config', array(
          'entity_type' => $this->entityTypeId,
          'field_name' => $this->fieldName,
          'bundle' => $this->bundle,
          'label' => 'Test translatable text-field',
        ))->save();
   +    \Drupal::service('plugin.manager.field.widget')->clearCachedDefinitions();
        entity_get_form_display($this->entityTypeId, $this->bundle, 'default')
   

   I think we can just keep the last plugin cache clearing call?

2. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
   @@ -164,4 +164,28 @@ public function isExternal() {
   +    // @todo The storage controller should take care of this, see
   +    //   SqlCOntentEntityStorage::loadFieldItems.
   

   This still needs to happen in this issue?

3. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   @@ -12,13 +12,14 @@
   + * @property \Drupal\link\LinkItemInterface link
   

   Is this a new annotation thing? Have not seen this before.

4. +++ b/core/profiles/standard/standard.install
   @@ -22,18 +22,27 @@ function standard_install() {
   -  \Drupal::configFactory()->getEditable('system.site')->set('page.front', 'node')->save();
   +  \Drupal::configFactory()
   +    ->getEditable('system.site')
   +    ->set('page.front', 'node')
   +    ->save();
    
      // Allow visitor account creation with administrative approval.
      $user_settings = \Drupal::configFactory()->getEditable('user.settings');
   -  $user_settings->set('register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL)->save();
   +  $user_settings->set('register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL)
   +    ->save();
    
      // Enable default permissions for system roles.
      user_role_grant_permissions(DRUPAL_ANONYMOUS_RID, array('access comments'));
   -  user_role_grant_permissions(DRUPAL_AUTHENTICATED_RID, array('access comments', 'post comments', 'skip comment approval'));
   +  user_role_grant_permissions(DRUPAL_AUTHENTICATED_RID, array(
   +    'access comments',
   +    'post comments',
   +    'skip comment approval'
   +  ));
    
      // Enable all permissions for the administrator role.
   -  user_role_grant_permissions('administrator', array_keys(\Drupal::service('user.permissions')->getPermissions()));
   +  user_role_grant_permissions('administrator', array_keys(\Drupal::service('user.permissions')
   +    ->getPermissions()));
   

   These are unnecessary changes; they don't actually change anything, they just reformat existing code.

   Fine to keep though.

I think we can just keep the last plugin cache clearing call?

Not sure whether this was maybe just some sort of desperation.

This still needs to happen in this issue?

Opened an issue for that. A new critical issue in entity storage is a good start into the day: #2414835: SqlContentEntityStorage does not unserialize base field data

Is this a new annotation thing? Have not seen this before.

It is IMHO the best thing since sliced bread. It adds DX, because it explains, which properties exists, instead of of having nothing.

Not sure whether this was maybe just some sort of desperation.

:D

Manually tested, still works fine. RTBC!

Two simple whitespace fixes in this reroll.

dawehner had an excellent remark in IRC:

14:20:05 dawehner: WimLeers: do we want to store the title on the shortcut and not on the link field?
14:20:19 dawehner: WimLeers: so the patch stores the title still on the entity itself
14:20:26 dawehner: which is needed for entity label and what not
14:20:40 WimLeers: That is a great, great question.
14:20:44 WimLeers: I should've thought of that.
14:20:48 WimLeers: dawehner++
14:21:06 WimLeers: dawehner: is there any technical reason why it cannot live on the link field?
14:21:49 dawehner: WimLeers: well ... for the label() you can't
14:21:55 WimLeers: dawehner: that's what I thought
14:21:57 dawehner: WimLeers: it would also change the UI potentially
14:22:03 WimLeers: definitely

Plus amateescu's comment in #3:

I'd prefer to not use the title property of the link field, mostly because I see it as a main property of the entity.

IOW: anything that is a semantically essential field of an entity, should be an actual base field itself. Hence keeping this at RTBC.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
   @@ -118,4 +118,12 @@ public function save(array $form, FormStateInterface $form_state) {}
   +    // Overwrite the default validation implementation as it is not necessary
   

   Override :)

2. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   @@ -64,8 +65,8 @@ public function getTitle() {
      public function setTitle($link_title) {
   -    $this->set('title', $link_title);
   -   return $this;
   +    $this->link->set('title', $link_title);
   +    return $this;
   

   Based on the discussion above, this seems wrong.

3. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   @@ -224,16 +174,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -    $item_definition->setClass('\Drupal\shortcut\ShortcutPathItem');
   

   I don't see where we remove this class in the patch. Same for ShortcutPathValue.

4. +++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
   @@ -37,42 +40,39 @@ public function testShortcutLinkAdd() {
   -      if (strpos($entity->route_name->value, 'node.') === 0) {
   -        $entity->save();
   -        $loaded = Shortcut::load($entity->id());
   -        $this->assertEqual($entity->route_name->value, $loaded->route_name->value);
   -        $this->assertEqual($entity->get('route_parameters')->first()->getValue(), $loaded->get('route_parameters')->first()->getValue());
   -      }
   +      $entity->save();
   +      $loaded = Shortcut::load($entity->id());
   +      $this->assertEqual($entity->link->options, $loaded->link->options);
   

   We should assert more fields/properties like before, not just ->link->options :)

5. +++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
   @@ -81,15 +81,15 @@ public function testShortcutLinkAdd() {
   -    $this->assertRaw(t('The shortcut must correspond to a valid path on the site.'));
   +    $this->assertRaw(String::format('The URL %url is not valid.', ['%url' => 'admin']));
   

   Afaik, messages that might be translated in the UI are also asserted with t().

6. +++ b/core/modules/shortcut/src/Tests/ShortcutTestBase.php
   @@ -62,18 +62,24 @@ protected function setUp() {
   +          'options' => [],
   ...
   +          'options' => [],
   

   Shouldn't the default be an empty array? Why do we need to set it specifically?

7. +++ b/core/profiles/standard/standard.install
   @@ -58,7 +58,7 @@ function standard_install() {
   +    'link' => array('node/add'),
   
   @@ -66,7 +66,7 @@ function standard_install() {
   +    'link' => array('admin/content'),
   

   Here we rely on mainPropertyName(), how about passing an array with the 'uri' key?

Good points!

Note for other reviewers: #125.3 is not in the interdiff but it is included in the patch.

I worked a lot on this patch many months ago but I think it received enough updates and reviews from other people so setting back to RTBC :)

Note for other reviewers: #125.3 is not in the interdiff but it is included in the patch.

Damn, I suck at git.

+++ b/core/modules/shortcut/src/Entity/Shortcut.php
@@ -205,13 +146,22 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['link'] = BaseFieldDefinition::create('link')
+      ->setLabel(t('Path'))
+      ->setDescription(t('The location this shortcut points to.'))
+      ->setRequired(TRUE)
+      ->setTranslatable(FALSE)

It was discussed above that even though the link field has a title (and a description following #2413029: Change LinkItem schema to also store a description), that is not being used because the entity needs a label. Why is the field kept translatable then? Do we expect to support links different per language in a shortcut? The similar menu link patch at #2406749: Use a link field for custom menu link does not make the link field translatable.

Less shortcut specialness - yay. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f2fa574 and pushed to 8.0.x. Thanks!

I misread that. setTranslatable(FALSE) is fine. That is the default, so we don't do setting it to FALSE anywhere other than 2 tests, so it would be nice to remove.

Why is the field kept translatable then? Do we expect to support links different per language in a shortcut?

But the code already says setTranslatable(FALSE) — so the link field isn't translatable.

Discussed with Gábor in chat. setTranslatable(FALSE) is the default. Hence it's opt-in, and he misread it precisely because it's opt-in: presence of a setTranslatable() call at all typically implies it's translatable.

So, rectifying that detail in #2415129: Small clean-up for Shortcut entity's usage of link field.

I am uploading a patch to address #134 and setting that to Needs review. I know it is not critical and is already default, so wouldn't break anything, but it would be nice to keep this consistent.

#136: that's very helpful — thank you! — but we don't post follow-up patches after the patch for an issue has been committed. I've already filed a follow-up issue with the exact same patch, as you can see in #135 :)

@Wim Leers: We must have submitted at the same time. I saw your comment once the page reloaded. :)

Late to the party, but I could indeed cry at the amount of weirdness that was removed here :-)

I too was wondering why the Shortcut entity preserves a separate 'title' field and keeps the 'title' property in its LinkItem unused, but I see this has been discussed at length above. Just, since it seems a couple people raised the topic, maybe it would be worth writing down the reason in Shortcut::baseFieldDefinitions() ?
(also - Shortcut::label() could return $this->link->title, couldn't it ?)

Other than that :

+++ b/core/modules/shortcut/src/Entity/Shortcut.php
@@ -12,13 +12,14 @@
 /**
  * Defines the shortcut entity class.
  *
+ * @property \Drupal\link\LinkItemInterface link
+ *
  * @ContentEntityType(

I thought that was unintentional, then I saw #121 / #122 :

- Is this a new annotation thing? Have not seen this before.
- It is IMHO the best thing since sliced bread. It adds DX, because it explains, which properties exists, instead of of having nothing.

Hmm, why not on principle, but that indication is incorrect, $shortcut->link is a FieldItemList, not a LinkItem ?

@yched, the discussion here was not really at length, see #2406749-17: Use a link field for custom menu link and below if you want to join the fun.

it would e.g. require a shortcut entity query selection plugin, if you'd want to support autocomplete.

@Berdir, or we could do the second to last paragraph from #2406749-25: Use a link field for custom menu link. Not saying that we have to, I'm just trying to lay down all the possible options.

We kind of broke every D8 install in a way that I'm not sure is even fixable manually :( #2415633: Fatal error: Class 'Drupal\link\LinkItemInterface' not found in core/modules/shortcut/src/Entity/Shortcut.php on line 156

I'd also like to see us use the associated Link field title column as long as translation will work - otherwise the link field vs shortvut and menu ink will potentially remain somewhat divergent.

@pwolanin: I believe that would need #2403455: FieldTranslationSynchronizer does not sync columns that are absent from any column_group in the annotation (which IMHO is not a problem) to be able to translate title/description but not the rest.

Interesting to see LinkTypeConstraint being a single class, as suggested by #2226821: Combine Drupal Constraint and ConstraintValidation classes into one a while ago. However, looking at it I'm not sure it's something we should practice, let's discuss at #2226821: Combine Drupal Constraint and ConstraintValidation classes into one

patch #1 here has code for default_value and max_length = 560 but that is dead code.
#2417809: link and shortcut have baseFieldDefinition settings that do not do anything: default_value max_length