Add support for linking to entities in CKEditor 5

Haven't reviewed or tested yet, but big +1 to Linkit in core.

Great idea! Ideally this wouldn't only exist in context of CKEditor, but also when using link fields. LinkIt has an open issue for this functionality, having used it in multiple projects I must say it's a big improvement: #2712951: Linkit for Link field.

The difficulty is serving multiple use cases with a single field - entity reference fields can only point to entities, they can't point to a remote URL or a local URL that is not part of the entity type the field was designed for.

You can use something like https://www.drupal.org/project/link_fix_absolute_urls to convert Link fields to internal paths if the hostname is recognized, which can help a little.

Warning: I have a lot of thoughts on this. Like many others, we've used linkit for years extensively including the link field patch. I did not yet look at the patch, this is just feedback on the concept/idea. I generally like the idea of having this in core, but this is a complex case, there are many existing issues and use cases to consider.

* You mention that medias are already covered, but it's not quite so simple. A very common link use case is linking to documents to download, which is quite tricky. Linking that is exactly where the usefulness of substitution plugin comes in, because media entities should then link to the file. But then there are modules like http://drupal.org/project/media_entity_download which provide alternatives (our solution to the replace-file-results-in-broken-links problem). We even have a custom addition on top of that that allows to add a new media document through a link in a new tab and then that gets auto-selected. I don't know yet how to support that with the ckeditor5 integration, it's an important frequently used feature for us.
* there is a reason that drupal core has entity reference selection plugins, it is not enough to just do an entity query. to be fair, these plugins are older than entity queries, but there still edge cases to consider. published content for example, core does not properly implement entity access and exposing unpublished content to users who are not allowed to see it is a security issue. users have no entity label and weird logic around anonymous users, and so on. Instead of hardcoding the entity query, you must at least use those selection plugins.
* Plus, matchers aren't just about entities. there is for example an email matcher in linkit that ensures it's prefixed with mailto because editors don't know how to do that properly. maybe ckeditor5 has something for that built-in, I don't know? They often also don't know how to properly link to external content, that often results in broken links as well.
* I'm generally fine with core not having the same level of configuration as linkit module, but it is important that it is extensible IMHO and can be customized (As entity browser maintainer, I'm kinda frustrated about the media library and it's decision to to _not_ be extensible). For example, you claim that users are not often linked, but core has user profiles and on community sites and also blog/news sites where it absolutely can be a common use case to link to author profiles. If core thinks that's not useful as a default, fine, but it shouldn't require a contrib module to entirely replace what core does to change that.
* #2712951: Linkit for Link field is a complex topic. I fairly strongly disagree with many things you said in #14. First, linkit has been mostly unmaintained for many years, and anon later on agreed that this is useful and he is willing to commit it. The issue has 188 followers and 20 or so different people worked on it over the years. You will not find many contrib issues with the same level of interest and activity. Second, the core link widget has built in node autocomplete and reference support already, it's not like this is a crazy idea. And our URL API has built-in support to link to entities as well. That was a lot of hard work and we did it because we know that it's being used. You might only be thinking about configurable link fields here, but keep in mind that menu links are a regular link field and widget too. Mixing external links and content links and other things in menus is absolutely a common use case. Dynamic entity reference does not solve that use case. I do agree with you in one aspect. That it is indeed a rather different beast and as the ckeditor integration of this module gets integrated more tightly into ckeditor5, that will make it much harder to reuse it for link fields. But there are aspects of this that are generic and should not be limited to ckeditor5 module/integration. Specifically the logic and API, and then it could be separate client/js logic.
* Kind of a small thing, but what happens with longer titles, the window seems quite narrow? On sites with a lot of content, titles tend to be a lot longer and you need to see much more to differentiate them.
* The issue summary and title is a bit dramatic :) Yeah, we don't have good link functionality for ckeditor, but we have link and entity reference fields. It's one scenario of many.

See #2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element for example as an existing issue, and #2427803: [PP-1] Expose target type, selection handler and selection settings in LinkWidget. I know that it is tempting and easier to just create isolated things from scratch, but I think it's important to consider these other use cases. If we can reduce the linkit module to a backend/configuration/UI for this then it will be much easier to maintain in the future and I'd be willing to help as well.

Great to see this issue! I'd add my +1 in for including substitution plugins as well. It seems like a fairly straightforward system to implement that would make it far more extensible, as Berdir mentions for things like linking to media files, etc.

a little bit of feedback.

> is it "just" that you are saying this should not be using entity field queries directly, but EntityReferenceSelection plugins instead? I'm happy to refactor this MR to use that for sure! 😊

that's the main thing yeah, that we likely have to go through that to find matching entities. As for the security thing, it's complicated and has a very long history, but as of now, everyone querying and displaying nodes is responsible for respecting the status field themself as you don't rely on an active node grants system. That's one reason why there are fairly frequent information disclosure security issues in that space. There are also plenty of related limitations, like node/core entity having a view own unpublished permissions, so you need to mix it with a check on uid. See the special views filter that the default admin/content view has.

