Linkit 8.x-4.x flaws from the POV of the Text Editor/CKEditor APIs in Drupal 8 core

Note this will be some work now, but it will mean far less work in the long run (because less to maintain), and it will allow you to easily fix:

• #2688353: Problem with language prefix in url, uploading a file
• #2652710: Use the translated entity when available/#2280441: Entity Translation support
• #2687901: Wrong links on multi language page with configured prefix
• #2692619: Use internal path instead of alias
• … and more

Finally, please know the reason I took the time to write all of this is simply because I want to see the ecosystem for the Drupal authoring experience be as strong as possible — this also means ensuring that the various important modules (of which this is definitely one!) integrate nicely with each other, and support all use cases, including more complex ones with changing URLs, multilingual content, and so on.

I firmly believe the above is how you can make Linkit be a great module in the authoring experience ecosystem.

I'm happy to help you with reviews and help unblock you when you get stuck.

Based on #6, I think you would appreciate a rough patch that shows how to do all these things. Let me know if that's the case, then I'd be happy to write that rough patch. You can then use that rough patch as a starting point, as an outline.

+1 for #9.

I don't think I'll get this done today, but I should be able to do it somewhere in the next few days.

Here's a first round of work on the filter, continuing on top of what you did in #11 and #12. This should be fairly solid.

Sadly, this made me run into a core bug. Upon presenting text to be edited to a content author, \Drupal\editor\EditorXssFilter\Standard::filterXss() is called, which relies on \Drupal\Component\Utility\Xss::filter(), which in turn uses \Drupal\Component\Utility\UrlHelper::filterBadProtocol()… which considers entity: as a bad protocol, and thus strips it. The database contains the URL just fine, and the filtering works just fine, but upon starting to edit text, it undergoes this filtering process and href="entity:FOO" ends up being transformed to href="FOO". That sucks.
Related issues: #2105841: Xss filter() mangles image captions and title/alt/data attributes, #2609928: Xss::attributes() mangles valid attribute names containing numbers. Sadly, there are only two possible solutions:

1. Add 'entity' to \Drupal\Component\Utility\UrlHelper::$allowedProtocols
2. Don't use href, but use data-href. (data- attributes don't undergo the same scrutiny.)

The latter is probably the easier solution, and we can update EditorLinkDialog to give preference to that if it exists.

… and this got me thinking…

It reminded me of #2666382: EditorFileReference filter: generate URLs for inline images, which is highly similar, but for file uploads.

What if we take the same approach? What if we use data-entity-type and data-entity-uuid? Then we also benefit from \Drupal\editor\Plugin\Filter\EditorFileReference automatically recording any links to files as usages, preventing premature deletion (i.e. it will be listed in the "used in" column at /admin/content/files). Two birds, one stone!

So, let me rework this to do that.

Forgot the interdiff.

Yay, did what I described at the bottom of #17. Now much simpler, much more robust, much more integrated. In summary: much better!

This also fixes the changes to EntityMatcher introduced in #11, which only happened to work by accident.

On to the next step.

I just noticed I apparently changed core's filter schema rather than Linkit's. Oops.

You alter EditorLinkDialog, which means you can delete \Drupal\linkit\Form\LinkitEditorDialog, \Drupal\linkit\Plugin\CKEditorPlugin\Linkit, js/plugins/linkit/plugin.js (which is 99% copy/paste from core/modules/ckeditor/js/plugins/drupallink/plugin.js anyway, so it's literally duplicating it without keeping up with recent changes to it) and don't need to maintain any of that. You leave those foundations to Drupal core.

This is now a reality, so we can delete the custom CKEditor plugin and the custom dialog.

I have to run now. Will continue tomorrow.

Next steps:

• remove attributes from the matchers, because A) title is now automatically set based on text format configuration, B) all other attributes are manual input anyway, and should be set manually, we have https://www.drupal.org/project/editor_advanced_link for that
• …

Looking forward to feedback on what I posted so far!

#30 does not apply at all:

error: cannot apply binary patch to 'js/plugins/linkit/linkit.png' without full index line
error: js/plugins/linkit/linkit.png: patch does not apply

I never have problems with this sort of thing usually, despite modifying images relatively frequently. Weird.

#23:

Ah, now I got what you ment by removing the attributes from the Linkit profile. Seems ok to let editor_advanced_link take care of that instead.

