Child of #2417827: Evaluate and document each use of base: in core. See that issue's summary

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the user-path: scheme should be used for user input, not base:.
Issue priority This is a child issue of #2417827: Evaluate and document each use of base: in core, which is major, but is in itself normal.
Prioritized changes This is a followup for critical work related to #2407505: [meta] Finalize the menu links (and other user-entered paths) system, and therefore a prioritized change. The main goal of the issue is to provide consistent UX and DX for handling paths and links with the new API.
Disruption No direct disruption. There are outstanding issues to further update Views, e.g. #2423913: Leading slash in link fields and views has different UX, but this issue does not add to the scope of that work, just helps define it.
CommentFileSizeAuthor
views-base-to-user-path-for-user-input.patch1.29 KBeffulgentsia

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +VDC

Thanks!

dawehner’s picture

dawehner
Primary language German
commented

We should be aware that we now execute a full routing for every row of a view.

xjm’s picture

xjm
she/her
Primary language English
commented

@dawehner, from the change in this patch? they only are changing the path plugin (which should only happen once I thought) and the link plugin (which seems like it should do that negotiation and eventually have the link widget, ideally).

If it's adding significant overhead we should mark it NR rather than leaving it RTBC (and figure out how to address the problem).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs review

I think @dawehner is saying that using user-path: and actually resolving that URI into a URL means we're doing the whole "if it's a route, then map to a route, otherwise, map to a base: URI" dance.

This only applies to the second hunk, right?

dawehner’s picture

dawehner
Primary language German
commented

I'm just expressing that this change potentially introduces like 100nds routing calls on a page.

effulgentsia’s picture

effulgentsia commented

I'm just expressing that this change potentially introduces like 100nds routing calls on a page.

I don't think this patch does. This patch only changes the Drupal\views\Plugin\views\field\Links class, which is abstract and whose only subclass in core is \Drupal\views\Plugin\views\field\Dropbutton. I think the code that has the more significant performance issue is FieldPluginBase::renderAsLink(), which already in HEAD uses the user-path scheme, so this patch just makes the Dropbutton handler consistent with that. So, I don't know that we want to do anything different in this issue, but:

  • Should we have a followup to add a public FieldPluginBase::getUrl() method (or similar) that both FieldPluginBase::renderAsLink() and Links::getLinks() can use, instead of having them go through duplicate logic that we'll forget to keep synchronized in future patches?
  • Do we need a followup to investigate performance concerns of HEAD's FieldPluginBase::renderAsLink()? I don't even think #1965074: Add cache wrapper to the UrlGenerator addresses it, since that only caches UrlGenerator, not PathValidator::getUrl().
dawehner’s picture

dawehner
Primary language German
commented

Fair.

dawehner’s picture

dawehner
Primary language German
commented

Sorry for that short answer, it is late here.

In case we understand that things are not fast enough, maybe #2370651: Make (routing by path) cacheable will help.

effulgentsia’s picture

effulgentsia commented

And here's a fun one, not made better or worse by this patch: #2426399: FieldPluginBase::renderAsLink() loses language prefix for tokenized paths

xjm queued views-base-to-user-path-for-user-input.patch for re-testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

So I think #7 - #10 adequately address @dawehner's concerns here; while we need to consider performance for sure, this issue does not regress it, and there are other issues to address it. So setting back to RTBC. Thanks @dawehner and @effulgentsia!

xjm’s picture

xjm
she/her
Primary language English
commented
Category: Task » Bug report
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Yep, this is pretty clearly user input. Looks like performance concerns were addressed.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed fd73d48 on 8.0.x 
    Issue #2423929 by effulgentsia, xjm: Convert views.module usage of base...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.