The sad truth is that "$query->accessCheck(TRUE);" does absolutely nothing with just core, only when you add contrib or custom modules using node grants or directly implementing those query alter hooks it starts to do anything at all.

> Am I right in concluding that the 99% use case for linking to media is for linking to downloadable media such as documents, archives, etc — not images/videos/embeds? The answer to that question could help inform a direction to tackle this too.

Yes, but that's where it gets complicated and why it's configurable in linkit. This depends on your media types, which are configuration, there isn't really a reliable way to automatically detect that. you could look for media types with a file field instead of image, but we have an svg media type with a file field for example. So not just entity types but also their bundles. And then you quickly approach the config UI that entity selection plugins offer. user does it on the roles for example, which for entity reference fields is a very common use case (e.g. only being able to reference user with the author role).

// Add decorator to links.
  assert($plugin_definitions['ckeditor5_link'] instanceof CKEditor5PluginDefinition);
  $link_plugin_definition = $plugin_definitions['ckeditor5_link']->toArray();
  $link_plugin_definition['ckeditor5']['config']['link']['decorators'][] = [
    'mode' => 'manual',
    'label' => t('Open in new window'),
    'attributes' => [
      'target' => '_blank',
    ],
  ];
  $plugin_definitions['ckeditor5_link'] = new CKEditor5PluginDefinition($link_plugin_definition);

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.

Linking to the media container should be an option, though, because I imagine some people may have a need to let people see the container itself.

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!

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

@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"?

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.

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 #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.

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

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.

#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/hover styled tooltip for different things using JS. I don’t think I’d ever do that personally, but that’s one use case for it. Probably modern methods would use data-*. That may be something that should be handled with contrib like Editor Advanced Link (what I have enabled in a screenshot up above, I think…)

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.

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 getVariantOptions(): ?array;
  public function getLinkTarget(string $variant = 'default);
}

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

FWIW, agree with #45

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.

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 a hook is added, then Webform can add its own functionality.

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).

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!

@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 which should make the blocks better.

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 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.

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

I’m hopeful this is possible/easier with the landing of https://www.drupal.org/project/drupal/issues/3073554!

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!

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.

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

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.

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

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.

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 the diff 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!

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?

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.

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

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.

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!

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".

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 (show field)
• Always set target _blank on external links and not set on internal links (don't show field)
• Don't allow setting target (don't show field)

Further, it would be good to use plain English labels on the field such as "Open in new tab or window" rather than HTML jargon such as "_blank".

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. Would it be better to use 'Default' which I think matches other instances of config that is provided?

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 and provides lots of information.

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_

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.

I'm thinking it would make sense if there was a checkbox at the top labeled "All" that was checked, but not if it's simply checking to see that all the pre-existing checkboxes were previously checked. At the very least, is this behavior documented somewhere?

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?

update: and it looks like the ui-widget class (in the theme.css from jquery ui line 19) is overriding the background color and setting it from the primary color blue / and grey for the regular button to transparent. and comparing the buttons in the footer of a dialog modal with admin/structure/display-modes/view/manage/node.full?destination=/admin/structure/display-modes/view, the sizing differs. but that is rather cosmetic. the color contrast is the more serious problem.

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 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. Probably mostly removing coverage for things that aren't supported any longer, keeping the actual plugin test just with whatever default entity types/bundles we support.

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 :)

@sourojeetpaul that's great, sometimes jumping in at the deep end is the best way to learn - both core APIs you might not be familiar with, and also things about core contribution you might not have come across yet.

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.

If you've made any fixes, even very small and incomplete ones, please make a commit and push them to the MR here - makes it easier to get early feedback and/or for people to follow along.

Hi @catch,
Thanks for your kind words and motivation. Its not that I haven't yet came across Core contributions, I've actually worked on a no of core issues and thankfully enough, got recognised for 3 of them as of now. I'm aware that Core issues are bit tricky and threads go on for a longer period of time. But this one is truly exceptional and interesting to work on, and lengthiest as of my experience. Sharing the similar thought I've started looking forward to Core issues as that provides a solid learning experience.

I've already pushed my changes on your branch before making the previous comment, but I was also wondering why that's not getting shown on the threads. Is it because you've kept the MR as draft?
Here's the commit link: https://git.drupalcode.org/project/drupal/-/merge_requests/9807/diffs?co...
Please have a look and pour in your valuable feedback :)

I'll dig up further from tomorrow, had to scratch my head a lot today on this. Was getting quite a few issues while applying the patch, but luckily resolved those, and now my system also gave up by running multiple instances xD

Both those changes look good, fixing my silly mistakes/typos when I made the draft.

Hi @catch,
Though after fixing the syntactical errors, the patch is now cleanly getting applied and entity_links filter is getting listed among the available filters to be enabled, but on trying to enable the filter its trowing an Ajax error on the console, which is due to the fact that we're calling the loadMultiple() method of EntityLinkSuggester, but the entity was never defined in the codebase. So I think the MR is still missing some files!
Please correct me if I'm wrong :)

That's why I resorted back to the previous solution that @wim and others have provided, to experience the functionality. Also played around and performed a bit of testing on that to ensure that the previous issues which were mentioned have been addressed well and jotted down few pointers, which IMO will be an enhancement on top of that, or incorporate those in this latest approach.

