Drastically improve the linking experience in CKEditor 5

Created on 27 October 2022, over 1 year ago
Updated 29 February 2024, 2 days ago

Problem/Motivation

Drupal is a CMS. Content creation is its strength — or it should be, at least.

It took until Drupal 8 shipping with CKEditor 4 before content creation had good usability out of the box — you'd have had to install https://www.drupal.org/project/wysiwyg plus a lot of manual tweaking before your Drupal 7 site had a good authoring experience.

Drupal 8 and 9 shipped with CKEditor 4 enabled by default. Drupal 9.5 and 10.0 will ship with CKEditor 5 enabled by default. It improves on the authoring experience on many fronts.

One front remains though: even though content creation covers the "C" in CMS, the "M" in CMS is not quite covered yet: Drupal enables you to manage the content through Views, but it makes no effort at all to facilitate linking from one piece of content to another. Even though that's quite literally the essence of the world wide web: web pages (nodes) linked to other web pages (nodes), forming one enormous graph.

Fortunately, there is a Module For That too: https://www.drupal.org/project/linkit . It has matured for twelve years in contrib! 🤩

I think it's high time we bring the essential parts of Linkit into Drupal core: no configuration required (but possible! think: which entity types and bundles to provide suggestions for), just linking:

Proposed resolution

Add the essence of Linkit to Drupal core:

  1. while writing content in CKEditor 5, make it easy to link to other content — links and URL fields are completely different beasts because they have
  2. automatically suggest links based on the host entity type (when writing text on a Foo entity, find Foo entities), as well as entity types marked as common reference targets except User (unlikely to link to) and Media (which has its own optimized CKEditor 5 experience already), which means that by default Nodes and Terms will also be searched
  3. require the (newly added) entity_links filter to be enabled, which is conceptually exactly like \Drupal\editor\Plugin\Filter\EditorFileReference (which handles HTML elements with data-entity-type="file" data-entity-uuid="*" src), but this will instead handle HTML elements with data-entity-type="*" data-entity-uuid="*" href) — this ensures links are always pointing to the latest URL alias 👍
  4. 🚀 improve the responsiveness of the autocomplete by applying the same private client-side caching optimization used by \Drupal\ckeditor5\Controller\CKEditor5MediaController::mediaEntityMetadata() and \Drupal\media\Controller\MediaFilterController::preview().
  5. 🕵️ for every suggestion: automatically list the author if the entity type has one, automatically list the creation datetime if the entity has one, and list both if the entity type has both — this helps remove the need for some of the configuration surface area

🤔 What does https://www.drupal.org/project/linkit have that this merge request does not?


  1. ✅ @Berdir and others made the case that we need functionality like this. Solution: introduce link_target handlers for entity types. See #39 for details. CR: https://www.drupal.org/node/3350853 .

  2. ✅ @Berdir in #18 and #21 articulated why using entity queries is problematic, and why we should use Drupal core's existing EntityReferenceSelection plugins instead! 👍
  3. (optionally) setting the title attribute on links based on the linked entity's label — if you need this advanced feature, just use contrib Linkit 🤓 → furthermore, on this issue this was reported to be problematic for accessibility in #51, #55 and #56

🫣 Which contrib modules does this partially or completely replace?
(Usage stats on March 30, 2023.)

  1. https://www.drupal.org/project/linkit (~128,000)
  2. https://www.drupal.org/project/media_entity_download (~7,000)
  3. https://www.drupal.org/project/ckeditor_entity_link (~5,200)
  4. https://www.drupal.org/project/media_entity_file_redirect (~500)
  5. https://www.drupal.org/project/ckeditor_link (~37,200 — D7-only)
  6. https://www.drupal.org/project/ckeditor_link_file (~2,100 — D7-only)
  7. https://www.drupal.org/project/ckeditor_link_user (~200 — D7-only)

And if this functionality is adopted by a site, then arguably most of the functionality in the following modules becomes obsolete:

  1. https://www.drupal.org/project/pathologic (~46,500) — in combination with the pre-existing editor_file_reference filter in core
  2. https://www.drupal.org/project/insert (~38,000) — in combination with the pre-existing editor_file_reference filter in core

🙏 I want to use this but I'm currently using Linkit, how do I switch?
Glad you asked!

https://www.drupal.org/project/linkit used the same data-entity-type and data-entity-uuid that \Drupal\editor\Plugin\Filter\EditorFileReference introduced >8 years ago. It's specifically designed for portability. And it makes the transition from the contrib module to this core feature trivial:

  1. Disable the linkit filter
  2. Enable the entity_links filter
  3. Configure which suggestions you would like to appear:

That's it 😊

Remaining tasks

TBD

User interface changes

The CKEditor 5 linking experience becomes vastly better!

API changes

  1. Addition: new entity_links filter plugin
  2. Addition: new EntityLinkSuggester config entity type
  3. Addition: new ckeditor5_link_entity_suggestions CKEditor 5 plugin
  4. Addition: #type => radios's #options array no longer must be an array of strings, but an array of arrays is accepted too — this allows setting a #description per radio button for example

