Alias should not get created if system path is <nolink>

Created on 15 June 2023, over 1 year ago
Updated 28 November 2023, 12 months ago

Problem/Motivation

I'm using a bundle class to override the toUrl() method on a particular node type. If some condition is met (the specifics are not important) I don't want teasers and cards to render any links to the node because there's a corresponding check in the bundle class access() method to prevent the node from being accessible at its own page.

  public function toUrl($rel = 'canonical', array $options = []) {
    if ($rel === 'canonical') {
      if (//some condition) {
        return Url::fromRoute('<nolink>');
      }
    }
    return parent::toUrl($rel, $options);
  }

When I save such a node that meets the "" condition, pathauto is creating an alias for the / system path.

It doesn't cause any major problems from what I can tell. For example, this does not serve the node when I go to the front page of my site. It does however result in a few annoying things.

Proposed resolution

If the source url is <nolink> pathauto should avoid creating an alias.

Remaining tasks

User interface changes

API changes

Data model changes

πŸ› Bug report
Status

Needs review

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @danflanagan8
  • Status changed to Needs review over 1 year ago
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.3 & MySQL 5.7
    last update over 1 year ago
    48 pass
  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    Here's a patch. This should have tests. I'm also wondering if and should be handled like this too.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Not against adding a check, but I'm not sure if that really works consistently and is meant to be supported? Isn't that just going to result in links with an empty href in many places like content overviews? What about things like xml sitemaps and other places that generically link nodes? And I think it won't make the detail page inaccessible, you still need an access check or something like rabbithole for it?

    If you need logic for that in your templates anyway, I'd just add a hasLink() method or something and check for that and not call toUrl() then.

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    Hi @Berdir,

    Isn't that just going to result in links with an empty href in many places like content overviews?

    Rendering cards and teasers and views fields is actually the scenario where this is really nice. By having toUrl return <nolink>, I can let field formatters handle all the linking. If I return <none> rather than <nolink>, then yes there are empty links all over the place. But this at least is the really smooth thing about <nolink>.

    And I think it won't make the detail page inaccessible, you still need an access check or something like rabbithole for it?

    You're absolute right about that. I forgot that there's a access() method on the bundle class. If the condition is met, that denies access to non-editors when trying to view the node at the canonical route. I modeled that bit of code off of something in the micronode project. I'll update the IS.

    What about things like xml sitemaps and other places that generically link nodes?

    The access() method on the bundle class makes this all work with simple_sitemap, at least.

    If you need logic for that in your templates anyway...

    This behind the scenes stuff specifically prevents me from having to put logic in templates as I can rely on the field formatters to do the right thing. I'm able to use a single card template for all my node types even though most of them are "normal". There's a single check for {% if url %} when rendering the title, but that's it.

    but I'm not sure if that really works consistently and is meant to be supported?

    I'm not totally sure either. I've been messing around with this and finding problems here and there, but it mostly works and the fixes have all been fairly simple. I really like that the field formatters render things as linked and unlinked as I want. I wish field formatters treated an inaccessible route as , but they (mostly at least?) render links that 403. Here are a couple relevant issues:

    πŸ› Entity reference label formatter may render link to inaccessible entity Closed: duplicate
    πŸ› StringFormatter always displays links to entity even if the user in context does not have access Needs work

    Though I guess that likely mostly boils down to how the link_generator service handles things and there are probably reasons that it works the way it does.

  • πŸ‡ΊπŸ‡ΈUnited States bvoynick

    Also running in to some of these pathauto issues @danflanagan8 is reporting.

    We're using this type of logic for similar reasons: it's a very centralized way to "turn off" the canonical-having nature of specific entities, of types that need to optionally have it. Using a toUrl() override plus a #pre_render callback on the link element, most things that construct links will automatically not link to these non-existent detail pages.

    It's true that an access check service, or some other approach such as overriding the access() entity method, is also necessary in case someone does make their way to the canonical route anyway. Doesn't make it unworkable though, and 2-3 places are far easier to track down than every last place an item might be output: things like the Label entity reference formatter, which otherwise needs to be overridden, and all other such programmatic toUrl() calls. (For example, even in the default Content overview, nodes that implement these kinds of overrides show up without the title being linked. Can't remember if that's just from the toUrl() override or if that relies on the #pre_render, but otherwise It's pretty fantastic.)

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    Hi @bvoynick! It's very reassuring that someone else is trying to do this same kind of thing for all the same reasons.

    I just linked a related issue in the redirect module: πŸ› redirect_form_node_form_alter calls getInternalPath on potentially unrouted url Needs review

    I think with this patch, the redirect patch, and the core patch ( πŸ› deleteEntityPathAll calls getInternalPath on potentially unrouted url Needs work ) everything is working for us.

    If you find any additional bugs with the approach I'd be interested in hearing about them. I'm on Drupal Slack with the same handle. Cheers!

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US
Production build 0.71.5 2024