Drastically improve Drupal's default linking experience in text fields

Time to get this going again! I'm working on addressing @Berdir's superb feedback — stay tuned 😊

Very small user-experience suggestion for the naming of the text format filter. Enabling this text format filter will be the trigger that enables the autocomplete within the Drupal core link plugin. The title "Entity Links" may not be intuitive to users here, and the current description text only explains the text filter part of the behavior (""Updates entity links with data-entity-type and data-entity-type-uuid attributes to point to the latest entity URL aliases.").

What about something like "Drupal Linking" for the title, and include "Adds autocomplete suggestions for Drupal entities into the link plugin" in the description?

Am I right in thinking this means sites will need two editor toolbar buttons for what practically all users consider a single purpose?

• One for linking to internal entities, and
• Another for linking to external urls and internal paths like views pages?

Can I just check that you're considering one absolutely essential use case, please?

The media library contains documents as well as images. When we link to a media document container (whether in a field reference or in CKEditor), we need to have the path resolve to the file itself rather than the media entity. So that when we link to a PDF that's in the media library, for instance, the browser displays (or optionally downloads) that PDF.

If made from CKEditor, that link must also be a normal inline element.

The second requirement, which follows naturally from the first, is that if we replace the file in the media container (with an updated version of the PDF, for instance), any existing links then resolve to that updated version.

I hope I explained that well enough because it really is an absolutely critical need!

Thanks, Andrew

To expand on #27, the way we want our links to behave by default in the editor depends quite a bit on the media type we're linking to. For the most part, we link to the entity with PDFs, and display them in an embedded viewer using the PDF module and PDF.js library. For other document types (for which we use a different media type), we want the link to download the file. I imagine we'd want to be able to configure the default behavior for a link based on entity type and bundle of the target.

I would echo what the two above have said. If we can replicate what CKEditor Link → and CKEditor Link File → did in Drupal 7 in core, it would be freakin' amazing. Right now the CKEditor 5 dialog just presents a popup for the editor to enter a URL, but expecting them to go and hunt down a URL and paste it in there is expecting way too much. Additionally, if the title is updated or some other change is made to the alias, they'd have to go in and update it manually everywhere! A nightmare for sure. This is a great initiative to get Linkit functionality in core!

I said in #24 that I'd start working on this again. And I meant it. But then https://www.drupal.org/project/automatic_updates → took my full attention 😅 — see https://wimleers.com/blog/high-concurrency-composer for a fun anecdote 🤓.

Tomorrow is entirely dedicated to this. If not tomorrow, expect progress early next week. Both test coverage and addressing the feedback posted so far.

#29: my neck hurts from nodding along so strongly 🤣

@mark_fullmer has stepped up to maintain LinkIt in contrib until this issue lands. That's wonderful news! That's why 📌 CKEditor 5 readiness Fixed actually shipped in https://www.drupal.org/project/linkit/releases/6.0.0-beta4 → , meaning sites using CKEditor 5 can finally use LinkIt since March 5! 🥳🚀

In his closing comment on that issue, he mentioned 2 major issues, which we should take into consideration here too:

1. multilingual autocompletion support, and I've responded to both: #2886455-62: Add support for multilanguage → + #3041045-25: Allow "Linkit URL converter" filter to generate links based on the language of the current page, rather than the language of the referenced entity →
2. LinkIt support for the link field type has landed since then: on March 10, that landed: #2712951-334: Linkit for Link field → . With over 200 followers for that issue, we'll need to carefully consider how to bring that into core as well, which @Berdir has so articulately argued for :)

Today, I got back into this MR and added test coverage for the filter → removing tag. I hardened & cleaned up LinkIt's filter test and in the process I discovered that:

1. there is a major bug in the test for translations' path aliases: 🐛 LinkitFilterEntityTest::testFilterEntityTranslations() does not test what it appears to test Fixed
2. there was no test coverage yet asserting that no exceptions were logged
3. there was no test coverage yet asserting the cacheability metadata is correct
4. this core port of the contrib module did not yet support linking to File entities — because it did not yet support substitution plugins. I would still like to try to avoid adding new API surface, and instead rely on the existing general plugin API capabilities for overriding/extending this. So instead of adding substitution plugins, for now, I added protected static function getUrl(EntityInterface $entity): GeneratedUrl { … } which means customizing this is as easy as: subclass, override ::getUrl(), then:
   

   function MY_MODULE_filter_info_alter(&$info) {
     $info['entity_links']['class'] = 'Drupal\my_module\CustomizedEntityLinks';
   }
   

   I'm working on an analysis of the entire contrib ecosystem to check if this is viable or not! Before responding to this, please wait for that 🙏

5. as written in #31.1, there are several bug reports related to multilingual handling, but the root cause seems to lie elsewhere — although we definitely should have test coverage for the autocompletion too → restoring tag 😮‍💨

The filter test coverage is now stronger for this core MR than it is in the contrib module! 😊

Next up:

1. make the functional JS test pass again (I suspect the update to CKEditor 5 36.0.1 earlier today caused this, because locally the test still passes: 📌 Update CKEditor 5 to 36.0.1 Fixed )
2. process feedback in #25–#29
3. contrib ecosystem analysis of @Substitution plugins, to articulate either an alternative or indeed add @Substitution plugins here too
4. entity reference selection plugins as suggested by @Berdir in #18.
5. expand functional JS coverage to test suggestions in various multilingual situations

https://github.com/ckeditor/ckeditor5/commit/c276c45a934e4ad7c2a8ccd0bd9... caused this JS error, and it's actually a performance improvement in CKEditor 5. After a long search, I found a minimal solution 👍

→ finished #32.1, onto the next item!

Thanks for the compatibility fix for the JS error, Wim! I'll follow suit in Linkit's implementation for the time being.

@mark_fullmer in #25: You're totally right that "Entity Links" is perhaps even more meaningless than "Linkit" — worse, it probably sounds scarier! 😳🙈 I don't quite like "Drupal linking" either, because we should not be using the word "Drupal" in any UI text except the most high-level ones. A Drupal site built for an end user will not (or rather should not 😅) mention "Drupal" at all! What about "Local content links" as the filter title and "Suggests local content when creating links" as the description?

@John Pitcairn in #26: fortunately you're wrong 😄 Single Link button, single UI, but whatever you type in the <input type=text> will try to find autocompletion matches. For external URLs it will simply not find matches.

@Andrew.Macpherson in #27, @wrd-oaitsd in #28 and @dalemoore in #29: Thanks for stating that explicitly! I did notice that the Linkit contrib module has an explicit @Substitution=media plugin which does what you describe (its description: Direct URL to media file entity. I'm not sure you're all saying the same thing though 😅

This does tie in closely to what @Berdir has raised in #18 about Linkit's substitution plugins and @acbramley confirmed in #19, even though none of you use that term.

I've been doing a comprehensive analysis as promised in #32.

I've worked my through all that, and am still working on a comment that summarizes my analysis, but I do have a working solution that I believe to be better than Linkit's Substitution plugins. Looking forward to y'all reading my next comment, and especially @Berdir 😁🤞

@Wim Leers in #35: I'm not sure "Local content links" makes sense for non-technical end users. To them "local" could just as well mean geographically. I do agree that it makes more sense than the previous suggestions, and that "Drupal" should be avoided in the UI. Maybe "Site content links"? Just spitballing.

I'm wondering if just "Content links" would be clear enough? I don't know if site adds anything to the name. We will have to complement the name with a description either way.

How about "internal links"?

🕵️ Analysis of Linkit's substitution plugins in contrib

1. Y'all are asking pretty much for behavior identical for Media entities as what Linkit in contrib does today. That's obviously reasonable 👍 Pushed equivalent test coverage to ensure we match the behavior.. The subsequent commit added the missing logic.
   But that does not yet address @Berdir's and @acbramley's concerns!
2. I checked and found 10 substitution plugin implementations in total:
   1. dam_asset: a href=" https://www.drupal.org/project/usage/media_acquiadam → ">526 sites using it
   2. linkit_custom_link: a href=" https://www.drupal.org/project/usage/linkit_custom_link → ">8 sites using it
   3. media_download_inline + media_download: 6480 sites using it →
   4. static_asset_cache_buster: 0 sites using it →
   5. media_file_redirect: a href=" https://www.drupal.org/project/usage/media_entity_file_redirect → ">470 sites using it
   6. canonical + file + media: built-in to LinkIt since version 5, so 88195 sites using it →

   That's only 7484 out of 88195 sites using Linkit, or less than 10%.

   But it's obviously not a few dozen or hundred, and I do see the value:

   • dam_asset uses a custom media source plugin (which is exactly why the Media system is designed this way), and the Media module cannot possibly know or anticipate every possible scenario ⇒ REQUIREMENT: we need to somehow enable each media source plugin to be able to generate these URLs
   • linkit_custom_link is similar to ckeditor_entity_link's support for MenuLinkContent & Shortcut entities (see #35), and is simply an addition to LinkIt's canonical (fallback for all entity types) + file (File entities) + media (Media entities) ⇒ REQUIREMENT: we need to somehow enable each entity type to be able to generate these URLs
   • media_download + media_download_inline + media_entity_file_redirect override Linkit's default media linking behavior to provide stable URLs that themselves whose controller redirects to the current actual file ⇒ REQUIREMENT: we need to somehow enable contrib modules to override the URLs that are generated by default for a particular entity
   • static_asset_cache_buster: decorates every existing substitution plugin and rewrites all local file URLs to contain a cache-busting querystring ⇒ same thing: contrib overridability (although in this case one could argue it should be implemented as a filter plugin that runs late)

   So the requirements are that we need optional logic to generate link targets for a linked entity of A) any entity type, B) for the media entity type: any media source plugin, C) it must be overridable.

🕵️ Analysis of CKEditor Linking modules in contrib

• #29 surfaced Drupal 7 modules (which supports linking to Node, Term and MenuLinkContent — it has 37,000 sites using it despite being D7-only!), https://www.drupal.org/project/ckeditor_link_file → (File entities) and https://www.drupal.org/project/ckeditor_link_user → (User entities, which was also mentioned by @Berdir!).
• I mentioned in #35 that I also discovered ckeditor_entity_link for Drupal 8 & 9 (with 5219 sites using it), which allows linking to MenuLinkContent, Shortcut, File and Media (supporting BOTH file and oembed:video media sources!) using custom logic and to everything else falls back to the sane default of using the canonical URL.

  In essence, this module does more than most substitution plugins combined with just two dozen lines of code 👏