:) Sorry for not being more clear initially!

Note to myself: Add a link to https://www.drupal.org/project/editor_advanced_link on the project page.

That would be great!

and if that solution will be used by other modules as well

It already is! We established together with the https://www.drupal.org/project/entity_embed people, and made sure the same solution was used in core. Media in D8 relies on Entity Embed, and Entity Embed uses this/relies on this, and core supports it. With this module also supporting that, the ecosystem is strengthened!

One thing that come across my mind is how do we handle the update for users that already installed Linkit in the way it works now?

Their old links will continue to work in the same way they do today. Any new links they create with this functionality/logic will be better. Their old links aren't harmed in any way. I don't see how you could ever automatically update all those existing links. There's no way to detect them.

The old URLs will continue to work, but how do we handle the fact that we now alters the core link plugin and also lean on a new filter plugin.

Oops, you already said that. Well, we agree then :)

The only way to help them upgrade is IMO by making it very clear in the release notes. You can automate part of the upgrade process though:

1. You can use hook_requirements() to warn users.
2. You can use hook_update_N() to automate the process: to remove the LinkIt button etc.

But since this is quite a big change, I think this warrants a new major version.

#26: Woot! That's a LOT less code! Must make you excited too, because it means far, far less work for you as a maintainer :)

#27 + #28: I did not quite like your approach at first, but then I realized it is the only way to be able to add configuration to DrupalLink plugin, because it does not implement CKEditorPluginConfigurableInterface by default. However, this means that if any other module wants to do something like this module (the chances of that seem very, very slim), that it will run into the same problem. I think that for now this is the best approach.

Perhaps Drupal core 8.2 could change DrupalLink to already implement this interface. But that doesn't really make sense, because there won't be any configuration … unless #2292159: EditorLinkDialog should validate URLs, and autocomplete like the Link widget were to happen.

That is actually a very important issue I wanted to bring to your attention. It's basically what LinkIt provides. I think it would be incredibly valuable to have that in Drupal 8.2 core. I think everything in LinkIt (with this patch applied) makes sense to move into core, with one big exception: the concept of LinkIt profiles.

I think that there can be a sensible default: perform linking/autocompletion to all entity types. Only entities that are not accessible for the current user are omitted ($entity->access('view')). And the exact pattern to use would be stored in configuration that is either hardcoded or just stored in configuration without being exposed. Then the LinkIt contributed module would just provide the ability to do the really advanced things, but the fundamental functionality would be in core: making it a wonderful experience to link to existing content.

What do you think? :)

If you could live with that assessment/approach, then the problem of needing to be able to configure "LinkIt profiles" and being able to select one would also go away.

#30:

The data attributes is set at first, but they disappear when toggling "source" on and off. I think this is because we are not allowing them in the HTML limit filters settings.

Indeed, you currently need to manually whitelist them. Whitelisting based on filters being enabled. This is a known limitation, we have #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11 to fix that.

The title doesn't seems to be added when I try this.

You also need to whitelist the title attribute, because the title attribute is added by the LinkIt filter, which runs before the HTML filter.

#31: Ugh, I forgot to read ahead again. Same conclusion again. For now, that will have to remain a manual step in the installation process.

The only remaining missing there here IMO is providing a default LinkIt profile. And, ideally, we would also remove the concept of LinkIt profiles, but that seems quite difficult to achieve, so I think it would probably be better to have a separate issue for that, if you're willing to have that conversation.

So, what do you think about my proposal? Basically: first clean up LinkIt like this patch does, then work on getting it into core. I'd love to welcome you as a co-maintainer of the editor module, either for just this part, or for everything, because you clearly know your stuff :) Then all Drupal 8 users would get to benefit from your years of experience building LinkIt! :) :)

#35:

If we do that, do you think we can deprecate the 4.x version in favor for the new one right away or is that bad practice?

Yes, that's totally fine! :)

Oh, haha! #2292159-39: EditorLinkDialog should validate URLs, and autocomplete like the Link widget was visionary, but the reason LinkIt wouldn't be necessary anymore is your very work being moved into core :P Not because core already did it.

You mean that the Linkit module would provide a way for users to change the default provided (by core) settings for the link field?

I mean that the LinkIt module would provide a UI to change the default matchers: which matchers, and for each matcher, the tokens for their results.

