EditorLinkDialog should validate URLs, and autocomplete like the Link widget

Even stranger, when I hover over the saved link in the body field while in Edit, the URL it sees is [protocol]://[domain]/node/2/node/1 . Does that offer any clues?

So the rendered link in the body is being written without the leading slash, node/1 which the browser interprets as relative to the current page url.

By contrast the link field is adding a leading slash, /node/1 which the browser sees as relative to the domain's root.

This is the way Drupal has worked for years, so I am wont to say "It works as designed." But then you have to admit that there is a blatant inconsistency with the way the link field works.

So now that we know what the difference is, what is the prevailing community guideline or opinion on what behavior we should preserve and which to deprecate?

Issue exists in the latest dev release also.

Adding a slash at the beginning of node/1 will not work if your site is located in a subdirectory like: example.com/drupal. In that case the field the URL field adds the subdirectory path also which is not done in case of CKEditor.

Changing the component to CKEditor since the anchor element is created by CKEditor plugin and not by EditorLinkDialog (correct me if I'm wrong). Relevant code is in core/modules/ckeditor/js/plugins/drupallink/plugin.js:

 66               // Create the new link by applying a style to the new text.                                                                         
 67               var style = new CKEDITOR.style({ element: 'a', attributes: returnValues.attributes });                                              

Everything in the issue summary can be summarized as follows: EditorLinkDialog simply accepts whatever string it is given; it doesn't perform any validation or transformation. Hence it also does not transform "Drupal paths" (which aren't actual URLs) to URLs, but e.g. the Link field does this, so EditorLinkDialog probably should do the same.

It's not CKEditor's responsibility to transform these, so this definitely belongs in the editor.module component.

See LinkWidget::massageFormValues(); that's how the Link field does it, and how we should do it.

Oh, and as cs_shadow says in #8: merely prepending a slash will not work when Drupal is installed in a subdirectory.

I tried to take a look at this but I'm having a problem as I'm not quite sure where to put any of the code. There's LinkWidget::massageFormValues(), yes, but LinkWidget and the other classes with this function extend WidgetBase whereas EditorLinkDialog extends FormBase. Would the validation and the transformation both occur in a validateForm function? That feels weird to me, to be editing a value while in a validate function, but I may be wrong about that.

Any insights, anyone?

#12: Thanks for jumping in and giving it a shot! :)

I can understand your confusion. LinkWidget is a Field API widget. Therefore it is more abstract, and it indeed doesn't translate 1:1. A Field API widget is sufficiently abstract so that Drupal can automatically build forms that include one or more link widgets.
Compare that to EditorLinkDialog, which indeed just extends FormBase, which indicates what it is: it is nothing more than a simple form.
So rather than being an abstract building block of a form (LinkWidget), we're dealing with a simple form (EditorLinkDialog).

Basically, LinkWidget::massageFormValues() is more complex than what we need: it needs to handle multiple values (because a link field may be configured to accept multiple values, hence multiple URLs), but we only need to handle *one*!
So you can take the key parts of the logic:

$parsed_url = UrlHelper::parse($value['url']);

          // 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']);
          }

          // et cetera

and indeed put it in a new EditorLinkDialog::validateForm() method. I agree that it can feel bizarre to be altering/"cleaning up"/"massaging" a form value during the "validate" phase, but since Drupal's Form API doesn't have a separate "massage" stage, that's how it's done!

So: you were very close, your hunches were right, your sense of code smell was right, but… it's in fact how it needs to be done in today's Drupal — hopefully we can remove that code smell in the future — perhaps with your help! :)

Thanks @wim-leers!

I think I managed to take the key parts of the logic and apply them to validateForm(). I've attached a patch for review.

Okay, let's try this again with a patch that isn't corrupted.

Wonderful, this is looking great already! :)

There's one problem though: if I enter node/1, that gets converted correctly to /node/1, but if I then try to edit that link again, save immediately without changing it (so saving /node/1 instead of node/1), it tells me it's invalid. We'll need to fix that. I think the easiest solution would be to tell Url::createFromPath() to generate an absolute URL (http://example.com/node/1)for us instead of a relative one (/node/1). You can do that by calling ->setAbsolute(TRUE).

Code review:

1. +++ b/core/modules/editor/src/Form/EditorLinkDialog.php
   @@ -79,6 +84,46 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor
   +      $url->setOption('attributes', array('target' => $attr['target']));
   ...
   +      $form_state->setValue('attributes', array('href' => $path, 'target' => $attr['target']));
   

   Setting the target attribute is not necessary here, because Url will ignore it. After all the <a target="something"> attribute is something that lives at the link level, not the URL level.

2. +++ b/core/modules/editor/src/Form/EditorLinkDialog.php
   @@ -79,6 +84,46 @@ public function buildForm(array $form, FormStateInterface $form_state, FilterFor
   +    catch (NotFoundHttpException $e) {
   +      $form_state->setErrorByName('attributes', $this->t('Failed to save URL. The server reports the following message: !message', array(
   +        '!message' => '<p class="error">' . $e->getMessage()  . '</p>',
   +      )));
   +    }
   +    catch (MatchingRouteNotFoundException $e) {
   +      $form_state->setErrorByName('attributes', $this->t('Failed to save URL. The server reports the following message: !message', array(
   +        '!message' => '<p class="error">' . $e->getMessage()  . '</p>',
   +      )));
   +    }
   +    catch (ParamNotConvertedException $e) {
   +      $form_state->setErrorByName('attributes', $this->t('Failed to save URL. The server reports the following message: !message', array(
   +        '!message' => '<p class="error">' . $e->getMessage()  . '</p>',
   +      )));
   +    }
   

   First: we don't want to expose the exception's error message to the end user — this is information for developers, not for end users. It would only be very confusing. :)
   I think think the message could just say something like The entered path does not exist. in all cases, without any further information.

   Second: there's a lot of repetition here! But we cannot combine the catching of all these exceptions in a single "catch" block. However… because Drupal 8 requires a fairly recent version of PHP, we can just create an anonymous function inside of this function to always do the same thing.

   So at the top of validateForm() you could do something like:

   $show_error = function(\Exception $e) {
     $form_state->setErrorByName(…);
   };
   

   And then call $show_error($e) in all three "catch" blocks!

