Allow entity: URIs to be entered in link fields

Minor :

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -60,6 +61,18 @@ protected function getUriAsDisplayableString($uri) {
   +  public function elementValidate($element, FormStateInterface $form_state, $form) {
   

   Can we make that a static method, and add it as '#element_validate' => [[get_class($this), 'elementValidate']] ?
   Nicer on form serialization :-)

   (would mean the getUserEnteredStringAsUri() helper has to be static as well, but AFAICT it can ?)

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -219,19 +231,34 @@ public function validateTitle(&$element, FormStateInterface $form_state, $form)
   +  protected function getUserEnteredStringAsUri($uri) {
   

   Can we move that one next to the existing getUriAsDisplayableString() ? they look kind of related :-)

3. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -244,15 +244,6 @@ protected function actions(array $form, FormStateInterface $form_state) {
   -  public function validate(array $form, FormStateInterface $form_state) {
   
   @@ -287,23 +278,4 @@ public function save(array $form, FormStateInterface $form_state) {
   -  protected function doValidate(array $form, FormStateInterface $form_state) {
   

   Oh yeah !

Also, really minor :

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -219,19 +231,34 @@ public function validateTitle(&$element, FormStateInterface $form_state, $form)
+  protected function getUserEnteredStringAsUri($uri) {

getFooAsBar($bar) is a bit weird - that $uri param is not a URI yet, that's the whole point :-)
--> s/$uri/$string/ ? $input ?

Wim left the sprint and is on his way back home, so I'll finish fixing the remaining failures.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -60,6 +61,32 @@ protected function getUriAsDisplayableString($uri) {
   +      // Disallow external URLs using untrusted protocols.
   +      $disallowed = $disallowed || ($url->isExternal() && !in_array(parse_url($uri, PHP_URL_SCHEME), \Drupal::config('system.filter')->get('protocols')));
   

   I was wondering whether we should introduce UrlHelper::getDangerousProtocols and use it, which calls out to static::$allowedProtocols

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -60,6 +61,32 @@ protected function getUriAsDisplayableString($uri) {
   +      if ($disallowed) {
   +        $form_state->setError($element, $this->t('The URL %uri is not valid.', ['%uri' => $this->getUriAsDisplayableString($uri)]));
   +      }
   

   I'm wondering whether we could / should provide a more specific message? What means valid here / should we care?

3. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -244,15 +244,6 @@ protected function actions(array $form, FormStateInterface $form_state) {
       */
   -  public function validate(array $form, FormStateInterface $form_state) {
   -    $this->doValidate($form, $form_state);
   -
   -    parent::validate($form, $form_state);
   -  }
   -
   -  /**
   -   * {@inheritdoc}
   -   */
   

   Afaik you have to update validateConfigurationForm to point to something existing ... doValidate() removing should break it.

Hopefully green.

reroll

Addresses remaining feedback from yched and dawehner and fixes the failures.

Unifying the violation message to match the one from menu UI, cause it's better.

Using user-path: in the UI explicitly is pointless, so removing that from the issue purpose. Added a test for entity:, along with the test-only version to demonstrate the failure in HEAD. Also updated the summary to include the only UI change that I think is in here.

I think this is ready now, unless there's further feedback.

Would be great for this to land before tomorrow morning's sprint, to unblock #2418017: Implement autocomplete UI for the link widget.

I tried it manually. allows saving with entity: and also allows user-path: (but user-path will never be entered by a user so it's kinda irrelevant. after save it displays user-path:one as one so it's ok either way)

but

two concerns yet to be addressed. discussed in person.

undelete doValidate()

move one of getUserEnteredStringAsUri() or getUriAsDisplayableString()

#2418031: Remove MenuLinkContentForm::doValidate() since the logic was moved to LinkWidget::validateUriElement() to handle the doValidate() concern.

Addresses #28. Adds a @todo linking to the issue in #29.

I'm convinced that this is RTBC at that given point in time.

Reviewing whilst we wait for testbot. If anyone's still awake, I'm in #drupal-nj.

I tried this out in the following way:

- Created a node/1 called "bananas" with a path of "bananas"
- Created two menu links: one to entity:node/1 and one to "bananas"
- Changed the path of node/1 to "bananaz"
- Expected result: first menu link continues to work, second one breaks.
- Actual result: Both menu links continue to work (?!?!)

Apparently our path system is doing something kinda magic here like remembering old paths, not sure?

At any rate, though, the patch is doing what it says, which is letting you enter entity:node/1 and it's linking to something. Also tried entity:user/1 for kicks and confirmed that there were no regressions in linking to e.g. <front>. It's a little funny that the patch doesn't also introduce an addition to the link field help text explaining you can now type entity:node/1 here, but I guess the thinking is that the autocomplete will be done soon enough we won't need to bother people with doing this from the UI (which obviously would be preferable).

Code-wise, only a couple of things.

1. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
   @@ -48,23 +48,34 @@ public function validatedBy() {
   -      $url_is_valid = FALSE;
   +      $uri_is_valid = TRUE;
   

   This was the only part that gave me pause, because we've moved from pessimistic validation to optimistic validation. I seem to remember this being raised in another issue as well... maybe by larowlan? Not going to block commit on it, but flagging it so once everyone has got a good night's rest, we can revisit this to make sure we're capturing all cases and didn't inadvertently introduce a sechole here.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/UserPathUri.php
   @@ -0,0 +1,35 @@
   + * Contains \Drupal\migrate_drupal\Plugin\migrate\process\d6\UserPatbUri.
   

   Patb? ;) Will fix on commit. OTOH, great to see that the migration path is already taken care of! That was one of the concerns I had about all of this URI transmogrification stuff.

Rock on, folks! Onward!

Committed and pushed to 8.0.x. Thanks!

Great work!

Issue ping pong :-)

#2417783: Remove widget specific logic in MenuLinkContentForm was closed as a dupe of #2417367: Use the new entity: URI scheme,
which in turn was closed as a dupe of this issue here,
which is now fixed,
but didn't address the issue described in #2417783: Remove widget specific logic in MenuLinkContentForm :-)

So, should we re-open #2417783: Remove widget specific logic in MenuLinkContentForm ?

What you described was removed in #2417793: Allow entity: URIs to be entered in link fields IMHO.

Trivial followup for #10, which was left unadressed :-)
#2418109: Misleading param name in LinkWidget::getUserEnteredStringAsUri()

@dawehner : no, the lines mentioned in #2417783: Remove widget specific logic in MenuLinkContentForm in doValidate() and extractFormValues(), hardcoding logic on the widget structure, are still there.

Also, wondering if the widget validation contains stuff that would rather belong to the field/TyedData constraint (LinkTypeConstraint) ?

It's always a tricky split, but roughly the rule of thumb is: if some check needs to run on non-UI saves (like REST operations), it belongs to field constraints. Widget FAPI validation is typically about stuff specific to the $form shape of that widget (and that would not necessarily make sense for a different widget shaped differently), or input errors that prevent forming a valid FieldItem to begin with.

The LinkWidget's #element_validate currently:

- forbids unrouted internal URLs (i.e. disallow 'base:' URIs)
is that form/UI specific ? Why don't we want to prevent that on REST operations too ?

- forbids linking to a URL the current user doesn't have access to
REST operations are behind user authentication too, right ? Am I allowed to POST a link value that I would be denied in the UI ?

- Disallow external URLs using untrusted protocols.
Likewise, untrusted protocols don't seem like something we'd want to allow on REST ?

Maybe this is valid, just wondering - so, not opening a followup for now :-)

Thanks for the explanations @Wim.

- disallowing base: URIs is merely an oversight/omission in HEAD, that this patch didn't want to change

Not sure I get this - does it mean this will be further adjusted in a separate issue ?

- disallowing inaccessible (from the current user's POV) is a relic from the D6/D7 past; and it prevents a use case that is desired by more than a few: the ability to create design the IA/URL design of a site by creating the desired menu tree structure before those URLs actually serve the desired content

OK - that desired use case seems more about non-yet-existing rather than non-accessible for a given user ? But right, if this keeps D7 behavior, changing that would be a separate feature request if anything I guess.

No, it's a 6+ year old regression: #308263: Allow privileged users to bypass the validation of menu items

The follow-up for #43: #2418181: Remove 404 validation from link creation

- Created a node/1 called "bananas" with a path of "bananas"
- Created two menu links: one to entity:node/1 and one to "bananas"
- Changed the path of node/1 to "bananaz"
- Expected result: first menu link continues to work, second one breaks.
- Actual result: Both menu links continue to work (?!?!)

Apparently our path system is doing something kinda magic here like remembering old paths, not sure?

The expected behavior happens correctly (well, after a a cache clear, so we're missing a cache tag for url aliases, which makes sense since those are neither content nor config entities) if you do the same scenario via a link field on a node (or any other entity) rather than a menu link. For a menu link, even a cache clear doesn't fix it, because of the materialized {menu_tree} table, but #2417837: Update menu link definitions when aliases change is for fixing that.

This was the only part that gave me pause, because we've moved from pessimistic validation to optimistic validation.

Not really, because highlighting a few more lines from the committed patch:

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
@@ -48,23 +48,34 @@ public function validatedBy() {
-      $url_is_valid = FALSE;
+      $uri_is_valid = TRUE;
-      $url = $link_item->getUrl(TRUE);
-      if ($url) {
-        $url_is_valid = TRUE;

The old code was also becoming optimistic so long as getUrl() returned some URL, and then proceeded to make it false later based on specific checks about that URL. So both prior HEAD and current HEAD follow similar "assume valid until you find a problem" logic. But that's the same with a lot of our validators. For example, in HEAD, UserNameConstraintValidator contains a bunch of if statements that add violations. If none of those statements evaluate to true, then no violation is added, and therefore, the constraint is satisfied. The difference with LinkTypeConstraint is just that the violation message is the same in all cases, so we have a local variable to let us keep it in one place.

I seem to remember this being raised in another issue as well... maybe by larowlan? Not going to block commit on it, but flagging it so once everyone has got a good night's rest, we can revisit this to make sure we're capturing all cases and didn't inadvertently introduce a sechole here.

If someone spots a flaw in the above reasoning, please open an issue for it.

Fair enough! Thanks for checking into it.

so we're missing a cache tag for url aliases

I thought about adding a new issue for that, but for now, just noted it in #2335661-23: Outbound path & route processors must specify cacheability metadata, and would like to leave it to people working on that issue to decide whether to spin off a specific sub-issue for that.

[Wim #42] Disallowing base: URIs is merely an oversight/omission in HEAD, that this patch didn't want to change
[yched #43] Not sure I get this - does it mean this will be further adjusted in a separate issue ?

So, not sure if we still need a followup for this ?

[edit: never mind, #2418181: Remove 404 validation from link creation does take care of this. @effulgentsia++]

FYI : #2076321-9: Link "title" validation should use a static method is about making the pre-existing LinkWidget::validateTitle() consistent with the validateUriElement() that was added here.