Even if it hurts a little (as this is my cute little Drupal baby module leaving home)

I totally understand that! :)

I agree with you

Great, I'm glad! It would be awesome to finally have an awesome linking experience built in right to Drupal core out of the box.

there is no longer a need for Linkit =(.

Well, no, there would still be that place for it where the contrib module (this module) can provide the super advanced things, like profiles, search results tokens that can be modified in the UI, etc. Or perhaps even allowing the user to select a particular translation to link to (for which we could use the hreflang attribute).

So with that in mind, I would love to become a co-maintainer for the editor module and help evolving core, share the knowledge and experience I have from building and maintaining the Linkit module.

That would be amazing! :)

The branch looks fine, but I'm actually able to apply the patch in #36 without any problems. So, no need even!

#36: that test looks good! It will eventually need to be expanded further though, to deal with entity translations for example. But it's good enough for now (LinkIt currently doesn't support translations either). Would be good to add a @todo for that.

To me, the only thing left to be done here is:

3. You provide a default Linkit profile.

My ideal step-by-step plan to get LinkIt's awesomeness in the hands of as many people as possible:

1. commit this as version 5 of the LinkIt module (because this is such a big change: the filter is now required etc), and release it as an alpha version
2. work to improve the quality to bring it to core-level quality: coding standards (great already, but not perfect), dependency injection (great already, but not perfect), test coverage (great already, but not perfect) — particularly integration tests for the auto-complete, and more situations and edge cases tested for the filter
3. turn the version 5 branch into a core patch, post at #2292159: EditorLinkDialog should validate URLs, and autocomplete like the Link widget
4. remove all the LinkIt Profile stuff from the core patch (this will enormously reduce the size of Linkit), figure out a sane default that we can apply to all entity types
5. get that committed to the 8.2.x branch, so it ships with Drupal 8.2
6. when that gets committed, if you want to, work on a version 6 of the module which is basically just the LinkIt Profile stuff plus any other advanced features that I currently probably don't know of that they exist :)

Then starting with Drupal 8.2, the authoring experience will have gotten significantly better, and tens of thousands of people have a less frustrating day, every day, thanks to you :)

Wow, awesome work, @anon! I can't believe this is actually happening, this is what I hoped for/dreamed of when I was working on CKEditor integration into Drupal 8 core :) So glad you're on board with this!

Alpha, beta, RC are all unstable. Everything else is stable.

A "dev" release is just following the HEAD of a branch. So, whenever you push code to 8.x-5.x, the "dev" tarball is going to be updated with those new commits.

Therefore alpha/beta/RC releases are to encourage testing. Alpha is when APi is not yet frozen. Beta is when API is frozen. Any RC is a candidate to be a release, so only bugfixes, and should be quite stable already.

I would love some feedback/examples on what's missing and what we need to improve here. As I think you already know, you easily get blink on your own code. =)

Unit tests are missing. And particularly tests (unit or integration) that verify all the edge cases:

• translated entities (both in filter and in autocomplete)
• inaccessible entities (both in filter and in autocomplete)
• for all of the above, test as different users (with different permissions/node grants)
• for all of the above, if content or config are modified, then no stale results should ever be shown
• et cetera

That should be the goal. Though I think I would need some help here too to learn the process of this.

Yay, glad to hear that! :D And of course, I (and others) will help you!

A URL can be sensitive: if it has a path alias that reveals information. Otherwise, something like example.com/node/4523453 does not exactly leak any information, so should be fine.

Overall, that's a great question: what should happen if the link is actually inaccessible for current users? I'd say: let's look at Drupal core: how does the link field handle this? I'm surprised to see nothing access-related in \Drupal\link\Plugin\Field\FieldFormatter\LinkFormatter.

So I'd say: let's not deal with inaccessible entities in the filter right now, only in the autocomplete. But let's add a TODO to not forget. If that's the only remaining concern, we'll be in a very good place :)

The autocomplete should already take care of inaccessible entities, i.e. not showing them.

Sure, but do we have test coverage for it?

How do we continue with this issue? I mean we have done pretty much everything now that relates to this module, though we still need some more tests for stuff, but I think they can be separate issues.

+1, I think this issue can be marked as fixed :)

Thank *you*!

@Cottser: happy to hear you're on board! :)