Data model changes

None.

Release notes snippet

TBD

Feature request
Status

Needs work

Version

11.0 🔥

Component
CKEditor 5 

Last updated about 3 hours ago

Created by

🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

Live updates comments and jobs are added and updated live.
  • Usability

    Makes Drupal easier to use. Preferred over UX, D7UX, etc.

  • JavaScript

    Affects the content, performance, or handling of Javascript.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates : some issue and comment data are missing.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇺🇸United States mark_fullmer Tucson

    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?

  • 🇳🇿New Zealand John Pitcairn

    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?
  • 🇦🇺Australia Andrew.Macpherson

    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

  • 🇺🇸United States wrd-oaitsd

    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.

  • 🇺🇸United States dalemoore

    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!

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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 🤣

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @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 :)
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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!

  • 🇺🇸United States mark_fullmer Tucson

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @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 😁🤞

  • 🇧🇪Belgium Jelle_S Antwerp, Belgium

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

  • 🇫🇮Finland lauriii Finland

    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.

  • 🇦🇺Australia mstrelan

    How about "internal links"?

  • Status changed to Needs review 11 months ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    🕵️ 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:

    "substitution" plugins:

    \Drupal\linkit\SubstitutionInterface

    (introduced in #2786049: Make entity URL substitutions pluggable to support a wider variety of use cases. and the accompanying data-substitution attribute — I don't think it's necessary in 99% of cases — if you need this advanced feature, just use contrib Linkit 🤓

    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

    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.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    Updating issue summary per #39.

  • 🇧🇪Belgium tijsdeboeck Antwerp 🇧🇪 🇪🇺 🌎

    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.

  • 🇺🇸United States dalemoore

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

  • 🇺🇸United States dalemoore

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

  • 🇩🇪Germany mrshowerman Munich

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

  • Issue was unassigned.
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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.
  • 🇺🇸United States smustgrave

    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!

  • 🇺🇸United States tedfordgif

    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
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇫🇮Finland lauriii Finland

    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.

  • 🇺🇸United States wrd-oaitsd

    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.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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.

  • 🇺🇸United States dalemoore

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

  • 🇹🇭Thailand Nick Hope

    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 .

  • 🇺🇸United States dww

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

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @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 😄

  • First commit to issue fork.
  • 🇺🇸United States mherchel Gainesville, FL, US

    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)

  • 🇺🇸United States mherchel Gainesville, FL, US

    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)

  • 🇫🇷France DrDam

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

  • Status changed to Needs work 11 months ago
  • 🇺🇸United States smustgrave

    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.

  • 🇩🇪Germany geek-merlin Freiburg, Germany

    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);
    }
    
  • 🇺🇸United States Chris Matthews

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

  • 🇺🇸United States Chris Matthews

    FWIW, agree with #45 Drastically improve Drupal's default linking experience in text fields 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.

  • Assigned to Wim Leers
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    581 pass, 1 fail
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    582 pass
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    572 pass, 1 fail
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    588 pass
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    #68.1–3 are implemented 👍

    Next up: filter settings as requested by @mherchel.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇺🇸United States pyrello

    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.

  • 🇺🇸United States mherchel Gainesville, FL, US

    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!

  • 🇺🇸United States mherchel Gainesville, FL, US
  • 🇺🇸United States Chris Matthews

    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.

  • 🇺🇸United States Chris Matthews

    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?

  • e0ipso Can Picafort

    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?

  • 🇺🇸United States pyrello

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

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    Custom Commands Failed
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    586 pass, 2 fail
  • 🇪🇸Spain ckrina Barcelona

    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.

  • 🇫🇷France Artusamak Bzh

    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!

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    588 pass
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    588 pass
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    588 pass
  • Issue was unassigned.
  • Status changed to Needs review 8 months ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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! 🥳)

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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?

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • 🇦🇺Australia acbramley

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

  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    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.

  • 🇺🇸United States mark_fullmer Tucson

    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.

  • 🇺🇸United States daddison

    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.

  • 🇺🇸United States smustgrave

    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.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @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

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 8 months ago
    589 pass
  • Status changed to Needs review 8 months ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • 🇺🇸United States smustgrave

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

  • 🇨🇭Switzerland Berdir Switzerland

    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.

  • 🇺🇸United States mark_fullmer Tucson

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

  • 🇺🇸United States mark_fullmer Tucson

    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 Drupal's default linking experience in text fields 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.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @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!

  • 🇺🇸United States pyrello

    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.

  • 🇺🇸United States smustgrave

    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?

  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

    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.

  • last update 6 months ago
    Build Successful
  • 🇺🇸United States texas_tater Austin, TX

    @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 Drupal's default linking experience in text fields 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!

  • First commit to issue fork.
  • last update 6 months ago
    Custom Commands Failed
  • First commit to issue fork.
  • last update 6 months ago
    Build Successful
  • last update 6 months ago
    Build Successful
  • 🇮🇳India omkar.podey

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

  • last update 6 months ago
    Build Successful
  • last update 6 months ago
    595 pass, 1 fail
  • last update 6 months ago
    596 pass
  • last update 6 months ago
    595 pass, 1 fail
  • 🇮🇳India omkar.podey

    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.

  • last update 6 months ago
    Build Successful
  • last update 6 months ago
    596 pass
  • Status changed to Needs review 6 months ago
  • 🇺🇸United States tim.plunkett Philadelphia
  • Assigned to Wim Leers
  • Status changed to Needs work 6 months ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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?

  • last update 6 months ago
    603 pass, 1 fail
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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.

  • 🇺🇸United States mark_fullmer Tucson

    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.

  • 🇺🇸United States mherchel Gainesville, FL, US

    I don't think it should be a hard blocker

    +💯

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

  • last update 6 months ago
    603 pass, 1 fail
  • last update 6 months ago
    603 pass, 1 fail
  • last update 6 months ago
    601 pass, 2 fail
  • Issue was unassigned.
  • Status changed to Needs review 6 months ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    #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? 😬

  • last update 6 months ago
    610 pass, 1 fail
  • last update 6 months ago
    610 pass, 1 fail
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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! 😊

  • last update 6 months ago
    610 pass, 1 fail
  • Assigned to Wim Leers
  • Status changed to Needs work 5 months ago
  • 🇨🇭Switzerland Berdir Switzerland

    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.

  • 🇮🇳India omkar.podey

    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.
  • Pipeline finished with Success
    3 months ago
    #57346
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇺🇸United States mherchel Gainesville, FL, US

    I think y'all will like it

    😍😍😍

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    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

  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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
  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    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.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • Pipeline finished with Failed
    3 months ago
    #58900
  • Status changed to Needs work 3 months ago
  • 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.

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    @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!

  • 🇺🇸United States mherchel Gainesville, FL, US

    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!

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    #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! 😄
  • 🇬🇧United Kingdom Alina Basarabeanu

    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?

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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 😊

  • 🇬🇧United Kingdom Alina Basarabeanu

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

  • 🇺🇸United States pyrello

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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! 🤩🤞🤞🤞

  • 🇬🇧United Kingdom Alina Basarabeanu

    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

  • 🇬🇧United Kingdom Alina Basarabeanu

    Many thanks, @ben_a

  • Assigned to bnjmnm
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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:

    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 🙏

  • Pipeline finished with Failed
    about 1 month ago
    #80954
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • Pipeline finished with Failed
    about 1 month ago
    #80958
  • Pipeline finished with Failed
    about 1 month ago
    #80961
  • Pipeline finished with Running
    about 1 month ago
    #80973
  • Pipeline finished with Running
    about 1 month ago
    #81227
  • Pipeline finished with Failed
    about 1 month ago
    #81351
  • Pipeline finished with Failed
    about 1 month ago
    #81396
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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! 🥳

  • Pipeline finished with Failed
    about 1 month ago
    #81419
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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 .

  • Pipeline finished with Failed
    about 1 month ago
    #81514
  • Pipeline finished with Failed
    about 1 month ago
    Total: 613s
    #84450
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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.

  • Pipeline finished with Failed
    about 1 month ago
    #85617
  • Pipeline finished with Failed
    about 1 month ago
    Total: 506s
    #85644
  • Pipeline finished with Failed
    about 1 month ago
    Total: 166s
    #85680
  • Pipeline finished with Failed
    about 1 month ago
    Total: 498s
    #85685
  • Pipeline finished with Failed
    about 1 month ago
    #86121
  • Pipeline finished with Failed
    about 1 month ago
    Total: 483s
    #86130
  • Pipeline finished with Failed
    about 1 month ago
    #86148
  • Pipeline finished with Failed
    about 1 month ago
    Total: 525s
    #86234
  • Pipeline finished with Failed
    about 1 month ago
    Total: 519s
    #86249
  • Pipeline finished with Failed
    about 1 month ago
    #86301
  • Pipeline finished with Failed
    about 1 month ago
    Total: 192s
    #86311
  • Pipeline finished with Failed
    30 days ago
    Total: 3811s
    #86312
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • Pipeline finished with Failed
    22 days ago
    Total: 180s
    #91197
  • 🇮🇩Indonesia el7cosmos 🇮🇩 GMT+7

    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.

  • 🇬🇧United Kingdom scott_euser

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

  • 🇮🇩Indonesia el7cosmos 🇮🇩 GMT+7

    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.

  • 🇬🇧United Kingdom scott_euser

    Ah I see, makes sense, thanks for clarifying!

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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.

  • Issue was unassigned.
  • 🇺🇸United States bnjmnm Ann Arbor, MI

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

  • Assigned to Wim Leers
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

Production build https://api.contrib.social 0.61.6-2-g546bc20