Automatically disable links to unpublished/non-existing internal entities

Confirming the added functionality. With Linkit enabled and the new "Remove links to unpublished/non-existing entities" checkbox checked in the text format, a user is able to add a link to an unpublished node, and the link shows up as a link in the WYSIWYG to an admin, but the link is rendered as a span to users that don't have permissions to view unpublished content. Likewise, the link for a node that is linked to and subsequently deleted becomes a span after deletion.

Realized that doing an access check will lock out users that might be able to view an entity after login.
Now checking the publish status instead.

Hi hello,

I reviewed !MR133 on a D10.0.3 site and got the same result as #6 with links in text editor field behaving as expected.

Tested this with a link field and it was not working for me. All links were showing as normal. Dug into the code, found this could be fixed by negating the call to isEntityLoadable() on line 124 of LinkitFormatter.php. So it goes like this..

if ($profile && !$this->isEntityLoadable($link_item)) {

hope that helps..

@schillerm, I can't reproduce your issue. It is working for link fields as well as links in text fields in my setup.
The method isEntityLoadable() simply checks whether the appropriate data attributes (type and UUID) are present, so the call in line 124 should not be negated.
I renamed it to hasEntityAttributes() to better reflect this behavior and avoid misunderstandings.

Hi again,

ok I retested this again (D10.5.2 site), with same result. I'm using dogit (https://github.com/dpi/dogit) to load up the linkit module and MR. My local site is set to "Do not cache markup" on the page /admin/config/development/settings.

I tested three types of node links, published node, missing (deleted) and unpublished.

Each of the related LinkItemInterface items I tested had nothing in $item->values["options"], when passed into the function hasEntityAttributes(). I don't know why I am not seeing anything here and you are mrshowerman. Am I doing something wrong somewhere?

All LinkItemInterface items had a uri, so I wrote a little function based around that as a starting point to test if entities load..

  /**
   * Determines whether the given link item loads an valid entity.
   *
   * @param \Drupal\link\LinkItemInterface $item
   *   The link item.
   *
   * @return bool
   *   TRUE if the item loads an entity, FALSE otherwise.
   */
  protected function canEntityLoad(LinkItemInterface $item): bool {

    // Get the uri.
    $uri = $item->get('uri')->getString();

    // Check if we have a node uri.
    if (strpos($uri, 'entity:node/') === 0) {
      // Extract the node ID.
      $nid = (int) substr($uri, strlen('entity:node/'));

      // If no node ID (Missing) then return FALSE.
      if ($nid === NULL) {
        return FALSE;
      }

      /** @var \Drupal\node\NodeInterface $node */
      $node = \Drupal::entityTypeManager()->getStorage('node')->load($nid);

      if (!$node || !$node->isPublished()) {
        // Missing or unpublished.
        return FALSE;
      }
      // Node exists and is published.
      return TRUE;
    }
    // No node id found - not a node.
    return FALSE;
  }

This worked for me.. feel free to use if you want.

Link items should have those options, provided that Linkit's widget has been used in the node entity form.
Maybe this was not the case in your scenario?

Your suggested helper method is pretty similar to the existing helper method LinkitHelper::getEntityFromUri($uri), but I have to say that the whole concept of this feature is about the existence of the UUID data attribute, which provides proof that a node had been found by the LinkitWidget at the time the node had been edited.

FYI, as a maintainer of Linkit, this feature makes good sense, and the implementation is sensible. I approve of this being an opt-in feature. Assigning to myself for closer review of the code...

Responding to the questions for the maintainer:

Is span the preferred neutral markup for the fallback, or should an alternative be considered?

This seems to be semantically as good an option as any other. The title attribute can be used on any HTML tag, so that's not a problem, and span does not denote any other meaning, so it's effectively neutral. I also considered whether this should remain an a tag without an href attribute, but that seems like it would be confusing to site visitors, and https://css-tricks.com/how-to-disable-links/ supports that thinking

Should the title text be made configurable or is translation sufficient?

It seems sufficient to start with it not being configurable and see where community demand goes, rather than building in the configurable text from the beginning.

I've added a documentation page at https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... → , since the configuration between the link fields and the text format may not be intuitive.

All LinkItemInterface items had a uri, so I wrote a little function based around that as a starting point to test if entities load

I tend to agree with the reasoning in #11, that this should be evaluated based on UUID information, rather than trying to determine this from the URL (and besides, #10's proposal doesn't handle entities types other than nodes):

this feature is about the existence of the UUID data attribute, which provides proof that a node had been found by the LinkitWidget at the time the node had been edited

My code review of this checks out, and I also did functional tests. Everything works as stated/designed, with one exception. In comment #6, there was the implication that removing unavailable links will only affect users that do not have the "view own unpublished" permission:

and the link shows up as a link in the WYSIWYG to an admin, but the link is rendered as a span to users that don't have permissions to view unpublished content

In my review of the code, I didn't see this permission check anywhere, and a functional test shows that even for user 1 (bypassing all permissions), the link is rendered as plaintext. I'm open to considering that the current implementation is how this should be designed -- i.e., that for all users, the link should be rendered as a span. I think the consistent behavior here would reduce the likelihood of confusion where a user with elevated privileges sees the link and doesn't understand why an anonymous user doesn't see the link. But I'd like to hear from the community.

Thanks a lot for the review, @mark_fullmer.

The first attempt did perform an access check, so super users did see the link, as reported in #6.
However, I changed this behavior for the reasons mentioned in #15 and for another reason: there might be setups where anonymous users should be able to click on a link to a restricted page that they can view after logging in. That's why I replaced the "view access" check with a check on the status flag.

I shared this feature request with some colleagues, and they had some observations that I think are worth considering as we finalize the implementation:

1. Part of the stated motivation for this issue is "this leads [...] valid errors in QA/SEO tools (e.g., Ryte) when internal links return 404." If we are describing these as _valid_ errors, then why would we be effectively hiding these errors from QA/SEO tools? Put another way, if a site transforms broken links into plaintext, doesn't that take away a tool for someone using a QA/SEO tool to identify broken links on their site?
2. What if, during the processing that converts these links to span tags, we added a Drupal log message that flagged the occurrence? Would this be a way to surface these problematic links to assiduous content creators, thus mitigating the fact that these "valid errors" are no longer showing up in QA/SEO tools?
3. While we agree that a broken link provides a poor user experience to a site visitor, one of my colleagues wondered whether a plaintext rendering with a title attribute that displays "The referenced content is not available" would likely also be confusing, since most site visitors wouldn't be able to have any clear indicate of where, in a paragraph of text, the message is referring to: hovering over some parts of the text show the title attribute, and others don't. Given this, would it make sense to provide a default visual styling of the "linkit-unavailable" class? What about using a text-decoration-style: wavy;? Conventionally, this CSS style is associated with potentially problematic text, most commonly potentially mispelled words; semantically, this is pretty close to what we're trying to indicate, namely, "there's something wrong with this link."

Thanks for the feedback, @mark_fullmer.

1. That's why the new feature is opt-in. Of course, using the the aforementioned tools to spot and fix broken links is the desired behavior.
   But in cases where this can't be done (due to missing resources or whatever reason), this feature can improve UX for end users.
2. I like this idea! Adding a log entry for broken internal links can definitely improve the situation (and partly replace external QA tools).
3. Funny, that's exactly how we are styling such links in sites that make use of this feature:

   We're also adding a few other styles:

   .linkit-unavailable {
     color: var(--#{$prefix}link-color);
     text-decoration: underline wavy;
     cursor: help;
   }
   

   (The link color is Bootstrap specific, but I'm sure you get the idea)