📝 Proposal

I wrote this in the original issue summary:

I was wrong. There is a proven, justified need for something like this, otherwise too many entities are excluded from being linkable.

I just do not believe that Linkit's substitution plugin architecture is acceptable in Drupal core: a new plugin type that is maintained entirely separately from the owner of the domain knowledge: the entity type itself.

That's why I propose instead:

1. a new optional link_target handler that can be specified for any EntityType plugin → this automatically means that it's overridable!
2. these link_target handlers would have to implement \Drupal\Core\Entity\EntityLinkTargetInterface which has a single method: getLinkTarget(EntityInterface $entity): GeneratedUrl.
3. prove that this works: convert existing hardcoded logic I had in the merge request until now: add FileLinkTarget + MenuLinkTarget
4. prove that this works for additional entity types: implement it for MenuLinkContent + Shortcut (both of which are essentially metadata containers for other (accessible) targets (\Drupal\menu_link_content\MenuLinkContentInterface::getUrlObject() + \Drupal\shortcut\Entity\Shortcut::getUrl()) → also ensures that this supports just as much as contrib modules do today
5. prove that some entity types can choose to delegate this to the relevant piece in their architecture: the Media entity type should let @MediaSource=file and @MediaSource=oembed implement the actual logic, with a fallback in case standalone media URLs are enabled

🚀 Implementation of proposal

• 1+2+3
• 4: → failing test + implementation
• 5: → failing test + implementation

Initial change record created: https://www.drupal.org/node/3350853 →

😅 TBD: <a download>

I think that means the only thing I have not yet addressed is http://drupal.org/project/media_entity_download's support for triggering a download. That could be addressed in one of three ways:

1. allow specifying the download attribute (which was not sufficiently supported a few years ago but now enjoys >96% browser support) on <a> using CKEditor 5 itself — see https://ckeditor.com/docs/ckeditor5/latest/features/link.html#demo-2 for a nice demo of that. The problem: that'd expose a "Download" toggle for all links, not just Media links. But that is something we could definitely work on.
2. create an additional filter that runs later, which looks for <a href data-entity-type="media" data-entity-uuid> elements, loads the entity, checks its bundle and/or source and based on filter configuration adds the download attribute to these links at render time — that'd definitely guarantee consistency, but perhaps that's not desired … in which case option 1 is better
3. override MediaLinkTarget and override the generated links, with even the freedom to choose to reuse or not reuse the logic in the media source plugins

→ finished #32.2 + #32.3 — next up: adopt entity reference selection plugins (#32.4)

@Berdir: Really curious about your thoughts! 😊 Marking for the link_target handler stuff.

#36 + #37 + #38: Thanks for the input!

Good point regarding the ambiguous meaning of "local"! 😬 I have a similar concern about the "internal" in "Internal links": it could too easily be interpreted as links to internal paths, not to entities.

I like as the human-readable label (even though that appears to imply it works only for Node entities since "Content" is the label for that entity type… 🙈), but then I think EntityLinks continues to make sense as the name in code?

Updating issue summary per #39.

First of all, thanks for all the hard work! Having this in core would be amazing!

I like "Content links" as it conveys the message. The term "Content" is broad enough here to contain more than just nodes from an average content admin viewpoint.

#35: I wasn't aware of CKEditor Entity Link tbh, I used CKEditor Link + User + File in D7 and (sadly) most of our sites are still on D7. When we started working to convert stuff to D8+ it seemed like Linkit was the way forward due to the sheer numbers of users it has so I've been trying to use it. (I work at a university with a small Drupal dev team... well, actually, the dev team is, uh, me... everyone else works on non-Drupal 😅).

Re: whether we're all saying the same thing in #27-29, I think so. My use of CKEditor Link File is also due to its ability to choose which method you want to present the link, either as a link to the entity (/media/id), as a path to the file (sites/[site]/files/file.pdf), or as a forced download link. My main complaint with CKEditor Link File though was that it was an all-or-nothing setting, you could choose from three link methods: File, URL, or Download and that applied universally to all files linked in CKEditor. You couldn't configure it per-link. 😢 I think that is the feature that we in #27-29 are requesting, and this may be possible already in Linkit.

#39: I think adding an additional "Download" toggle similar to the "Open in new window" toggle in this screenshot might work, but as you mention it also allows for the download of non-Media things. Not sure if that could "automagically" appear if they select Media only and go away if not.

#40: I think "Content links" works IMO, I'm not sure of any term more generic than that other than "internal link." I could go either way with content links or internal links. Thinking like a content editor, I'd say either one works, I'd assume that "internal link" means any type of link on my site (whether it be a node, media, paragraph, etc.). CKEditor Link uses the term "internal path" and I've had no confusion from editors about what that is.

in the screenshot above, internal path: any link to a file, user, node within Drupal. URL: external links (with the option to choose http:// or https:// protocol from select dropdown, but if you manually type it in there it selects it for you)

Ergh, linked to the wrong thing above and it won't let me upload the image by editing... adding here :D

#40: We've been using the terms "internal links" (that point to content from the site) and "external links" (that point elsewhere on the internet) for some time and it has been helping editors to understand the difference.

Turns out that switching to use Entity Reference Selection plugins was surprisingly simple! 🤩

Remaining things related to the conversion to Entity Reference Selection plugins:

1. Refine my last commit, which introduced class MediaWithLinkTargetSelection extends MediaSelection, to ensure that no link suggestions are made for Media entities that do not actually have a link. That is a , but I'd first like confirmation from an entity reference selection plugin expert such as @Berdir that this is direction he'd like to see this being taken, before spending a lot more time on a potential dead end.
2. Determine which entity types to provide suggestions for → I am currently thinking this should be a filter-level decision rather than relying purely on "common reference target"— just like CKEditor5ImageController::upload() is already checking whether image uploads are enabled. By default, only common reference targets are allowed, but you can change this to whatever you want.

Tested this out on Drupal 10.1x

On Node B
I could search for content in the link.
The alias appeared correctly.

On Node A
Changed the alias.

On Node B
Alias is updated properly.

Looks good so far!

This is fantastic work.

Adding to #3317769-43: Drastically improve Drupal's default linking experience in text fields → , the ability to select within the link dialog which type of link you want (e.g. for a given media item, do you want to link to media detail page vs file download) could be done in a follow-up issue. However, it might make sense to have some architectural review now to make sure it is possible in the future. For most of my sites a single link type will suffice, but there are occasional sites with e.g. a public media asset catalog where it would be nice to have that flexibility.

Options include:

• \Drupal\Core\Entity\EntityLinkTargetInterface::getLinkTargets() returns multiple links
• Allow multiple handlers in the link_target annotation, just like link templates
• Maybe integrate with the link templates themselves somehow, e.g. maybe I want a link to edit the given entity

#48: see at the bottom of #39, which lists 3 proposals. Looking back at it, the 3rd proposal does not allow choosing one media entity to be downloaded and not another, so that leaves only 2 proposals. The first 2 proposals are really only dealing with the UI part, not the "how to generate the link" part.

Well, the second proposal does deal with that part, but shifts that responsibility to a separate filter plugin, which is not consistent. I like your "multiple handlers" choice best, which would change it from:

 *   handlers = {
 *     "access" = "Drupal\file\FileAccessControlHandler",
…
 *     "link_target" = "\Drupal\file\Entity\FileLinkTarget",
 *   },

to

 *   handlers = {
 *     "access" = "Drupal\file\FileAccessControlHandler",
…
 *     "link_target" = {
 *       "default" = "\Drupal\file\Entity\FileLinkTarget",
 *       "download" = "\Drupal\file\Entity\FileLinkDownloadTarget",
 *     }
 *   },

(The link templates on entity type definitions are not an option — if they were I'd have picked that! The problem is that those link templates correspond to routes and routes must have a pre-defined URL structure and must be generated using the router. That's not the case here: file URLs do not involve the router at all and are often rewritten to be served from a CDN.)

But honestly, the bigger challenge here is how to surface this in the UI. As @dalemoore said in #43 (thanks for the thoughtful write-up! 👏), he really wants the ability to choose per-link whether it should be a "download" link or not. The first proposal I made in #39 would make it choosable per link. But the second and third would not. And that'd be equivalent to what CKEditor Link did in Drupal 7, but in Drupal 8/9 you would've been able to choose it per file thanks to the data-entity-substitution attribute. So channeling my inner @Berdir: this should support it too.

Thinking some more about how to make this UX possible… I'm currently thinking that for every suggestion, we also have a isDownloadable: true|false key-value pair. If that's true, we'd expose the "download" toggle that https://ckeditor.com/docs/ckeditor5/latest/features/link.html#demo-2 demonstrates. That'd mean we can't use CKEditor 5's built-in manual link decorators (at least probably not), but it should be theoretically possible. I'll see what I can cook up. 👨🏻‍🍳

It feels like this is the last stumbling block? 🤓 I've noticed that literally nobody has mentioned the title attribute that Linkit generates… 😇 Seems like people really don't quite use that?

Great job @Wim Leers! 👏

I really like how the link_target option implements the Substitution plugins as part of the entity system. 👏 It solves the problem with File and Media elegantly. 🤩 I definitely didn't think of MenuLinkContent + Shortcut before but it makes total sense that this could be used for those too. 🤯

The link templates on entity type definitions are not an option — if they were I'd have picked that! The problem is that those link templates correspond to routes and routes must have a pre-defined URL structure and must be generated using the router. That's not the case here: file URLs do not involve the router at all and are often rewritten to be served from a CDN.

The only concern I have at this point is that let's make sure that we focus on articulating clearly the intended use cases in \Drupal\Core\Entity\EntityLinkTargetInterface and link_target so that people understand how these are different compared to

links

. This quote here seems to do a pretty good job at explaining the differences conceptually.

Allow multiple handlers in the link_target annotation, just like link templates

💯 I really like this proposal for allowing multiple keyed handlers for specific use cases like downloading a file.

Adding to #43, the ability to select within the link dialog which type of link you want (e.g. for a given media item, do you want to link to media detail page vs file download) could be done in a follow-up issue. However, it might make sense to have some architectural review now to make sure it is possible in the future. For most of my sites a single link type will suffice, but there are occasional sites with e.g. a public media asset catalog where it would be nice to have that flexibility.

+1 to this.

I'm currently thinking that for every suggestion, we also have a isDownloadable: true|false key-value pair. If that's true, we'd expose the "download" toggle that https://ckeditor.com/docs/ckeditor5/latest/features/link.html#demo-2 demonstrates. That'd mean we can't use CKEditor 5's built-in manual link decorators (at least probably not), but it should be theoretically possible.

I'm wondering if there should be some site level control over this because I'd assume in most cases this would be fairly consistent for same type of data across the site. IMO this can be handled by a follow-up too, so long as we are on an architecture that provides enough flexibility for us to consider these different options in future.

I've noticed that literally nobody has mentioned the title attribute that Linkit generates… 😇 Seems like people really don't quite use that?

Our accessibility people discourage the use of the title attribute, to encourage our content people to use descriptive link text rather than relying on the attribute. I'm not sure if this is a common reason, but that's why we don't use it on our government sites.

#51: ah, TIL! That's a strong reason not to do it in core then! 👍 Thanks!

The 3 commits I just pushed fix a regression I introduced while converting to entity reference selection plugins: it would fail to find the currently linked entity based on the href attribute 😅

Fixed now, and simultaneously made it set expectations better: rather than pretending to link to /media/3/edit, which is blatantly wrong, I'm now generating an entity URI instead: entity:media/3. That's not familiar for the end user, but it's equally recognizable, it just doesn't set the wrong expectations. So it's IMHO a net improvement.

In the future we could switch to the actual link target, but that's not without downsides either. Curious what y'all think.

So, here's a PoC for the <a download> UX dilemma I mentioned in #49. It's rough, but proves it can work! 🥳

(The hardcoded part today: only allows File entities and bundle Media entities to be made downloadable — I'll need to add metadata to allow this to be set in a generic way. We have a plan for that already: the presence of a link_target.download handler. Which will work for everything except for Media — but that's entirely solvable. And it relates to what I said in #46.1.)

It does do all state handling correctly: shows the toggle when editing/creating a link to an entity that is downloadable, and retaining the current toggle state while changing the link to another entity that is also downloadable.

Here is a demo:

Looks pretty cool, no? 😁 (I neglected my actual responsibilities of today because I got excited about building this 🙈) Zero cognitive overhead when it doesn't make sense: the toggle is only presented when it is actually relevant. I think that addresses the concerns that @Berdir, @dalemoore and others have raised? To an implementation like this I am not opposed, precisely because we're not making the UX painfully complicated 100% of the time for the 1–10% that use this.

Next: implement the multiple link_target handlers and remove the hardcoded piece in the PR.

#53 looks amazing! You’re awesome Wim. That’s exactly what I was picturing it would work like in your demo. 😍

Re: the title attr, I don’t use it often either, but I have seen in the past it used by some sites to generate a clickable tooltip for different things using JS. I don’t think I’d ever do that personally, but that’s one use case for it.

Re #51/#52, I always thought that the title attribute is useful for accessibility, and "should" be used. But here's another claim that it isn't useful.

Some want to use title attributes for SEO purposes. They were always on the checklists for old-school SEO, although it's debatable whether search engines still give them any credit or not.

Personally speaking, I like them for the tooltips they generate, and they can be helpful for me as an editor to quickly know what my links are pointing at. I would very much like to be able to automatically use the name of the target entity as the anchor text for a link. I made a feature request for that on Linkit here → .

The title attribute would be useful for accessibility, but there's a huge variation in what, if anything, various assistive tech (and regular browsers!) do with the attribute. So the end result is that developers thinking they put useful info into the title attribute to "help blind users" is actually making it less accessible since that info is only rarely announced as the developer imagined it would. 😅 Now, the clear consensus of accessibility folks is to not use the title attribute at all, and make the actual text of each link descriptive enough to make sense on its own.

A related consideration is that many users with a screenreader (blind or otherwise) use a "tell me all the links on the page" style command (that nearly all assistive tech provides). This listing of links gets read "out of context", so each link has to be able to make sense on its own. These listings generally do not announce the title (although it's inconsistent). But this browsing methodology is a big part of why we need to make sure the link text itself makes sense, and why having 100 "Read more" links on your page is a bad idea. That's where the whole <a href="#">Read more<span class="visually-hidden"> about The Name of That Thing</span></a> practice comes from, instead of trying to put "about The Name of That Thing" into the title attribute...

Left some comments, leaving at NR to ensure more eyes on this.

@larowlan's review (responded!) made me realize that I have whew fortunately I did think of that back in #3 😮‍💨 — they did 99% of the JS work on getting the Linkit contrib module to support CKEditor 5 in 📌 CKEditor 5 readiness Fixed . 🤩 I'm less well-versed in the CKEditor 5 JS APIs than they are, and less well-versed in JS in general. So I'm hoping that either one of them wants to do a round of clean-up on the JS in this MR 😅🤞 I'm certain they would do a far better job than me.

I've also updated the issue summary with a list of contrib modules this functionality partially or completely replaces.

I will be out next week — it'd be great if others can push this forward and/or review this in the meantime 😄

Just pushed some code to add the .ck-reset_all-excluded CSS class to the entity link container. This class prevents CK5's CSS reset from affecting the styles within the container. Here's what it looks like now (updating IS)

I just want to say this is awesome and 100% should make it into core!

Suggestion: Finer grain control over entities that can be linked to

In most cases, I don't want my editorial team to be linking either to users, taxonomy terms, or custom entities. I would love the ability to restrict what can be linked to.

It would also be fantastic if we could limit by content type. Frequently there are "microcontent" content types that are solely used to be embedded in other nodes. Ideally these wouldn't be exposed for linking.

Suggestion: Preview link and title when selecting entity-linked content

When selecting linked text, Entity Link will give a popup that says entity:node/9 or something. This data doesn't tell the content editor anything useful. I'd love to have it show the same info that the dropdown menu shows (title, author, date, etc), and have it actually link to the content (open in a new window)

Just a test-review of MR2909 with Drupal 10.1-dev : Just work as promise ! Thanks, I'll see how I can help

Believe the 1 failure is legit.

Also this was tagged for additional tests which probably still need to happen (could be wrong)

Testing wise
It searches for content, media, and users. Wonder if could add a config to the editor to limit what can searched. What if an editor doesn't have permission to link to users?

This may be by design but noticed the filtering was case sensitive.

Scrolling was a nice touch.

Amazing progress!

As of the handler idea:

 *   handlers = {
 *     "access" = "Drupal\file\FileAccessControlHandler",
…
 *     "link_target" = {
 *       "default" = "\Drupal\file\Entity\FileLinkTarget",
 *       "download" = "\Drupal\file\Entity\FileLinkDownloadTarget",
 *     }
 *   },

Great pattern (and it would be even greater if all handlers were services like group does it, but that's another ceterum-censeo ;-)!
I have one concern though:
- To generate nearly identical target links in different classes feels like an anti-pattern, and it fragments code.
- It does not scale to a dynamic set of targets (i don't have a good example in mind, CDN links in different world regions might be one, but i bet soon s.o. will come up with sth)

Maybe sth like this

interface LinkTargetInterface {
  public function getTypeOptions(): ?array;
  public function getLinkTarget(string $type = 'default);
}

Just found this issue and pardon the random comment but this might be the most exciting proposed new feature in Drupal. Long Live Drupal. 🎉

FWIW, agree with #45 ✨ Drastically improve the linking experience in CKEditor 5 Needs work

We've been using the terms "internal links" (that point to content from the site) and "external links" (that point elsewhere on the internet) for some time and it has been helping editors to understand the difference.

Now working on:

1. download links support (see me in #39, @tedfordgif in #48 which proposed multiple possible implementations, me in #49 and @lauriii in #50 in which he expressed he preferred @tedfordgif's second proposal — so for now not going with @geek-merlin's suggestion in #64: that feels very complex for something that can be solved with creating a base class with tiny subclasses)
2. remove the hardcoded piece that was added in #53, to only make the toggle appear on relevant linked entities
3. documenting the intended use cases for link_target to avoid confusion with links per @lauriii's first response to a quote in #50
4. settings for limiting suggestions as requested by @mherchel in #61

Not yet working on as requested by @mherchel in #61, but we should totally do that too!

First pushing a rebase on top of latest 10.1.x (I'll rebase on 11.x later — this is hard-blocked on reviews anyway, so it's not urgent).

#68.1–3 are implemented 👍

Next up: filter settings as requested by @mherchel.

Will continue this tomorrow, but … meanwhile, what do you think about this proposed configuration UI, @mherchel? (And thoughts from anybody else are welcome too of course!)

Seeing the configuration UI concept makes me wonder about whether this should be something that is configured outside of CKEditor. Link fields already provide suggestions but I don't think there is any ability to configure it. I'm just wondering if the settings should be the same for both. Actually wiring up the config to link fields is probably out of scope for this issue, but maybe we should set this up to be compatible with a future state like that.

Seeing the configuration UI concept makes me wonder about whether this should be something that is configured outside of CKEditor.

If I'm understanding correctly, I'd worry that it might be a pain to jump around to the various entity types to configure this if this were the case.

Anyway, from my point of view, the UI laid out in #70 is absolutely beautiful! I'd love to see an option to selectively enable specific node types, but I can see that UI being really confusing. I like the help text at the bottom that points to a URL where developers can see how to do this, and I expect that contrib will create a module that does this though a UI.

Big +💯 from me!

I'd love to see an option to selectively enable specific node types

I would also add media types, comment types, and block types. But, as @mherchel mentioned, not sure a UI for that would be possible.

In the UI, I'm not sure the word 'entities' is necessary in 'Comment entities' and 'Media entities'. Could just be 'Comments' and 'Media'

Also, would it make sense to add 'Webforms' as an Entity Link Suggestion with logic to only include if the Webform module is installed?

I would also add media types, comment types, and block types. But, as @mherchel mentioned, not sure a UI for that would be possible.

Would a dialog be appropriate here? The checkbox label could have a Refine Configuration link that triggers the dialog.

Also, would it make sense to add 'Webforms' as an Entity Link Suggestion with logic to only include if the Webform module is installed?

I'm not sure. It probably depends on the Webform configuration. If you're using the Webform Node submodule, you'd probably want the nodes linked instead of the webforms. You'd also have to do access checking to figure out who has access to which webform.

Is there a hook or something so that a contrib module can add link suggestions?

If I'm understanding correctly, I'd worry that it might be a pain to jump around to the various entity types to configure this if this were the case.

@mherchel I wasn't thinking about a per-entity/bundle configuration form. Probably a basic version of what LinkIt provides where you can turn on/off entities to be shown as link suggestions. Whatever is selected would be used in providing suggestions for Link fields. The CKEditor config UI would just be to turn on the suggestions for CKEditor and it would inherit link suggestion settings.

LinkIt could still be used to provide more fine-grained control (e.g. having different profiles, more control over the output for each entity/bundle type).

@pyrello Link fields are a different beast altogether, see #16 :)

@mherchel is enthusiastic in #72 but would like to see per-node type configuration. @Chris Matthews echoes this and would like to also see it for other bundleable types. @e0ipso proposes a UI approach that could work but that would AFAICT be an entirely new UI concept in Drupal: I don't know of any form which essentially contains a subform that's not visible but only accessible via a modal dialog? Also, it'd imply that we'd need that modal dialog to alter form state of the actual form, to be able to collect that information, because the data entered in that dialog cannot be saved directly 🤔😅

So … for now going with #type => fieldset + #states:

This is really cool!

I think #80is the most usable solution to solve the problem at hand with the patterns we have nowadays in Drupal.

It looks great but beware of #states, since it only hides / shows the subform fragment, you may end up with inconsistent form states values if no cleanup is managed afterward (maybe you already did, i didn't examine the MR).

Use case would be:

* Check an entity type
* Check several bundles
* Uncheck the entity type
* Submit the form

The bundle might end up selected since they are still checked in the final submitted form.

Please ignore this if it's irrelevant with what has already been implemented, i just wanted to wave a caution flag!

And now it's actually respecting that configuration 👍 Including for bundles! 🤩 I'm done here for now. There are still clear next things that should happen:

• a nicer UX for entity links, so that not the actual link is shown in the balloon, but the entity title — requested by @mherchel in #61 → this requires overriding parts of the CKEditor 5 default Link plugin, so I defer to @lauriii to figure out if that is at all feasible 😅
• address @larowlan's prior JS feedback → since @nod_ and @lauriii are the primary authors of the JS, I'm hoping either of them can help address that 😇
• more test coverage:
  1. the "download" link toggle needs functional JS tests
  2. the suggestions controller needs a functional (maybe kernel?!) test to test the whole spectrum of configuration options and their impact on the suggestions
  3. the config schema needs kernel tests to verify all invalid configuration is detected

I'm hoping to find the time to tackle the test coverage part. But at the same time I want to make sure that this can actually land. I think we need a @Berdir review 🇨🇭🍫, since he's posted the most thorough, most critical review so far, in #18. I believe I've addressed all of his concerns. Perhaps with the exception of bringing this to link fields as well. With the shift towards A) entity reference selection plugins, B) link_target entity handlers to enable more complex use cases, C) configuration that is structured in a way that is not at all CKEditor-specific, I hope the current MR shows sufficiently clearly that we can bring the same UX to link fields. I'd rather not increase the scope to actually build this for link fields too, because this MR is already 2500 lines long! 😅

@ckrina Thank you for confirming that this is the best possible approach for now! 🙏😊

@Artusamak I'm very much aware! 😊 Because I've decoupled the design of the form from the design of the configuration, this very problem was avoided. Most configuration forms are 1:1 representations of the underlying config, which indeed leads to the brittleness you're referring to. The configuration you see at the end of the GIF in #80 results in this configuration:

    ckeditor5_link_entity_suggestions:
      allow_download_links: true
      suggestions:
        -
          entity_type_id: node
          bundles:
            - article
        -
          entity_type_id: file
          bundles: null
        -
          entity_type_id: media
          bundles: null
        -
          entity_type_id: taxonomy_term
          bundles:
            - pets

bundles: null means no bundle restrictions (bundles: [] is not allowed: either a non-empty array must be present or null, to make it an explicit configuration choice. This happens to match exactly how the target_bundles option for entity reference selection plugins already works! 🥳)

One last thing: as part of tying the last pieces together for bundle restrictions, I realized there is another thing that probably should be configurable: the priority of the search results. Right now, link suggestions to the same entity type as the host entity type are listed first. But other than that, there's no control over the ordering.

But wouldn't it be better to be able to specify the priority of each as well? For example, content entities before tags before media. And articles before pages, document and remote media before images and videos.

We already have a UI pattern for this: a table with drag-and-drop handles. And … in searching for that, I happened to find a UI that actually provides the nice modal dialog popover to select … bundles for an entity type (yes, literally the same use case!) in \Drupal\content_moderation\Form\ContentModerationConfigureForm::buildConfigurationForm(), which you can see at /admin/config/workflow/workflows/manage/editorial.

So …. yeah, I think I'm arguing I should scratch the UI I just finished 🤪😶‍🌫️ in favor of the table-style UI, with the same "Select" button to trigger a modal dialog, but with the drag-and-drop handles added to those rows. It'd be similar to the "list of entities with operations" pattern that we see everywhere (f.e. /admin/config/content/formats), but with the addition of the drag-and-drop handles to the beginning of each table row. To be fair, the form state problems I mentioned in #80 would still need to be solved, so it's not trivial. (But at least the config validation means that no validation logic needs to change at all! 🤓)

Thoughts?

@Wim Leers you're a legend mate! Love the progress, it's looking great.

This is amazing

Testing out on Umami install
Setup a Entity link settings for
Filter for Article content only

Creating a basic page
Adding a link
Search for a Recipe no results = Expected
Search for a Basic page, no results = Expected
Search for an Article, results = Expected

While updating text format I get this error, could be unrelated but wanted to note

Warning: foreach() argument must be of type array|object, null given in Drupal\Core\Render\Element\Checkboxes::valueCallback() (line 113 of core/lib/Drupal/Core/Render/Element/Checkboxes.php).

Also even though I've only selected content I now see

Seems like these should be hidden.

Updated filter to include Basic page
Creating a basic page
Adding a link
Search for a Recipe no results = Expected
Search for a Basic page, results = Expected
Search for an Article, results = Expected

Updated filter to only include Media, images and NOT content
Creating a basic page
Adding a link
Search for a Recipe no results = Expected
Search for a Basic page, no results = Expected
Search for an Article, no results = Expected
Search for Image, results = Expected
Search for Documents, results = Expected

Minus the one error in bold functionally this appears to be working.

Also drupalci.yml needs to be reverted.

For the ticket are additional tests still needed or can the tag be removed?
Can the issue summary be updated for the TBD sections.

This is looking great @Wim Leers!! super excited to see this land.

Admirable work, Wim! More to come as a dig into the code, but here's some feedback from functional testing.

A. Confirmations from functional testing
1. Media URLs work as expected, dependent on whether "Standalone media URL" is enabled.
2. Anchor links and query parameters can be appended after autocomplete and are preserved.
3. Custom entity types are available in the configuration UI and as autocomplete suggestions.

B. Minor bugs
1. "Download link" toggle, if set to "on," retains the correct value in the source but reverts to "off" in the UI when link is editing via balloon.
2. "Download link" set to "on" populates download="true" in the link, which results in a filename of "true." The filename should be the original file entity.
3. After saving a link, clicking to edit the link presents a functional link without a preceding slash (e.g., "node/1"). Clicking on this will result in being taken to something like "example.com/node/3/node/1". This is *not* a *new* issue introduced in this MR, FYI, but I didn't see an existing issue in the queue. I'm even sure whether there *should* be a functional link in that context.
4. Entity metadata ("by admin on Friday, June 30") does not show on entity types other than nodes, files, and media.

C. Minor enhancements
1. Add the standard "autocomplete" icon to the link input.
2. Currently, unpublished entities are shown as autocomplete suggestions. I think showing them *is* a reasonable default, since content editors frequently stage interlinked content. However, it would be valuable to indicate the unpublished status as part of the metadata shown in the matching suggestions. (Content moderation states are another conversation.)
3. Suggested rewrite of "Entity Links" description text: "Provides autocomplete-style suggestions for entity links from a list of allowed entity types/bundles."

D. Architectural considerations
1. Berdir has advocated for a standalone configuration entity that stores the link behavior settings, external from the text format configuration settings, largely on the principle that the same configuration should be available to link fields. Another benefit of this architecture would be that sites could configure once and then reference it across multiple text formats. My hunch is that on most sites, most text formats would want the same link configuration, so it's valuable from a UX standpoint. (It would also leave open the possibility, down the road, of referencing the entity linking settings on link fields, rather than closing off that possibility). Is there a technical reason that having the configuration within the text format settings is preferable? (To be clear, I do *not* think that the scope of the MVP should include link fields.)

E. Responses to comment thread questions

But wouldn't it be better to be able to specify the priority of each as well?

1. I agree: priority would be good, and that the drag-and-drop table UI is the convention. I don't think this needs to be in the MVP, though.

a nicer UX for entity links, so that not the actual link is shown in the balloon, but the entity title

2. I see potential disadvantages to showing the title. Many sites will have multiple entities with the same (or similar) titles. Seeing "United Kingdom" as the title and not knowing whether it's referring to a node or a taxonomy term, etc., would be problematic. Seeing /term/3 or /node/5 is at least unambiguous.

Great work! I also test on a clean install of the umami profile.

Why are content blocks available as a linkable entity? Blocks do not have url paths.

When I link to a content block in the wysiwyg, the rendered output points to the admin configure form for the block. I think this behavior is of limited utility, and potentially confusing for many users.

I think content blocks should be removed from the available entity types that can be linked.

See https://www.drupal.org/project/drupal/issues/3342998 ✨ Add view tab + standard template for block content Needs work which should make the blocks better.

@smustgrave in #87: Thanks for the detailed testing! Reproduced the PHP warning you got, fix pushed. More tests are still needed IMHO, so the tag is still relevant.

@mark_fullmer in #88: Thank you so much, especially for B!

• B.1: I cannot reproduce this — can you provide detailed STR?
• B.2: SUPERB catch! 👏 Rather than specifying a filename (which could change), I'd much rather just specify it without an attribute, because per the HTML spec, <a download> is a boolean attribute. AFAICT https://ckeditor.com/docs/ckeditor5/latest/api/module_link_linkconfig-Li... and https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_view_downca... do not support these. So reached out to the CKEditor 5 team to get guidance 👍
• B.3: Another good point! Nope, this is actually caused by \Drupal\editor\EditorXssFilter\Standard::filterXss()! A simple addition to core.serv ices.yml fixes that. But that still leaves CKEditor 5 very much wanting to open that link as a relative link in a new tab 😅 We'll need to intercept that and point to the (current!) path alias. I've added preparational work to improve this, but I'd rather have a CKEditor 5 JS expert like @lauriii write this logic 😇
• B.4: But not all entity types have meaningful metadata. See \Drupal\ckeditor5\Controller\EntityLinkSuggestionsController::computeDescription() — it already does one of these 3 if possible:
  

    if ($owner && $creation_datetime) {
        return $this->t('by :owner on :creation-datetime', $args);
      }
      elseif ($owner) {
        return $this->t('by :owner', $args);
      }
      elseif ($creation_datetime) {
        return $this->t('on :creation-datetime', $args);
      }
  

  So … suggestions? 😊

• C.1: 👍 but … 🤔 would it look out-of-place in CKEditor 5? 🤔
• C.2: I think you were testing as an admin user and hence ran into \Drupal\node\Plugin\EntityReferenceSelection\NodeSelection::buildEntityQuery()'s logic? But you certainly describe a nice-to-have.
• C.3: 👍
• D.1: this would imply a kind of "inherit settings from other config but optionally override it"-style approach. That's something that does not exist anywhere else in Drupal core. But you're right that it's not technically impossible: we could achieve it through a combination of config_dependencies and optional overriding. Except that config dependencies can only declare a dependency on config entities, not simple config. So both the technical and UX parts of these would be entirely novel approaches AFAICT. (The only thing that comes kinda close is the "Inherit exposed filters", "Inherit pager" etc settings in Views I think, but Views has the concept of a "Main display" vs other displays, so it's inheritance within a single config entity. So still very different.) There are many things in Drupal core where ideally you'd specify some setting once and then re-use it everywhere. All throughout the Field UI and the Entity View/Form Display UIs one can find many examples of this. IMHO it's a problem independent of this one. I think the very example you give even indicates this already: → most of those text formats will have similar or identical settings for most filters, typically with different "levels" of allowed HTML tags. The exact same kind of "inheritance" would be helpful there. Unless somebody can think of a conceptually clear way to implement this, I think it'd be unfortunate to block the benefits this issue brings on fixing a problem that is pervasive throughout Drupal already.
• E.1: 👍
• E.2: very good point. Thoughts, @mherchel?

@daddison in #89

Can confirm the issue I was seeing has been resolved and everything still functional.

I'm not sure why D1 would require the ability to override the referenced "link configuration"? If you want something else, you'd create a different configuration entity then and select that.

Regarding: B.1 ("Download link" toggle, if set to "on," retains the correct value in the source but reverts to "off" in the UI when link is editing via balloon), here's a demonstration of the problem as I'm encountering it (latest commit to Drupal 10.1.x, latest commit to this MR). Based on my experience, there are no 'special steps' involved here, so I'm not sure why it may not be reproducible by others.

1. Configure a text format to use the new "Entity link suggestions" text format filter, with "Allow the user to create download links" enabled.
2. Create a file entity.
3. In a CKEditor-based text field, click the "Link" icon and start typing something that will suggest the file entity. Accept the suggestion. The "Download link" toggle should be set to "on."
4. Save the page.
5. Return to the edit screen of with the CKEditor field.
6. Click the link.
7. Click the "pencil" icon to edit the link.
8. The "Download link" toggle is now set to "off."

Regarding the most far-reaching point of still-debated architecture -- that of whether to have the link configuration be external from the CKEditor configuration setting -- Wim's points in D.1 of https://www.drupal.org/project/drupal/issues/3317769#comment-15135481 ✨ Drastically improve the linking experience in CKEditor 5 Needs work make sense to me now: there are many other things in core where it would be nice to have inheritable settings from an external config, but I accept that it is not a paradigm that works for core architecture. Put another way, it may be fine for a contrib module like Linkit to take that approach, but that doesn't mean the same architectural approach will follow core patterns.

So, for the record, I'm on-board with moving ahead with the current approach -- that of defining the config for the link settings within the CKEditor plugin config.

@Berdir in #94: I see, I had not understood your proposal that way! So you're proposing to introduce a wholly new configuration entity type that this CKEditor 5 plugin would depend on. That could work! In theory, at least.

In practice, I worry about:

Of course, you can flip the UX argument around: some prefer disjointed-but-once-and-reusable, others prefer

I'm not opposed to it. But I personally don't believe it's the right trade-off. It reminds me of extreme database normalization. I think that which link suggestions (entity types + bundles) are desirable is highly contextual, and so will likely be subtly different per link field. For text formats/editors I agree there's likely less need for contextual variation (because the context is unknown!), but … repeating the same configuration for the few text formats/editors like you already have to doesn't seem like that big a deal?

IMHO it boils down to this: do we want to block this issue on a nicer admin UX ("configure once, reuse") before we can ship a better content creation UX (the demonstrated linking experience)?

@mark_fullmer in #96: thanks! (And will look into #95 in the near future!) What you write in #96 is rather different from what @Berdir writes in #94. I'm curious if you had written #96 after having read #94 (and hence disagree with @Berdir) or if you perhaps overlooked #94?

Everybody else: please chime in! 😊🙏 I want to make sure this addresses the needs of the majority!

I'm not necessarily in favor of blocking this issue in favor of a nicer admin UX, but I do think there is something to the idea of moving in that direction eventually. My point earlier is that there is already link suggestions built into link fields in core. It seems like the user might expect, out-of-the-box, that link suggestions in CKEditor would work the same as the link suggestions in those links. It might be surprising to discover that you had no control over link field suggestions and that the best you could do to make CKEditor link suggestions work similarly is to configure them as close as possible.

I think this is obviously a much bigger issue and probably out of scope for what is being done here.

Since it's been a few weeks seems the agreement is to least forward with this improvement. Should follow ups be opened for others?

@wim.leers are additional tests needed?

Moving to NW for the additional tests if needed.

As far as #97 I think even if it can be improved later, it would be good to get in and provide users with a much desired feature.

@wim-leers, MASSIVE THANKS for all your time and energy in making this issue a priority. It’s been a long time coming! As stated eloquently in your issue description, this will go a long way to truly enhance the “M” in CMS.

As for #5 in Proposed Resolution, may I suggest a few adjustments based on many years as a site builder who trains many other site builders. In our enterprise/university setting, we often find that content editing is delegated, but site management is not. This is an effort to protect the site’s integrity, specifically the Information Architecture (IA). By limiting who can add/delete nodes but allowing the content editors to “fill in the blanks”, site managers are able to control the IA while delegating content maintenance. This delegation often reduces the usefulness of entity “author“ and “create datetime”, as they carry little meaning going forward.

With the expectation that we don’t want to overload the dropdown list with too much metadata, I suggest replacing the “who” of entity author with the “status” reflecting the Published/Unpublished state of the node, as mentioned in C2 by my colleague @mark_fullmer https://www.drupal.org/project/drupal/issues/3317769#comment-15132055 ✨ Drastically improve the linking experience in CKEditor 5 Needs work

I’m hopeful this is possible/easier with the landing of https://www.drupal.org/project/drupal/issues/3073554 ✨ Add a token for publication status Fixed !

With regards “when”, I believe the datetime stamp of the most recent Published/Unpublished status change would be most useful.

Lastly, if a “who” needs to be included, I suggest the last person to change the Published/Unpublished status (which I deduce would involve revisions info and may be too much for this effort).

Again, THANK YOU very much for spearheading this most important feature!

omkar.podey → made their first commit to this issue’s fork.

Now we have a failing test so in \Drupal\Tests\ckeditor5\Kernel\EntityLinkSuggestionTest we want \Drupal\ckeditor5\Controller\EntityLinkSuggestionsController::suggestions to actually use $host_entity_langcode and fetch the correct results.

Picking this up again! Since #30 ~6 months ago this issue doubled in follower count and is now the 19th most followed open issue in Drupal core 🤯 It's only 13 followers behind SDC now (161 vs 174) 😦

I last worked on this ~3 months ago — see #83 for where I left things, with minor follow-ups in early July.

That lists 3 areas of improvement. 2 of which I deferred to CKEditor 5 JS expert @lauriii, who wrote most of the logic for linkit, plus more test coverage:

1. "download" link toggle needs functional JS tests
2. suggestions controller needs a kernel test — thanks @Utkarsh_33, @tedbow and @omkar.podey, this now has a start … but it's nowhere close to being done: it doesn't come close to doing . That means this must exercise the full gamut of possible configurations in ckeditor5.plugin.ckeditor5_link_entity_suggestions AND test edge cases for them. Right now, it's only testing 'ckeditor5_link_entity_suggestions' => ['allow_download_links' => TRUE, 'suggestions' => NULL,],.
3. config schema needs kernel tests

#84 lists one more thing: the ability to specify a priority for each entity type+bundle suggestion. But that can be left to a follow-up? Thoughts?

Self-assigning for tackling the test coverage.

Last but not least: FieldType=link support, which @Berdir and others feel strongly about. But it's hard to gauge how many people think that should block this. Based on @smustgrave in #100, it seems at least some people are in favor of landing this issue without support for the Link field type. Do we consider that a hard blocker or not?

I think this is actually a more accurate title? The title I picked in #16 was still too broad.

I'll continue working on the missing test coverage. Also, I think that all of the "improve JS" bits mentioned in #83 (the part of #61, plus tightening the JS per @larowlan's review) could IMHO happen in follow-ups.

Also, the last JS-related remark from @larowlan was this one: … and the kernel test part is now done, but the functional JS test is still present. And that's exactly what (randomly) failed just now! 😬 Will tackle that too.

FieldType=link support, which @Berdir and others feel strongly about. But it's hard to gauge how many people think that should block this.... Do we consider that a hard blocker or not?

I don't think it should be a hard blocker. Assumptions/rationale:
1. An future enhancement to link fields that make them behave like this CKEditor interface doesn't need to have the same implementation.
2. In the meantime, people can use the contrib module Linkit for this.

I don't think it should be a hard blocker

+💯

Perfect is the enemy of good. That can come later IMO

#109 + #110: noted. 👍

Just pushed the missing test coverage 🥳

AFAICT the remaining feedback since #83 that has not been addressed:

1. @mark_fullmer's bug report in #95. I can reproduce it. We still need test coverage for that (and a bugfix).
2. @texas_tater's in #101 (thank you!). Note that thanks to @Berdir in #18 and #21, which is why since #46 EntityReferenceSelection plugins are used to provide link suggestions. That means that unless you have the bypass node access permission, which usually only is user 1. I think 99% of Drupal sites do not grant content creators that permission? (See also #88.C.2 by @mark_fullmer, and my response to it.)

   Furthermore, removing the name of the author seems to be something that most sites would not want…

   So IMHO in your use case, it seems reasonable to instead override the ckeditor5.entity_link_suggestions route's _controller: '\Drupal\ckeditor5\Controller\EntityLinkSuggestionsController::suggestions' to point to your own controller which subclasses that class and overrides \Drupal\ckeditor5\Controller\EntityLinkSuggestionsController::computeDescription(). Then you can use arbitrarily complex logic to generate suggestions tailored with the knowledge about both your data model and your end content creators!

P.S.: I've not been able to reproduce the sole failure locally. Can anybody else reproduce this? 😬

Wim Leers → credited witeksocha → .

Thanks to @witeksocha from the CKEditor 5 team, I learned that a "boolean attribute" (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean...) can be created in CKEditor 5 by setting the attribute value to ''. Which is apparently a thing in the HTML spec too: https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute#pa... — so weird! 🫣 TIL.

(I pinged them long ago, thanks @mark_fullmer: he had already pointed out the incorrect attribute and the resulting behavior in #88.B.2.)

The bug in #95 was caused by my implementation (af48eacb from March 29) blindly assuming that true and false were valid attribute values. As explained above, that's wrong. true gets cast to the string 'true'.

So test coverage for this is already present, and we can just tweak it:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\EntityLinkSuggestionsTest::test
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-''
+'true'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/EntityLinkSuggestionsTest.php:219

Next: push the fix! 😊

Slightly adjusted copy & paste from slack.

> FieldType=link support, which @Berdir and others feel strongly about. But it's hard to gauge how many people think that should block this.... Do we consider that a hard blocker or not?

that is not what I'm suggesting. I'm just suggesting that we allow to configure it in a way that is extensible and the link field can build on. once the configuration structure is in place, it is very very hard to change it. so adding it custom now for text formats, and then refactoring it later isn't going to work imho.

It might be less of an issue in the text format UI, but putting a whole configurable entity type/bundle UI into a formatter settings UI, which is quite limited in space, is going to be painful.

> #97: because it'd imply a disjointed experience: first go configure this new configuration entity type somewhere else, then come back here. Is that truly better than configuring the same stuff multiple times? Because … this perspective on how it should work is very different on how core does this in general — that's why I wrote that long D1 response in #91

I don't really get the argument that it would be a new concept for core to first configure something and then use it. many things work like that? you set up an image style and then select that in an image formatter. and that image formatter is part of a view display, that you then select in a view or in an entity reference formatter to use it.

> it's currently impossible to define config dependencies from within a text editor plugin for a Editor config entity 😬😬😬😬 See #2950795-29: CKEditor 4+5 plugin module dependency not added to text format configuration

I see how that's an issue, and I didn't look to closely at the current implementation a while, but fwiw, entity types, bundles and selection plugins *all* are things you already would need to depend on as well. Entity types are plugins provided by certain modules, so you'd need to depend on those modules too if you want to do it correct.

If I'm the only one to think like that then I'm not going to hold this up on that. but I I really think that core should provide extensible and flexible systems that contrib can build on and doesn't have to redo things from scratch. I'll try to do another code review as well when I find the time. Haven't done that since #18.

I went through the issue and found some remaining things @wim.leers , is there anything that should be prioritised from this list or something remaining not mentioned here ?

1.  The priority of the search results, if we want to implement it.
2. Whether to have the link configuration be external from the CKEditor configuration setting ? (Probably no but open for discussion)
3. No fix for this yet ,The bug in #95 was caused by my implementation (af48eacb from March 29) blindly assuming that true and false were valid attribute values. As explained above, that's wrong. true gets cast to the string 'true'. , needs test tweak.
4. Reply to @Berdir's comment worrying about hindering others building on top of this.

#114:

but fwiw, entity types, bundles and selection plugins *all* are things you already would need to depend on as well

That's fair. In what's below, that's definitely taken care of 😄

#115:
Thanks, @omkar.podey, that's very helpful and much appreciated!

Just spent 1.5 days working on this:

Merged in origin/10.1.x — since the last commit (Sep 19), Drupal core adopted GitLab CI, and so now this MR gets GitLab CI too 🥳 Much faster test results 👍

#115:

1. Priority 👈 DONE
2. config as separate config entity 👈 DONE
3. bugfix for #95 — I'd rather leave that as one of the things remaining to be tackled by a person with stronger CKEditor 5 JS API skills than I have (to avoid double work), so did not yet address that.
4. Berdir's contrib concern should be largely mitigated by 👆

I still need to work on updating the actual suggestions to respect this, but … here's a demo of where things are at — I think y'all will like it 😄

We already have a UI pattern for this: a table with drag-and-drop handles. And … in searching for that, I happened to find a UI that actually provides the nice modal dialog popover to select … bundles for an entity type (yes, literally the same use case!) in \Drupal\content_moderation\Form\ContentModerationConfigureForm::buildConfigurationForm(), which you can see at /admin/config/workflow/workflows/manage/editorial.

👆 This (from #84) did not pan out. It's far too complicated, with additional modals, additional routes, etc. After I scrapped that, I built on top of '#type' => 'field_ui_table', 😅 That also didn't pan out, because it has a weird "parent" thing (that I still don't understand). So I went with a hybrid of all that, with a much more compact UI. And thanks to some of the infra it makes available, it also is able to provide a guaranteed consistent UX across the different screens 😊

What do you think?

I think y'all will like it

😍😍😍

This will be a really great addition, very exciting!

Based on the GIF in @Wim Leers comment, one minor concern I have is the use of striking-out entity type names that are disabled. This isn't usually a pattern that we would use, and could be a problem from an accessibility perspective. As in, we still want people to be able to read and understand what the currently disabled options are so that they can decide if there is one there that they want to enable, but striking those options makes it harder to read them. I think have a checkbox is enough to communicate the state.

Thanks

All done. The experience for the end user is identical to what it's been for many months now — the only change is that now priorities are respected.

I did improve the admin UX slightly more after #116:

Next up:

1. Update test coverage, because the configuration now lives in the new entity_link_suggester config entity type, not in the ckeditor5_link_entity_suggestions CKEditor 5 plugin
2. Change target branch of MR to 11.x — I cannot do this, only core committers can
3. Address JavaScript feedback — see #83 for why I am not the right person to address that

Also, one other thing, on the editor configuration screen, where the user is selecting the link suggestion. The "create" option, I don't think should be a radio button. The way I understand the pattern, it can never be selected, so as such I see it shows as a disabled element, but we generally do not recommend using disabled form elements, as different browsers can communicate those differently and it may not be clear to all users that the option is disabled. Having the create option as an action link or a normal link may be a better pattern.

@AaronMcHale in #118: Agreed. I'd prefer to use the field_ui table pattern. I already spent half a day fighting that the various table UI mechanisms, plus #type => radios, plus opening this in a modal dialog (and I failed to make it update the underlying page — I can't find a single example in core), plus the use of #type => tableselect not working with #states. IOW: I've gotten an accelerated course in finding broken edge cases in Form API, AJAX API and DX 😅 I need to get something out there that conveys how I envision this before spending days on making that perfect.

If we're going to add this new config entity type, it for sure would need to happen in a separate, hard-blocking issue anyway, where the relevant experts in those subsystems would help us get all those details right :)

@AaronMcHale in #120 (we cross-posted just now): Agreed again, but unfortunately core has AFAIK zero examples of the "reuse or create" UI pattern — kind of like https://www.drupal.org/project/select_or_other → . This was again the best approximation I could come up with in limited time.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

@AaronMcHale in #118: Agreed. I'd prefer to use the field_ui table pattern. ... I need to get something out there that conveys how I envision this before spending days on making that perfect.

Yep that's totally fine :)

If we're going to add this new config entity type, it for sure would need to happen in a separate, hard-blocking issue anyway, where the relevant experts in those subsystems would help us get all those details right :)

+1

Agreed again, but unfortunately core has AFAIK zero examples of the "reuse or create" UI pattern

That's a great point, probably something our user interface standards should include in the future. Looking at the select_or_other module you mentioned, based on the screenshot on the module page, I could see that pattern working okay because it's possible to interact with the "Other" checkbox. I think in this case if it the user could actually interact with the "create" radio button, then yeah it would probably also be fine.

Hopefully we don't cross post this time 😀 And overall I think we're on a really great direction with this, excited to see it land, definitely something Core has needed! Keep up the great work!

Here's a quick UI review. Note that this doesn't apply to 10.2.x or 11.x, but it does apply to 10.1.x, which is how I left my review.

1. This link doesn't work. It links to the same page. It's especially confusing because when you hover the mouse over it, the tooltip says "open link in a new tab"

2. The modal's form doesn't have a "submit" or "create" button.

3. How does the option for "Allow the user to create download links" work? I didn't see how to do it.

4. The help text of the "Entity Links" checkbox doesn't tell the site builder anything helpful.

Maybe something like "Provides an autocomplete interface for creating links to Drupal entities that will auto-update when the entity's path changes."

5. If you create a new suggester via the modal, the page reloads and the original suggester is still selected. The newly created one should be selected if possible.

Overall this is looking amazing! Thanks for the hard work on it!

#125:

1. Yep, this was mentioned before. My CKEditor 5 JS-fu is not strong enough for this one.
2. Huh! I cannot reproduce that. I see a button. I bet something went wrong with applying the patch to 10.1.x. Until a core committer has chaned this MR to target 11.x and the MR has been updated, I recommend to test only a checkout of the actual MR.
3. It only is offered for links that point to an entity type + bundle that actually supports downloads. Supported on: File entities (all of them), Media entities that use the File MediaSource. Out of the box that is true for the "Document" media type for example.
4. -1 for that literal proposal, +1 for the idea behind it: improving discoverability of this functionality 😄. This is the filter plugin, not the text editor plugin. This is a consequence of "Text Format" and "Text Editor" being distinct concepts that are weirdly connected in the UI only. Solution: 🌱 [META] Discuss: merge the Editor config entity into the FilterFormat config entity Active . For the scope of this issue, we can tweak the description though, to something like — WDYT? 😊
5. Yes 😞 I fought tooth and nail and had to give up after having spent hours on this (see #121). That is exactly what I had envisioned, but failed to implement! 😄

By changing the branch from 10.1 to 11 the patch failed to apply because of the conflicts.
We heavily use this functionality on our 10.1 website. Can we have it back for 10.1?

We heavily use this functionality on our 10.1 website

😳

Attached is a 10.1.x patch reflecting the latest state of the MR. Note that it's never safe to apply patches directly from an MR, since the MR keeps changing. That is a much safer patch to apply 😊

Thank you for the patch. That saved my day.
Will revise all my patches using a pull request (lesson learned).

@Alina Basarabeanu FWIW, we have started copying patches into our project repo rather than pulling them from drupal.org or the Drupal Gitlab instance. That avoids having to make a copy of the MR diff and post it into the comments of an issue, which ultimately creates more noise for folks that are trying to parse through an issue and make sense of what they should use for a patch.

WRT #125.5: looks like ✨ Use modals in field creation and field edit flow Needs work could be introducing the infrastructure that will make this easy to achieve, because it has a similar UX, but an entire team on it! 🤩🤞🤞🤞

Hi @wim-leers
How can I generate a patch for the Drupal 10.2 version?

@alina-basarabeanu This patch works for me on 10.2

Many thanks, @ben_a

My git-fu is not strong enough to merge origin/11.x into the MR that was originally against 10.2.x 😅

So replayed all commits excluding the merge commits against the 3317769-cke5-entity-link-suggestions-11.x branch and created a new MR: https://git.drupalcode.org/project/drupal/-/merge_requests/6277. Only 2 conflicts, with:

• ✨ Add Media revision UI Fixed
• 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed (conflicted with 36682c4c45 because it was a forward port of 10.2 — just omitted that commit 👍)

Finished what I did in #116 through #119 — now the new test coverage should pass again. Which makes it ready for @bnjmnm to take over 😄 @bnjmnm, please convert the old MR back to 10.1.x target branch so that you can actually see @larowlan's feedback again in a digestible format 🙏

🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active caused the spellcheck job to fail … hard 😅 Please ignore the 11.x branch I just created; that's the recommended work-around right now.

I've finished getting this MR to green. That means everything except for the JS is ready. I do realize that we'll probably want to extract the new EntityLinkSuggester config entity type into its own (blocking MR), but for now it's AFAICT better to keep them together, because on its own, such an MR would not make sense 😅

Over to @bnjmnm for getting the JS in tip-top shape! 🥳

GitLab CI reports 100% success rate but a job failed 🤔 This is due to a bug in the GitLab integration:

    Remaining self deprecation notices (1)
    
      1x: Deprecated NULL placeholder value for key (@name) in: "The '@name'
    config does not exist.". This will throw a PHP error in drupal:11.0.0. See
    https://www.drupal.org/node/3318826

is not a test failure but triggers a job failure 😅

This is caused by a small bug in ConfigExistsConstraintValidator. Fixed here, but created issue to fix it elsewhere: 🐛 ConfigExistsConstraintValidator should ignore NULL values Active .

Calling out an edge case where this code is problematic: when a Drupal media item is the selection that the link is targeting, the ranges here are not populated, and the data- attributes are not added to the link.

Glad this was mentioned while I had some time to work on this. I have this largely working locally and will push once I've addressed some details. This particular issue is much easier to address when all of the functionality is provided by core plugins, so an especially good call on core-ifying this linkit feature.

🐛 ConfigExistsConstraintValidator should ignore NULL values Active , making this one a tad smaller! 👍 (I just merged in upstream.)

There is a Search component now, with a demo in https://ckeditor.com/docs/ckeditor5/latest/features/ai-assistant/ai-assi...

Can we look into the possibility of using this component?

If an AI component is integrated, then we need to include a way to disable it. Private sites with proprietary data might not want to have that data sent to/through an AI, and it might even be required by law that it isn't.

Might be slightly off topic here TBH, perhaps could move the #142 conversation to a separate issue in the openai module.

Not necessarily talking about AI, it's the component used by the AI plugin which is Search along with AutocompleteView and AutocompleteResultsView.

The screenshot is just to demonstrate this component and the views in action which is used by the AI plugin.

Ah I see, makes sense, thanks for clarifying!

Thanks, @el7cosmos! Good call. Curious to hear what @bnjmnm thinks 😊 I'm willing to bet that @bnjmnm would say that it'd need thorough accessibility review, and that it is very unlikely the CKEditor 5 team got everything right, and that hence it's better to stick with jQuery UI Autocomplete, which has been thoroughly vetted.

@solideogloria I think you misunderstood: this is about using the dropdown UI component, not the AI plugin.

I've largely addressed the JS needs that this issue was assigned to me for, and there are added FunctionalJavascript tests for that functionality. There's one last @todo in the JS I need some clarification on and will ask @wim.leers about that.

The big JS stuff that made it useful to assign the issue to me is bascially done, though. Unassigning myself but leaving at NW since there are still todos in .php and .yml to address before this can switch to "Needs Review".

Thanks so much for pushing this forward, @bnjmnm, I couldn't have done all that! Digging in … 🤓😄

bradjones1 → changed the visibility of the branch 11.x to hidden.

bradjones1 → changed the visibility of the branch 3317769-cke5-entity-link-suggestions to hidden.

Rebased and rebuilt the ckeditor JS artifacts. Let's see if the test baseline changed. I'll take a stab at some of the non-JS @todos, as well.

Tagging needs IS update for remaining tasks. I am investigating this issue on behalf of a client and would potentially be helping move this along.

Rebased and it looks like there are just a few errors relating to schema validation against the current test coverage.

https://git.drupalcode.org/issue/drupal-3317769/-/pipelines/201317/test_...

This feature would be an excellent addition to the core. I tried it out locally and came across a few issues (maybe not issues they could be intended behaviors :)).

1. Added some random text saying "random string" to the "Link URL" field, it is showing the message that No content suggestions found. This URL will be used as is.which is correct
2. However, when I clicked the green checkmark button, it didn't stop me from adding the string, even though it wasn't a valid URL.
3. Additionally, it treated the string as an external URL since it didn't include "HTTP(s)" or "www." If the string is allowed as a "Link URL," it might make more sense for it to be treated as an internal link.
4. Lastly, I think it would be a nice enhancement to have the option to specify the target attribute (e.g., _blank, _parent, _self, _top). This point I see already been discussed, as I noticed the "link_target" aspect mentioned in the issue descriptions:

Thank you!

In response to #156, nr. 1-3, please do NOT make any assumptions on link.

"random string" is a valid link. It will be url-encoded to "random%20string" though, but it is a perfectly valid relative url path. This is clearly a user mistake, not a bug. In our projects, we use and need all flavors of relative, absolute and schema links:

current-path-relative-links
/domain-relative-links
https://full-uri.com/link
whatsapp://send?phone=1234
mailto:john@doe.com
tel:01234567890
/taxonomy/[current_node_token:field_foo:entity:id]

@hudri

Correct me if I am mistaken but my point (in point 3) is the relative URL should be treated as internal only but currently it is getting converted to an external URL like "https://randomstring" which is not correct in my opinion.

"random string" is not a relative link though, there is no leading slash. If you tried "/random string", that would be an internal link.

"foo" is a path-relative link. Like domain.com/sub1/sub2/sub3 => domain.com/sub1/sub2/foo
"/foo" is a domain-relative link. Like domain.com/sub1/sub2/sub3 => domain.com/foo
Different things, both valid and both have their use cases.

Thanks @hudri, yes I meant domain relative :) This functionality works exactly the same way as Linkit, so I do not think there is anything further to discuss and don't want to create more noise, this issue already has 161 comments!

the needs another rebase. after adding when i try to checkout the branch i run into the following error:

$> git checkout -b '3317769-cke5-entity-link-suggestions-11.x' --track drupal-3317769/'3317769-cke5-entity-link-suggestions-11.x'
error: The following untracked working tree files would be overwritten by checkout:
	core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Connection.php
	core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Install/Tasks.php
	core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Connection.php
	core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Install/Tasks.php
	core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Connection.php
	core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Install/Tasks.php
Please move or remove them before you switch branches.
Aborting

it happens when the issue fork was created or last rebased before this issue went in https://git.drupalcode.org/project/drupal/-/commit/8b368d712d83900765744 on the 13th of august fixing spelling errors. the switch to camel case is the problem where the none case sensite file systems ( i am on a mac for example) in combination with git struggles and leads into this error.

1. Branch is again up-to-date with 11.x
2. Fixed new test failures / cspell reports / coding standards
3. Fixed merge conflict in system.install

If ability to set the link target is added, please make it configurable as to whether it is available or not, to allow setting a site policy, for example:

• Allow editors to set target
• Always set target _blank on external links
• Don't allow setting target

The ability to set the link target will not be added here.

Tested this manually, the link suggester settings are not saving correctly -- you can unselect a content type but this change doesn't persist:

I've also tried to update the IS to make it a bit easier to read, but it still probably needs more work.

Separate to the functional testing I have some overall feedback, but just wanted to say it's really awesome to see this getting so close to ready!

The 'Everything!' label feels a bit unserious for the UI. Plus, it's only everything that core provides, right? If you are installing this on a site with other targets, this wouldn't be everything?

I'm -1 to enabling everything by default anyway, it would seem much better to enable the common things: content and files and *maybe* taxonomy terms? Users doesn't strike me as a commonly linked item either, but files does, so I'm not sure what the 'commonly linked' descriptor is based on? I think shortcut sets and comment entities should definitely be disabled at least.

The link itself is prepended with entity:, which is different from Linkit and IMO this is a Drupalism that we should avoid.
Linkit:

This MR:

I've taken a lookt at the aural interface in voiceover on macos sonoma (see voiceover.mp4), a few observations:

• i've just entered a single character to get more results across different groups (tested on a test instance without any content), but somehow after entering the single character "n" the announcement providing the number of results and the instructions how to navigate the list right after is being cut off somehow.
• The group titles like content - article, contact form, file and so on are unavailable in the aural interface.
• The meta data of the username as well as the date and time an entity was created is unavailable in the aural interface
• The file name and the name of media item are identical but the name is only announced the first time either way (going downwards same as going upwards), the second time remains unannounced.

and unrelated to screenreaders, woult it make sense to leave a pointer either on admin/config/content/link-suggesters or on the link suggestor edit page either in a help text or a description pointing out that for activating the link suggestor the "Entity Links" checkbox has to be ticked on a text format? At the moment that fact is implied.

Has there been any consideration about showing something other than node/123 or entity:node/123? This is something I get asked/complained about most by clients and editors with the Linkit module in D10.

A node number means nothing to them. They want to know what it links to (page title or path alias) and they have no way of finding this out when editing, besides opening the link.

Maybe it's a niche request. For me, the way it works is totally fine, but I think less technical people don't know what "/node/2934" means and they want to know from within the editor what page is linked to. The autocomplete when adding a new link works amazingly.

This seems almost perfect what I would currently need in one of our project.

My suggestion is that this link could also optionally be utilising the UUID of the target node as in JSONAPI:Extras UUID link enhancer can output:
entity:node/{{contenttype}}/{{uuid}}

In the project I am currently working on, we have this need for the UUID in the links inside the text. Would it be possible to implement or could someone point me to correct way how this would be possible?

@rob230 that makes sense to me, sounds like a great follow up feature!

Have discussed this with @lauriii and @bnjmnm at DrupalCon Barcelona. Here is a summary:

1. We are not sure whether it's necessary to be able to configure multiple suggesters, since it's only configurable once on the text format. The use case for this is different text formats with different linking needs, which seems very much an edge case.
2. However, since it's already built this way, it may not be worth undoing.
3. Assuming we keep support for multiple suggesters, we should create two by default. One with just 'Content' and one with 'Content and files'.
4. Once the actual bug in #167 is fixed, this could go in core with the remaining issues as follow ups. This would only be the case if the accessibility issues raised are not preventing links from being created.
5. We should also just ensure that when 'Content' is enabled, that new content types are enabled by default.
6. We are going to discuss the term "suggesters" with the Drupal terminology folks as we may want to change the label. If we want to change the label, that should be done before it's committed, but we also don't want to block anything on that.

Discussing the terminology now with @_deadarm_

I self-assigned on Feb 29, >6 months ago, in #149. 🙈

Thanks everyone for pushing this forward, based on #173 this seems tantalizingly close ❤️

Pushed a fix addressing #167 →

Thanks @el7cosmos, confirming that issue is fixed.

Also checked that new content types are enabled automatically as long as all content types are enabled. If not, then new ones are not. I think this works as expected.

So the two remaining tasks are:

1. Update the default config to provide two suggesters, one with just Content enabled and one with Content and files.
2. Confirm the terminology, which is proving to be a struggle :)

We discussed it at DrupalCon and a bit after, but have yet to find something that works.

Hmmm, if the behavior of whether or not a new content type is automatically enabled is dependent on whether or not all existing content types are enabled...

Then is there any feedback to the administrator that this logic decision has taken place? While this may be the expected behavior, it is still easy to overlook if it is not the desired behavior for a particular content type. I recognize that the change in configuration may have occurred in code rather than through interaction with the administrative user interface. Just trying to avoid surprises.

An established pattern in core and contrib is that all checkboxes unchecked means "all". But what happens in that scenario if the only checked target type is deleted? Now nothing is checked. Is everything then silently enabled?

All checked meaning any new content types also become checked is very weird, don't think I've seen that behaviour anywhere else.

I agree an explicit "all" option would be better than either.

I've noticed one other detail applying the latest changes that went unnoticed on my first check. if you are on a text format edit form and you are setting up the linking and you have no link suggester created yet you only see the create new link suggester link on the entity link suggestions vertical tab. there are two problems

1) if you are clicking the create new link suggester link the add link suggester dialog is opened in a dialog modal. the create button as well as the button label are both close to invisible at the bottom of the modal. i had to actively search for it. that isnt meeting SC1.4.3 and SC 1.4.11. On hover the styles are correctly applied and the buttons turns dark blue.

2) If i create a link suggester via the dialog modal, when the link suggester was successfully created the entity links checkbox is unchecked. i have to manually recheck the checkbox and then select the newly created link suggestor and save. from my perspective that step should be unnecessary. the entity links checkbox should remain checked and the newly created link suggester should be auto selected. that was the reason i created the link within the text format page in the first place - to use it in the context of this text format as the suggester.

i think point 1 needs to be addressed within this issue, point 2 could be moved to a followup issue.

I disagree with #178 and #179 that enabling new content types by default is unexpected. I guess I don't have a big issue with adding an 'All' checkbox but I don't think it's needed.

Re: point 1 in #180 isn't this an edge case since it will ship with two suggesters? So this would only happen if someone deleted both of them (which you can only do directly in the link suggester config) and then tried to add one via the modal?

hm problem is, it is not only the case when a suggester is created on a text format. just explored it a bit more, it is also the case when you edit a suggester on the text format. there, instead of the save button, the problem applies to the update and delete button. and on a side note "update" as the button label is kind of unusual, usually "save" is used in that context?

I know that I've reviewed this in the past but I'm having some second thoughts about the UX now that I'm looking at this with fresh eyes, I'm concerned that we may be over complicating the user experience for the site builder. I'm trying to think of the use cases for this. I was able to identify use cases that map under two categories:

1. Allow linking to any content that is linkable
2. Restrict suggestions to a subset of linkable content to enhance content authoring experience

It seems that we are optimizing for the 2nd use case, whilst making 1st harder. I'm not sure if that's the right trade-off because this is not an actual restriction for the content authors, because they can still link to any content, they just don't get suggestions to all content.

The UX pattern that we require configuration outside of the Field UI is not great. This is not unique to this patch, for example, Pathauto suffers from this too. I'd imagine the ideal UX for this to be that you could configure this in the context of the content type, e.g., checkbox with label "Allow linking to this content type".

I think we should prioritize the use case 1. first, and do that really well. Once we've nailed that, we could have contrib or core implement a solution for the 2nd use case.

@rkoller the button issue you're seeing looks like a CSS bug with modals unrelated to the MR. I can't think of other UIs in core that use a similar modal but if we can find one it would be good to confirm whether it's a general problem or not.

@lauriii The UI does look very complex to me. I have one concern about just allowing all entities though in that I can't see anyone ever wanting to link to a comment from ckeditor, (except in the very specific case of Drupal.org which I realised the irony of as I was typing...), and the same with custom menu links (i.e. you wouldn't link to the link, you'd link to the page that the link points to maybe).

I'd imagine the ideal UX for this to be that you could configure this in the context of the content type, e.g., checkbox with label "Allow linking to this content type".

This gave me an idea - what if define this at the bundle level in hook_entity_bundle_info(), defaulting to off.

Then we can try to set sensible defaults in core (i.e. on for all node bundles and media bundles, off for comments and menu links). Sites can use hook_entity_bundle_info_alter() to customise it, and either core or a contrib module could do that via config with a UI later.

But also as long as we're happy to restrict the default available suggestions after the initial lands (i.e. a behaviour change for content editors), even that API level support could happen in a follow-up if it helps this get in.

I was thinking that we could provide the setting on different entity types but we should definitely do some curating where content types could default to allowed and others default to not allowed. This would enable use cases such as linking to media, which is a valid use case.

This gave me an idea - what if define this at the bundle level in hook_entity_bundle_info(), defaulting to off.

Then we can try to set sensible defaults in core (i.e. on for all node bundles and media bundles, off for comments and menu links). Sites can use hook_entity_bundle_info_alter() to customise it, and either core or a contrib module could do that via config with a UI later.

I like this suggestion. I'm wondering if we should also provide an API that makes it possible for contrib to provide multiple link suggesters for the use cases where you specifically want to restrict the list for a specific use case.

But also as long as we're happy to restrict the default available suggestions after the initial lands (i.e. a behaviour change for content editors), even that API level support could happen in a follow-up if it helps this get in.

IMO this would be fine for this functionality.

@catch for example if you are editing any of the view modes on admin/structure/display-modes/view. i just realized that the direct link i've provided for the full content view mode in #182 ✨ Drastically improve the linking experience in CKEditor 5 Needs work is displayed as a page instead of in a dialog modal, you have to click the edit button to open it in the dialog modal. but those view modes use a similar modal.

I can't see anyone ever wanting to link to a comment from ckeditor, (except in the very specific case of Drupal.org which I realised the irony of as I was typing...)

FWIW, even the Drupal.org use case does not really make sense to me, because for one thing comments here don't have a title. I suppose you could search on the comment body but how on earth would you find what you are looking for? So I really do struggle to place the comment use case (same for custom menu links as catch also noted).

I tried to update the IS based on #184 but re-reading it I am not totally sure what is being suggested as the initial approach.

But also as long as we're happy to restrict the default available suggestions after the initial lands

Is this to say it would initially suggest everything linkable? I find myself so strongly opposed to this but at the same time, if you don't have shortcuts, comment or contact installed, you won't see these anyway so maybe it doesn't matter much!

I find myself so strongly opposed to this

I also don't particularly like the idea of this and think we could do a per-bundle opt-in (in PHP, not config) pretty quickly/easily. But also if we can get an initial unfiltered issue and an opt-in issue done in the same minor release window, no-one would ever see the unfiltered version.

I can try to open up the two follow-ups that are implied by the recent discussion a bit later.

I thought about this more and re-reviewed the MR.

Because this is using entity reference selection handlers to get the suggestions, it needs some concept of allowed entity types, otherwise it would have to query every entity type one by one.

However if we use bundle info, it's just a few lines of code to rely on that.

I've put up an additional draft branch/MR here: https://git.drupalcode.org/project/drupal/-/merge_requests/9807

This does the following:

1. Hacks out most of the supporting code for the link suggester config entity. There are still more places to remove.

2. Does an untested implementation of using bundle info instead.

I didn't touch any of the test coverage or do anything else except rebase. However, this ought to be enough to show the general direction for someone who wants to take this further.

The next tasks would be:

1. Manually test the MR and debug any obvious bugs introduced, so that the ckeditor plugin actually works.

2. Clean up more remnants of the config entity (including references from the ckeditor5 config).

3. Adjust the tests.

I also noticed some additional issues in the MR, like CSS is still using .linkit classnames and similar, that also would need to be sorted out before commit.

I'm having a look at this!

Hii,
Wooh quite a pretty long comment thread, probably the longest I've participated yet! I think I was living under the rock till now and just couple of days ago discovered this from Drupal Slack. I'm quite excited and thrilled about this new feature request, it'll be a complete game changer and much awaited one!
After going through all the comments from the beginning, I must acknowledge that I got an enriching learning experience, got to know about many intrecacies that I wasn't even aware of till now!
Though the issue seems to be requiring of supreme experience, but I couldn't resist myself to participate here.
As of now, just noticed some tiny syntactical errors for which after applying the patch, the site and drush was getting crashed, so did some quick tiny fixes. I'll keep on having a look at it from tomorrow again to investigate and test further!

Thanks and Regards :)