1. On linking to a downloadable media, and coming back to that again on editing, the toggle as well as the downloadable state remains unchanged. So the issue that @mark_fullmer had mentioned seems to be resolved!

2. I wasn't able to reproduce the 'Update' button styling issue on the Link Suggester modal that @rkoller had mentioned. the 'Update' and 'Delete' button styling over there looks good to me!

3. Though the strike-through over the entity names doesn't seem to be causing much of a big deal in terms of accessibility to visual impaired users, but I think there's a design inconsistency happening. On deselecting the entity type the strike-through happens while deselecting the bundle remains clean with just the unchecking of the checkbox, so IMO it'd be better if we can style both of them in a single pattern.

4. Tested the issue that @pameeela had mentioned that link suggester settings not getting saved properly, but now I see its getting saved properly and coming into effect as expected. Keeping only Node A checked by deselecting Node B, as expected its restricting to the contents of Node A only and not showing any content of Node B while linking from CkEditor.

5. Performed a quick accessibility testing using Wave tools for the missing 'title' attribute on the a tag, on which there's a discussion on this issue, but honestly that doesn't seem to be an issue to me and Wave is also not reporting any issues for the same!

6. Coming to the linking aspect of Comments, I'll also echo the same as of you and @pameeela. Couldn't find major use case for this one, rather its a super confusing one with the fact that @pameeela already pointed on the earlier threads.
I found one difference from the way we're implementing here vs the way D.O does the same, as D.O uses specific comment id for each of the comments and uses the same as anchor linking, thereby we reach to that specific position of the webpage directly, whereas in our case as we're referring to the comment entity which in turn actually referring to the node, where the comment have been added. And let's say on such a blog post or webpage if there're tons of comments like this issue thread :) then there's no point on landing the user at the very beginning of the same, where the user have to manually scroll down to that specific comment again. It'll be a pretty bad UX IMO.

7.

you wouldn't link to the link, you'd link to the page that the link points to maybe

I'd partially agree with you and @pameeela on the 'Custom Menu Link' aspect. Though it'll link to a page but I think giving the ability to search and link to the page using menu titles might be easier for content editors, specifically when instead of referring to a node or any other internal link the link is of an external one.

Now coming to the part, which I think can be a great enhancement on top of it:

1. Moderation State Indication: On incorporating workflows and content moderation, though we can restrict the view permissions based on roles but there are no way we can restrict linking to 'Unpublished/Draft' contents as of now. I don't think that makes any sense at all to link to an Unpublished content that the end users might not access and without any sort of indication for the same on the list the content editor might accidentally link to such a content. So I strongly feel that you should somehow indicate about the content's current moderation state, which IMO will be more helpful than the authoring date information.

2. Permission based suggester access: As of now we can create multiple link suggesters, but there's no way to restrict and assign the same based on user role. Let's say we have multiple content editors and we want to give them different linking privileges and thus having the option of creating multiple suggesters will make more sense!

3. Suggester Clubbing: Though we can define multiple link suggesters and use them in different text formats and editors, I was thinking about from a more user role perspective.What if we give the user an ability to club multiple suggesters. Circling back to my previous example:
On a group of multiple content editors, we defined different link suggesters for each of the roles with defined access privileges, and for the Content Head/Editorial Head instead of creating a new suggester from scratch, we can just club all the suggester groups that we've created for other editors.(basically a multi-select of the existing ones).

Attaching the screenshots of my testing for quick reference!
I’ll be all ears for your feedback soon! Thanks :)

error on the console, which is due to the fact that we're calling the loadMultiple() method of EntityLinkSuggester, but the entity was never defined in the codebase.

Yes so the idea from the last few comments here is to completely get rid of the link suggesters from this issue, and use the bundle info instead. I didn't try to get to a working MR, I just tried to point in the direction things should go. It sounds like you found the next step that would need to be done.

The eventual UX of using the plugin in ckeditor5 should stay the same, we're just trying to get rid of all the configuration infrastructure and interface for that.

Tried to Update the IS based on the latest approach, but there may be corrections needed.

I checked out the branch 3317769-link-suggestions-minimum, then I went to Basic HTML configuration and enabled "Entity Links" filter, then I clicked Save configuration. Then I got the fatal error

The website encountered an unexpected error. Try again later.

AssertionError: assert(NestedArray::keyExists($subform, $parts)) in assert() (line 857 of core/modules/ckeditor5/src/Plugin/Editor/CKEditor5.php).

Does anyone run into this issue?

Yes, the branch is broken, this issue is set to 'Needs work' as it has to be reimplemented based on the updated plan.

I'd like to work on this one if there's no one working on it at this point. I spent a couple of hours to dive into the changes @catch did on the new branch and got the ideas.

My first step is to make the update editor configuration work after enabling the entity links plugin. By default, node entity type is allowed to provide link suggestions when the plugin is enabled. In the plugin settings section, we should be able to configure which entity types and their bundles providing link suggestions.

Hi
Can anyone provide a patch for Drupal Core 10.3?
None of the above applies and I can not upgrade the project.