Using ->setAbsolute(TRUE) creates a problem, though, in cases where you don't want WYSIWYG links to be absolute URLs. Say you were having a user create content on a dev site and then merging the database up to the prod site pre-launch. The links would all still be pointing to the dev site, right?

Using ->setAbsolute(TRUE) creates a problem, though, in cases where you don't want WYSIWYG links to be absolute URLs. Say you were having a user create content on a dev site and then merging the database up to the prod site pre-launch. The links would all still be pointing to the dev site, right?

This is true, but:

1. It's also the case for the relative URLs that the current patch creates! If your dev site is installed in a subdirectory and your production one isn't, then your links will look like /mysubdirectory/node/1 instead of /node/1.
2. This is a general problem, the only way to solve this is by not actually generating a link, but storing data- attributes that a filter transforms to the correct links for the current site.

This issue is the first step: make EditorLinkDialog generate URLs at all from "Drupal paths" (like node/1 and user/5). The next step would be to create links in such a way that they're automatically adapted to the current site.

That being said, I'm fine with root-relative URLs, but then you will have to adjust the validation :) That's why I suggested absolute URLs as the solution: it's the easiest solution.

As you can tell, there is no solution that is both simple and solves everything!

Good points regarding usability in #21. But that does mean this patch becomes more complex, because it will be necessary to store the path corresponding on the <a> element then, e.g.: <a data-drupal-internal-link="node/1">.

Are you still up for that, emclaughlin? :)

Wouldn't the link needing to be whatever the user put in in the WYSIWYG and needing to be an absolute path when outputted suggest that the validation function should be used purely to validate the link, not change it, and then the link be updated to the absolute URL of the site it's on during rendering?

#23: that's absolutely correct!

It could either be absolute (as in your example) or root-relative (/node/1). It could be configurable on the filter.

In testing #16, the patch applied, with the following message: "Hunk #2 succeeded at 85 (offset 1 line)."

I tried testing the current functionality of patch #16 before working on addressing some of the points in #17-27, but I am running into an ajax error and the dialog will not close or present the error messages:

I'd be interested in working through addressing the points in the above comments if someone would be willing to help me figure out the dialog issues.

Tested with Drupal 8.0.x with the Standard profile.

So, I'm having some trouble figuring out where the links are rendered so that I can make sure that links are being rendered as /node/1 and not node/1. Does anyone have any guidance on this?

@bneil The patch no longer applies correctly because code has been changed since it was created. I've got an updated patch for validation but was waiting to attach it to the post until I could figure out the rendering issue.

Okay, nope, new question, figured out the last one by rereading the thread and realizing I was doing everything wrong.

I've attached the patch I've created so far here. The problem is, when you input an invalid URL, and then input a valid one, the form redirects off to /editor/dialog/link/basic_html. I'm not sure how to track down where/why this is happening. Can anyone provide any guidance?

@mclaughlin -
I believe the issue you're running into is #2347469: Rendering forms in AjaxResponses does not attach assets automatically. Note the "referenced by" issues all relating to that problem.

Actually, the lack of clarity of how this should work points to a wider problem. And that wider problem is captured in #2407505: [meta] Finalize the menu links (and other user-entered paths) system, which is going to be the focus of the D8 criticals in New Jersey next week. Hopefully we'll come out of that week with all of those things solved, which should make this issue significantly simpler!

I think this should be unpostponed now? The majority of the critical work for #2407505: [meta] Finalize the menu links (and other user-entered paths) system is done. If there's a specific issue that's still blocking this, let's postpone it on that rather than the meta. Thanks!

This could use an issue summary update with clear next tasks to be done in the issue. Also need tests written for this.

So there is no need for me to port the Linkit module to D8 now?

It seems Novice tag was added 2 years back. As per #42 it required more discussion on it. It is no longer to be a Novice issue. I am removing the Novice tag.

A similar feature request was posted at #2689661: Add CKEditor Link to core.

Indeed I suggested it for several reasons but the most important I think, is when a domain and or a TLD is changed, and there is no suitable page2page redirecting mechanism. Moreover, when someone moves from Drupal 7 to Drupal 8 and needs to work with another similar module and copy all links manually in each page (No one can promise, I guess, that a similar situation won't happen in Drupal 9).

From an SEO viewpoint, Non-boilerplate internal-links ("content region" internal-links in Drupalese), are important and can give better information-navigation and lower a chance for what can be defined as "fast remission", that is, when the client goes back to fast to Google and checking other results.

Making this tool most accessible and intuitive in Drupal could benefit all users in these contexts.

This sounds like adding link-it to core, which feels like a feature request rather than a bug

Obsolete since #3306584: [11.x] Remove EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 11. Spiritual successor: #3317769: Add support for linking to entities in CKEditor 5.