- Issue created by @danflanagan8
- Status changed to Needs review
over 1 year ago 3:40pm 15 June 2023 - 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 workThough 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!