Updated: Comment #N
Problem/Motivation
We need to get with the times and have our operation links use routes directly instead of path (href). This is definitely needed to be able to integrate CSRF tokens into the routing system.
Proposed resolution
Convert operation link items to use 'route_name' and 'route_parameters' instead of 'href' key. theme_links is used for dropbuttons so this is where it needs to be fixed.
Remaining tasks
Convert remaining operation links (entity list controller in another issue as this is a different complexity?), Modify theme_links to accept route_name and route_parameters etc.. See drupal_pre_render_link as this is currently doing what we need.
User interface changes
None
API changes
Links and operations can use route names directly.
Related Issues
#1798296: Integrate CSRF link token directly into routing system
Comment | File | Size | Author |
---|---|---|---|
#85 | 2102777-85-docs-followup.patch | 1.21 KB | tstoeckler |
#75 | interdiff-2102777-75.txt | 2.35 KB | damiankloip |
#75 | 2102777-75.patch | 14.07 KB | damiankloip |
#74 | 2102777-74.patch | 17.32 KB | damiankloip |
#68 | 2102777-68.patch | 13.55 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedHere is a start, this is what we need to do when converting the links, this will not work currently due to changes needed in the summary, plus this is only a handful ofthe conversions.
Comment #2
damiankloip CreditAttribution: damiankloip commentedSo we could modify theme_links something like this? This is a quick/rough attempt...
Comment #3
dawehnerIt is great to see that there aren't any problems with converting things to route_name/route_parameters anymore.
When we added links to theme_local_task and co we realized that 'href' is often set for some reasons, so maybe it is safer to check for route_name first. One question: can't we put all parameters to the link element and then let drupal_pre_render_link decide what to do? (this already has the needed logic built in).
Comment #4
damiankloip CreditAttribution: damiankloip commentedThat's a good point, moved the check for route_name first.
Regarding passing all parameters: We could set defaults, then just pass everything along so pre_process_link will take care of it, including the active class on the link (which is doesn't do atm). This would make it more consistent, as the active class for a route created link is on the link, and for a href created link is on the wrapper around the link.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commentedThis makes no sense to me. l() does almost identical (more efficient) active class checking internally already, and there's work being done to make it more efficient at #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.
AFAICS, theme_links() has no reason to try and handle active classes on behalf of the individual links it is rendering, so that code should just be removed.
Comment #7
damiankloip CreditAttribution: damiankloip commentedThat's what I like to hear, we can just remove it!
Let's see how this get's on with the tests now. Should actually fix most of them I hope.
Comment #9
damiankloip CreditAttribution: damiankloip commentedLet's see how this gets on.
Comment #11
damiankloip CreditAttribution: damiankloip commentedOk, I spoke to Tim on IRC, and we needed to revert some of the stuff added in #2091691: Convert test non-form page callbacks to routes and controllers as using {entity} as the slug in the path causes issues (slap on the wrist content_translation ;)). This makes sure that the dynamic links created by content_translation us {$entity_type} as the slug, we then get '{comment}', {node} etc.. and peace gets restored.
This should* fix the tests now....
Comment #12
damiankloip CreditAttribution: damiankloip commentedComment #13
tim.plunkettI don't think these changes can be made here. This has a rather long in-depth discussion of its own in #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.
If its absolutely required to make this work, we're blocked on that :(
Comment #14
damiankloip CreditAttribution: damiankloip commentedIs that really the case? This just removes the active class from the actual wrapper around the link and not the actual link. That remains untouched. I have been told we can quite happily remove the active on the li element as we're kind of duplicating stuff we don't need...
Comment #15
tim.plunkettAh yeah I missed that it was on the LI not the A.
But.
This will be a problem
Comment #16
dawehnerAdded a new function on the link generator to support extracting the information about the activeness of the route_name/route parameters.
Additional I reverted some of the test changes.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedaaaaah, it's on the li. I knew I must have been missing something obvious. Well @tim.plunkett, I don't remember the linked issue touching on theme_links at all, it's all focussing on l() over there.
Additionally, the active class logic on theme_links is a little different to the logic on l().
amazing.
Comment #19
damiankloip CreditAttribution: damiankloip commentedHang on, don't we still want to go with removing the active on the li elements?
Comment #20
tim.plunkettWe should *not* remove them.
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedNobody wants to remove active classes from where they are currently.
Comment #22
damiankloip CreditAttribution: damiankloip commentedOK, I literally don't care either way :-) that's just what we discussed before.
Comment #23
damiankloip CreditAttribution: damiankloip commentedThis should fix that.
Comment #24
dawehnerOh I am sorry with all the bugs at my changes in the middle of the night!
Comment #25
webchickThis should get some eyeballs by the Twig folks.
Comment #26
dawehnerDon't you consider thedavidmeister as twig folks?
Comment #27
damiankloip CreditAttribution: damiankloip commentedWe did talk to some of the twig guys (thedavidmeister and Fabianx) about this issue before. I will get them to confirm.
Comment #28
Fabianx CreditAttribution: Fabianx commentedPlease remove that change and inline in the helper function again.
It is 5-15% slower (IIRC) to not inline and links are in the critical path. Dawehner pointed out that we could inline the attribute handling also and I agree, but that is a nice follow-up issue.
The helper function is nice for theme_links :).
But generate() should inline the code as it did before ...
Overall I like the cleanup, but please leave the inline function in.
This could need some profiling / is a good opportunity to benchmark it if any of the converted lists are user facing (instead of just admin facing like book admin list ...).
Thanks!
Comment #29
damiankloip CreditAttribution: damiankloip commentedThanks Fabian. Here is an updated patch with that change reverted.
Comment #30
dawehnerThis addressed the points mentioned in #29.
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commentedWhat Fabianx says in #28 is true, that previously we've considered pulling "is active" functionality into separate functions and it always comes back from profiling as a measurable slowdown :( Can we see what the impact of isRouteActive() is here?
Comment #32
damiankloip CreditAttribution: damiankloip commentedRerolled.
Also I think it is more performant to just call isRouteActive() in this case as the linkGenerator has a static cache of the active route info (see linkGenerator::setRequest). If we didn't use this we would have to \Drupal::request() or cache the active info about the request ourselves in theme_links.
Thoughts?
Comment #34
damiankloip CreditAttribution: damiankloip commentedWhoops.
Comment #35
dawehnerSo what is the reason to drop big parts of the patch? Was that stuff which should not have been in here?
Comment #36
damiankloip CreditAttribution: damiankloip commentedWell, basically the same changes were added by commit db6985b.
Comment #37
dawehnerI fear that people will complain about a missing profiling as this issue had a needs profiling tag.
As I know damian I would be fine without one :p
Comment #38
damiankloip CreditAttribution: damiankloip commentedI profiled yesterday quickly, and it didn't look great.. Unfortunately I'm now away for the weekend though so can't post anything.
We need this either way as we should be using route names.
Comment #39
thedavidmeister CreditAttribution: thedavidmeister commentedWe just need some public profiling results so an informed decision can be made by all of us together :)
Comment #40
damiankloip CreditAttribution: damiankloip commentedHmm, this is what I found before:
It causes quite some overhead. See attached archive too.
Comment #41
thedavidmeister CreditAttribution: thedavidmeister commentedThose numbers are so bad they're hard to believe :( I think we should leave the "Needs profiling" tag on this issue until we can corroborate that data.
Is there something we're doing here to cause the overhead? what can we do to speed this up beyond inlining the "is active" logic?
This overhead is coming from the route system itself isn't it? :/
Comment #42
damiankloip CreditAttribution: damiankloip commentedYea I think so :/ so were basically tied, because we need to use route_name for stuff like this...
Apart from the is active stuff, the rest is just passed to theme_link().
Comment #43
thedavidmeister CreditAttribution: thedavidmeister commentedThere's no such thing as theme_link(), if you use #type 'link' it goes almost directly to l() or the linkGenerator().
I actually thought we were still pushing the perf tickets in before trying to convert everything to use the generator/routes (only converting links that run access checks for now) - https://drupal.org/node/2047619#comment-7801557
Evidently that's no longer the case? I only say this because none of the tickets mentioned in the linkGenerator issue have been resolved:
- #1965074: Add cache wrapper to the UrlGenerator
- #2058845: Pre-load non-admin routes
- #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links
Comment #44
damiankloip CreditAttribution: damiankloip commentedSorry, that's what I meant, fingers doing the thinking there...
I don't know what the grand plan is for performance issues related to routing I'm afraid, but what do we do wait until the 11th hour when we are happier with the performance, then add all this new stuff to the API's?! That doesn't really work either IMO. We certainly can't ship with the current form, which is a muddle of route_name and href across the board.
I think the biggest problem here atm, is that we are loading the linkGenerator for every call to get the active class. I'm not sure moving inline will help too much as there is alot of stuff the link generator needs to get this info.
Comment #45
damiankloip CreditAttribution: damiankloip commentedSo just to give you an idea of what it looks like without the calls to isRouteActive() (I know this adds no active class to the li element so not totally fair, but still shows how much overhead this part adds) and the original current href implementation:
Comment #46
thedavidmeister CreditAttribution: thedavidmeister commented@damiankloip, I can turn what you're saying around just as easily, are we supposed to worry about performance at the 11th hour?
Anyway, if the active class handling in PHP is adding a 1880% overhead, I think these results should be referenced at #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links - there's still a window of opportunity to get JavaScript based active class handling happening over there which could help avoid the overhead here.
Comment #47
thedavidmeister CreditAttribution: thedavidmeister commentedOh, also, what is the scenario being benchmarked in #40 and #45? How many links are we talking and in what context?
Comment #48
damiankloip CreditAttribution: damiankloip commentedWell, you could argue the same but api level things affect developer's code directly whereas performance improvements do not. I.e.these changes can be made and people will likely not have to change their module code, whereas if we wait another few months before telling people all of their links have to use route names instead that probably won't fly so well.
The benchmarks I posted we rendering 2 links using theme_links() directly, 100 times. Repeated with href then route_name.
Comment #49
thedavidmeister CreditAttribution: thedavidmeister commented#48 - I know what you're saying. I'm sure you understand what I'm saying too. I guess time pressures are just frustrating everyone.
I don't think you can say that in general performance improvements do not effect APIs, because they can and do influence decisions about how things are implemented. In this case though, I can see that it makes sense to forge ahead with theme_links and try to clean up the "is active" overhead elsewhere.
Comment #50
damiankloip CreditAttribution: damiankloip commented#49, Indeed, it is definitely a catch 22 situation... :/
Thanks, I think the active stuff is a general problem that we need to solve, and this will benefit from the outcomes of the other issues.
Do you think this patch should still include that isRouteActive stuff? or should we try to bring that back in a follow up even? We could then only focus on that, as this issue makes a few other changes too.
Comment #51
catchThose numbers are shocking but unsurprising - could you post some more detail from the profiling about what's taking most of the time, or is it spread out within route generation?
If this goes in we'll likely need to promote #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links and #2058845: Pre-load non-admin routes to critical.
Comment #52
damiankloip CreditAttribution: damiankloip commentedAgreed, on the promotions above.
Basically most of the time is the calls to isRouteActive() to add the active class on an li element in theme_links(). Without that (see comment and numbers in #45) we is more similar to the current lookup of href property.
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commentedI think we need to leave in the isRouteActive stuff or we'll have regressions in functionality.
I'd be ok with pushing this patch through and marking those two linked issues critical.
Comment #54
damiankloip CreditAttribution: damiankloip commented@thedavidmeister, So the patch in #40 is ready to go in your opinion?
Then we can bump the other issues.
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commentedI don't totally understand what this is doing:
I see there were problems in #11, but the code that was causing troubles at the time seems to have changed since then?
Could I just get confirmation that this is still needed?
Other than that, it seems fine to me atm.
Comment #56
damiankloip CreditAttribution: damiankloip commentedYeah, I think the big local task conversion, and another patch for content translation makes that un needed now. Removed those files from the diff.
Comment #57
damiankloip CreditAttribution: damiankloip commentedMeh
Comment #58
thedavidmeister CreditAttribution: thedavidmeister commentedCan we add a comment or something on the inline active logic saying something along the lines of "This logic is the same as isRouteActive, but inlined for performance"?
Comment #59
thedavidmeister CreditAttribution: thedavidmeister commentedShould we try replacing "isRouteActive" with "getActive" and putting that outside
foreach ($links as $key => $link) {
in theme_links() then inline the active class logic?Comment #60
damiankloip CreditAttribution: damiankloip commentedFor sure! I think this is the way to go, xhprof comparison looks much much better :)
Comment #61
dawehnerThese numbers are promising.
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commenteddoes getActive() not need to set the request if active is empty?
should we even introduce isRouteActive() at all if it's so slow?
Comment #63
damiankloip CreditAttribution: damiankloip commentedhuh? No. Where would it get the request from? This is set by the DIC and already called. See symfony docs on setter injection and anywhere else we use this.
It's not that it's so slow, it's just how it is being used more than anything. I think it's handy to have it there, but don't really care. Let's remove it for now then.
Comment #65
damiankloip CreditAttribution: damiankloip commented#63: 2102777-63.patch queued for re-testing.
Comment #66
damiankloip CreditAttribution: damiankloip commentedWhoooops. Interface changes needed.
Comment #68
damiankloip CreditAttribution: damiankloip commentedSorry, not having a good morning! Need to remove the isRouteActive assertions from linkGeneratorTest too...
Comment #69
dawehnerComment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedseems like
+ $language_url = language(Language::TYPE_URL);
could go outside the loop also, just below
+ $active = \Drupal::linkGenerator()->getActive();
Comment #71
Xano68: 2102777-68.patch queued for re-testing.
Comment #74
damiankloip CreditAttribution: damiankloip commentedRerolled. Moved the language back to the top of the theme_links function, thanks beejeebus.
Ugh, looks like #2084463: Convert contextual links to a plugin system similar to local tasks/actions changed theme_links to support route_name. This patch does it properly IMO, and tidies up the function and uses a link type to render instead of calling l() directly (as well as benchmarks it).
Comment #75
damiankloip CreditAttribution: damiankloip commentedSorry, rebased from the wrong branch. This is based on the last passing patch, unlike the previous one...
Interdiff attached to show changes to resolve merge conflict and moving language per comment in #74.
Comment #76
thedavidmeister CreditAttribution: thedavidmeister commentedI agree that what we're doing here looks better than the other issue.
Looking at it now though, I do wonder why we call drupal_render() inside a foreach, and then concatenate it onto strings instead of just building a single render array and returning
drupal_render($build);
right at the end...But, I think looking at that should be a followup.
Comment #77
damiankloip CreditAttribution: damiankloip commentedYes, agreed. I did think that a while back. Another issue is the place though, as how it wraps the li elements currently means it has to work like it does at the moment.
Comment #78
tim.plunkettThis was RTBC before, and the rerolls look good.
Comment #79
dawehner+1
Comment #80
Gábor HojtsyThis would make our code in #1952394: Add configuration translation user interface module in core way simpler by requiring less extra method on our interfaces to produce these paths that are now needed... It also cleans up yet another piece of API to work with routes rather than paths, the duplicity of which is really painful for #1952394: Add configuration translation user interface module in core at least, but I suspect for many others. So elevating to major and tagging up.
Comment #81
xjmComment #82
alexpottCommitted c4b089f and pushed to 8.x. Thanks!
Is there are follow up already? If not lets get one created.
Fixed the following in commit - replaced a deprecated function in a moved line of code and removed href from a link array where the route_name has been defined.
Comment #83
damiankloip CreditAttribution: damiankloip commentedYep, good catch, and thank you. That's the correct change :)
afaik, there is no issue for the views ajax thing. I'll create one.
Comment #84
damiankloip CreditAttribution: damiankloip commentedCreated that follow up: #2136641: Remove boolean usage of ajax property in Views UI link operation building
Comment #85
tstoecklerWanting to write a change notice for this I noticed that we didn't update the documentation of theme_links() here. Attached patch does so.
Comment #86
tstoecklerCreated change notice: https://drupal.org/node/2137053
I couldn't get it to link here, it may be that that specific autocomplete got broken by the D7 upgrade. It's pretty short/basic, so feel free to improve on that, but I didn't know what else to say.
Also updated 1 existing change notice: https://drupal.org/node/1989646/revisions/view/2681396/6708387
Comment #87
tstoecklerRemoving tag.
Comment #88
Gábor HojtsyThe docs followup patch looks good :) The change notice too.
Comment #89
Gábor HojtsyBTW I attached the change notice to this issue now. I think the problem was the + is in the title not escaped in the lookup or something along those lines... Now with no +, its trivial to find :)
Comment #90
effulgentsia CreditAttribution: effulgentsia commentedThe docs followup is nice, but nothing's blocked on it.
Comment #91
xjmIn the future, please create a separate issue for small followup patches, as @alexpott suggested above. Changing around the status of a single issue like this and having multiple commits against it confuses contributors, pollutes lists of tagged issues, and messes up metrics.
Comment #92
tstoecklerThis broke the docs gate so should never have been comitted. In that case it is more usual to re-open the issue instead of creating a follow-up.
Comment #93
xjmNo, @tstoeckler, when a core maintainer asks for it as a followup issue, create it as a followup issue.
Comment #94
Gábor HojtsyWho asked for it? AFAIS @tstoeckler found it independently above :)
Comment #95
jhodgdonI just committed that patch to 8.x, so don't bother with a follow-up. Thanks for being thorough!
Comment #96
jhodgdonComment #97
xjmDisregard previous comment about the change notice being missing; I just don't know how to use Drupal.org anymore. :)
Comment #98
xjmBut, restoring proper component and priority.