Updated: Comment #118
Problem/Motivation
HTML links generated by either l(), theme_links() or linkGenerator() all dynamically check whether they are currently "active" by comparing the current URL's structure against the parameters being used to construct the link tag. This "active" class is used very commonly by themers to provide visual cues for users navigating the site, and there is no "browser native" or other CSS equivalent - this functionality can't be achieved without modifying the DOM.
The problem is that we cannot cache these links with any better granularity than "per page". Ideally, we would want to be able to cache links globally as link generation functions can be called hundreds of times on each page load and most sites would get a decent cache "hit rate" as it's very common for the same link to be shown on multiple pages in a site.
This problem existed in Drupal 6 and 7, but there are two main reasons why it is becoming more important to resolve this issue in Drupal 8:
- Render caching is something that we're trying to support as much as possible in D8 core (it exists in D7 but is not widely supported "out of the box"). Any content containing a link "inherits" the per-page cache granularity restriction, even if that content would otherwise be globally cacheable. If the content wrapping the link is only cacheable per-user, we end up with a per-page+per-user limitation potentially due to a single link. #1830598: [meta] support render caching properly.
- The new routes system in Drupal 8 has been profiled a few times with respect to link generation, and while results are preliminary and hard to refine, the pattern seems to be that it is significantly slower than the soon-to-be-deprecated l() function has been historically. This puts pressure on core to avoid uncacheable links wherever possible if we're to stay competitive performance-wise with previous versions of Drupal. #2102777: Allow theme_links to use routes as well as href, #2047619: Add a link generator service for route-based links .
There is a very closely related issue with the "active-trail" class that is also applied to links, but not directly from within l(), linkGenerator() or theme_links(). This is also important to address but is out-of-scope here and the two classes should not be confused as they serve distinct purposes.
Proposed resolution
There are two proposed approaches to a resolution here. There was a third, CSS based approach presented in #64 and #92 but it hasn't gained much momentum as it introduces too many new problems/limitations, see #93.
Approach #1: Applying the active class via JavaScript
The idea is that either all links, or links with a Drupal-specific data attribute, are processed client-side and the server can simply ignore the "active" class altogether, allowing global cacheability by default. A "page ID hash" system was proposed in #67 that would require the server to "tag" each link with a hash that represents the URL, query parameters, language, etc... at this point JavaScript just has to match the current hash (likely also provided by the server) with any hashes on links in the DOM.
For the large majority of sites, the extra JavaScript wall time will be much lower than the rendering time saved on cached links, so we get to the "page loaded" event faster in absolute terms, and reduce server load at the same time.
Benefits:
- Guarantees global cacheability for all links.
- Unblocks future support for "active" class on "raw"
<a>
tags in Twig templates - something Twig initiative members and Core Maintainers have requested time and time again over the past few years - The "old" behaviour can be re-implemented in contrib relatively easily by removing the JS file adding "active" classes and implementing hook_link_alter()
- "active" class logic can be disabled globally pretty easily for sites that don't need it by removing the JS file
Consequences/Limitations:
- Requires javascript to work and therefore adds javascript to every page with at least one link on it. Comment #7, but note that the latest JS patches do not require jQuery as they are implemented with native JS only, as per #8.2
- For sites without javascript aggregation enabled: An extra http request will be required to load the js logic
- For sites without any other javascript at all: This would be the only javascript on the page, invoking the js parser/compiler, adding 100ms+ on mobile devices before the JS can be run at all.
- Potential "FOUC" issues, comment #63
Approach #2: "Opt-in" server-side active class logic
This approach adds an extra flag in the parameters passed to link generation functions that will determine whether Core bypasses the "active" class calculations. This means that some links will be globally cacheable and others will retain the per-page limit. Some examples of links that we can be reasonably sure either need or don't need the class can be found in comments #16 and #18.
Benefits
- Does not require JavaScript
Consequences/Limitations
- Makes an assumption that we know ahead of time what a themer will want to do with a link. Wherever we have uncertainty, we limit our cache granularity unecessarily or we limit what the themer can achieve.
- Introduces complexity and new API features into the system.
- All navigation HTML elements are still only cacheable per-page, but this content would benefit from global caching as it would have a high cache hit rate across different pages.
- Cannot be cleanly "swapped out" for a JavaScript solution in contrib.
- Continues to block "proper" usage of
<a>
tags in Twig templates. - It's unclear how the content containing a link determines whether it contains a link that has a "check active" flag set to TRUE or not. Likely it can't, and so this approach only unblocks global caching of more "hard coded" content.
Summary
The JavaScript approach is universal, simpler to maintain and more flexible, but will incur some client-side overhead.
The "opt-in" server-side logic has limitations that will likely prevent us from achieving as much as we would like in the area of render caching and introduces new complexity into the link generation API but does not introduce new client-side dependencies.
Remaining tasks
Profiling
Both of JS and the server-side logic. We have some numbers in #78 and #79, some discussion in #74 and #84.
So far, hypothetical numbers like "100-200ms" have been thrown around, unfortunately there is little hard evidence to discuss. The evidence provided actually showed a maximum of 25ms overhead from the JS - but there were no steps to reproduce, so it's hard to discuss this objectively - for example, does this 25ms represent 1 link in the page or 1000?
There is no XHProf profiling of the PHP from either of the JS or server-side approaches.
Decision making
Both approaches have WIP patches that are close to RTBC-able, both have pros and cons. Profiling of both the server-side and client-side impacts of each approach will hopefully clearly favour a single approach - if not, we're going to have to make some hard decisions.
User interface changes
It is possible that, depending on the final implementation, links that are not "navigation" links will never have the "active" class applied. Otherwise, there will be no UI changes.
API changes
The "opt-in" server-side approach introduces a new boolean flag for link generation functions that can be used to bypass/invoke "active" class logic.
Related Issues
- #1830598: [meta] support render caching properly
- #2102777: Allow theme_links to use routes as well as href, #2047619: Add a link generator service for route-based links - profiling shows that route-based link generation is slower than anything we've had in core previously
- #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page - Drupal 8 could support pages with no javascript (issue still open at time of writing)
- #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees
Original report by @catch
Part of #1830598: [meta] support render caching properly.
The active class added by l() means that any content rendered with an l() call in it can only be cached per page - otherwise the .active class will be wrong.
If we added that via JavaScript instead, the markup would be the same for all pages. This applies to the toolbar, menu blocks, and most entity rendering.
Comment | File | Size | Author |
---|---|---|---|
#257 | interdiff.txt | 2.34 KB | nod_ |
#257 | core-active-class-1979468-257.patch | 103.66 KB | nod_ |
#238 | testcases.txt | 621.27 KB | Wim Leers |
Comments
Comment #1
andypostI case different roles markup would be different. Also active trail could depend on other conditions so menus could have different render
Comment #2
catchRoles changing markup is a separate issue - that doesn't affect l() at all - see the contextual links issues linked from the one above for an approach to that.
Active trail depending on other conditions, yes that's also tricky but one step at a time - and that only applies to menus, not everything that calls l().
Comment #3
nod_initial patch, doesn't work when page has aliases and probably not with l18n either.
Comment #4
webchickComment #6
thedavidmeister CreditAttribution: thedavidmeister commentedI'd like to add that if we do this we can probably remove theme_link() completely which would be nice.
Anything that ends up using '#theme' => 'link' if #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig and #1961876: Convert theme_link() to Twig land as-is will be up to 10% slower than the same markup rendered with l(). With this we could move pretty much every link in Twig templates out of variables in the preprocess functions into
<a>
tags inside the templates which would probably speed things up further as we wouldn't even need l() at that point.Comment #7
attiks CreditAttribution: attiks commentedI understand the problem stated here, but fixing this in javascript will have some consequences:
1/ Will not work without javascript, #sadpanda
2/ Will force the loading of javascript (and JQuery) on each page, #verysadpanda
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commented#1. For me, this is not a major problem. This is 2013 and js is everywhere. I understand that this a matter of personal taste.
#2. We could certainly optimize this to run without jQuery if desired. This is a solvable optimization issue.
Comment #9
attiks CreditAttribution: attiks commented#8 To clarify 1) a bit, this will mean it will be impossible to use Drupal 8 without sending js to the client, avoiding js when possible has some advantages:
which will be a performance gain on mobile devices.
For #8-2) see #1541860: Reduce dependency on jQuery, #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page
Comment #10
catchI think we could make this optional too - I'm sure there's plenty of sites that don't use active class anyway.
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commented#11 - sure, the decision whether to provide the active class could be per-request even. It could be off by default and themes/modules would have to enable it in hook_init() or similar.
Up to now we just have been throwing it on every link "because we can" but I'd be interested in a clear outline of exactly what "active" class is supposed to provide and who is the target demographic.
For example we could say the only valid use-case is for themers to write CSS that displays "navigation" links (as opposed to links that don't navigate anywhere :P) in a different way when following that link would have the same effect as refreshing the current page. If this was the rule it would imply situations where we shouldn't bother trying to discover link "activity" with javascript, or otherwise.
Comment #12
webchickWell, it's pretty standard fare whenever creating any navigation to indicate to people where they are.
http://ui-patterns.com/patterns/ModuleTabs: "Use color coding or other visual support to indicate what tab is currently being viewed"
http://ui-patterns.com/patterns/NavigationTabs: "The current selected tab should visually stand out compared the the not selected tabs."
http://quince.infragistics.com/Patterns/Global%20Navigation.aspx: All of the examples here have a means of glancing at the nav and identifying where you currently are.
http://developer.yahoo.com/ypatterns/navigation/bar/index.html: Same deal here.
Tagging with "Usability" just to make sure I'm not missing a cluebat, but IMO we can't do away with the active class or make it optional.
Front-end vs. back-end performance is an interesting trade-off discussion, esp. since users tend to feel front-end performance hits moreso than back-end ones.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commented#12 - What about if you were rendering HTML through the Drupal API in a way that called l() and assigning it to the property of an object that was to be converted and displayed in JSON format - do we still need to spend time discovering that none of the links are active there?
Comment #14
catch@webchick most of those links are 404s for me?
Without being able to read the links, it looks like those primarily apply to primary navigation/tabs. Currently l() does this for every single link that goes through it. i.e. if you look at github, the four main tabs are styled but as far as I can see no other links on the page are (for example the my account link doesn't change). So we could apply this to /only/ menus + local tasks and that'd be a big improvement over the status quo. This would also mean the logic would be applied by tab/menu theming rather than hard-coded into l() so it'd be less drastic to swap out the implementation from there as well.
On front-end vs. back end performance, the front-end argument only holds if you have zero JavaScript at all. All sites are currently experiencing the back-end performance hit and querying menu links is one of the more costly things that happens on at least one Drupal site I'm involved in.
Comment #15
attiks CreditAttribution: attiks commented#14 I think limiting it to menu's is reasonable and moving the logic out of l() is a big + as well
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commented#12, #14 - maybe making active classes "optional" in the global setting sense isn't possible but even just allowing for passing a flag to l() that could disable active link processing on a per-link basis would be useful. Main things I can think of that "active" would be almost always useful for:
- Navigation menus
- Pagination
- Tabs
- Breadcrumbs
We could process those with javascript without causing problems.
Situations I can think of where running through l() to process "active" would likely always be wasted CPU cycles:
- Anything that's a verb/button style link but not a tab: "edit", "delete", "clone", etc.. they all lead elsewhere.
- Any absolute link that leads off-site
- Links generated as part of a Services style API callback api/*
- Links with a "javascript:" href
- Links within descriptions of form elements
- Simple username links to user profiles
- Links to the current page intended to be used as javascript widgety things (see Refine your search at the top of this page)
- The site logo
- Contextual links
- Links to a node within a "teaser" view
Comment #17
catchRe-titling. thedavidmeister's list looks like a good one to me.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedI thought of a few more things that it's a waste adding an active class to...
- mailto: links
- links in drupal messages or log entries
- links that lead to 30X redirects
I'd also like to say that if the final solution *is* a javascript snippet that it would be best to make it easy for themers to extend the selectors that determine which links on the page are processed and/or provide them with a class they can add to any link they'd like processed that currently isn't. If we do that it should hopefully mop up any navigation "edge cases" that we don't think of now.
Comment #19
nod_Elements that needs checking for links with active class needs to be tagged with a
data
attribute. We don't want to have to rely on ids or classes. With this we get out of the way of themers completely. Name could bedata-drupal-links
or something.Comment #20
thedavidmeister CreditAttribution: thedavidmeister commented#19 - +1 to that. I don't know that
data-drupal-links
would be the final name for it as it's a bit vague (there's lots of links that wouldn't have this attribute), but yeah the idea sounds good to me :)Comment #21
nod_Not on the links themselves, on the container having the links to check for.
Comment #22
thedavidmeister CreditAttribution: thedavidmeister commented#21 - why not on the link itself?
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commented@nod_ - in terms of building on the patch in #3, what about building a list of all aliases for the current system path with und langcode or the current language with a query to url_alias and then comparing each link to be checked for activity against the list of possible matches?
Comment #24
thedavidmeister CreditAttribution: thedavidmeister commentedHey, we were talking about this in IRC while discussing #1961876: Convert theme_link() to Twig.
Not sure if I'm just stating the obvious here but if we make a javascript implementation of the active class then l() is alterable and theme_link() has a preprocess so the current server-side behaviour can all be re-implemented in contrib if a site really needs the non-js support OR it could even be an option in core where you tick "active class via js" on and off on the performance page.
Comment #25
catchTagging.
Comment #26
thedavidmeister CreditAttribution: thedavidmeister commentedI was wondering, could we add a
data-lang
style attribute to links in l()? That would make it easy for javascript to know what links are active from a language perspective.Comment #27
thedavidmeister CreditAttribution: thedavidmeister commentedOr just
lang
? Possibly related: #1164682: links with a known language need language identifier.Comment #28
nod_proper attribute for that is
hreflang
. Which we should probably have added a long time ago anyway :)Comment #29
thedavidmeister CreditAttribution: thedavidmeister commented@nod_ yeah, should we open a new issue for that or carry on in the one I referenced? I do feel a little like I'm hijacking a related-but-not-same thread over there but I don't want to open duplicate issues either.
Comment #30
thedavidmeister CreditAttribution: thedavidmeister commented@nod_ there's a patch up at #1164682: links with a known language need language identifier which would make detecting the language ID of a link with javascript much easier :)
Comment #31
pwolanin CreditAttribution: pwolanin commentedclarify title
Comment #32
pwolanin CreditAttribution: pwolanin commentedsomewhat related: #2047619: Add a link generator service for route-based links
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedi like where this issue is going.
very much in favour of sending down some js with the main page that we can use to implement 'active' on cached links without Yet Another HTTP Request (YAHR, pronounced 'yaaaaaaaaaaar').
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commentedSo, to get active class handling that is equivalent to what's in PHP right now, we need:
Language handling: Pull the hreflang attribute off links (that can be cached), see issue linked above
Alias handling: @beejeebus suggested in IRC that we add a data attribute to our links (that can be cached) containing the system path so we don't have to worry about aliases if we have the current system path (route) in Drupal.settings
Query string handling: @nod suggested in IRC that we use $.deparam('') after including jquery.ba-bbq.js in order to compare the query strings of links and the current url
We also need to add a data attribute to links that indicates they should be checked by js for active-ness, this can be cached - we need to bikeshed the exact name of this data attribute a bit first.
So the final tradeoff is that instead of calculating one un-cacheable, slow attribute for each link, we calculate three (hopefully fast) attributes for links that can be cached globally.
Comment #35
nod_Query string handling: I really don't want to use $.deparm at all. Because that means we'll require jQuery on all pages ever and that's just bad. Since we don't have
?q=<path>
that use case should be marginal no?Comment #36
thedavidmeister CreditAttribution: thedavidmeister commentedkk, obviously a miscommunication in chat.
Umm, is it? It's not just ?q=
, it could be any ?foo=bar that breaks active-ness of a link. We could *maybe* make the decision to not support this, but that would be an API break. Maybe we should just write a "lite" deparam function for this so we don't have to load scripts in everywhere.
Comment #37
thedavidmeister CreditAttribution: thedavidmeister commentedsome ideas/discussion for "lite" ways to parse the query string without libraries - http://stackoverflow.com/questions/901115/how-can-i-get-query-string-val...
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commentedalso http://unixpapa.com/js/querystring.html
Comment #39
thedavidmeister CreditAttribution: thedavidmeister commentedI'm going to see if i can make a little progress on this.
Comment #40
thedavidmeister CreditAttribution: thedavidmeister commentedThis adds support for aliases, language switching,
<front>
, and queries for active classes in JS instead of l().Not sure what to do about the usage of both jQuery (I did manage to avoid using BBQ) and underscore here - in IRC @nod_ promised to have a look at removing them for me.
This will fail tests because I haven't updated any tests yet, but I think it's progress.
Also, we'll need a followup to somehow manage .active-trail I think.
Comment #41
thedavidmeister CreditAttribution: thedavidmeister commentedComment #42
thedavidmeister CreditAttribution: thedavidmeister commentedWe should profile the changes made to l() while we're at it.
Comment #44
dawehnerThis should be 'path.alias_manager.cached'
Comment #45
thedavidmeister CreditAttribution: thedavidmeister commented@dawehner, thanks for that.
Also, I had a thought. I don't think data-drupal-links-type="navigation" is needed at all, if the link has data-drupal-link-system-path we already know that it's been processed by l().
Comment #46
nod_addressed #44.
New script doesn't use jquery or underscore.goes around the ugly query string processing by adding a
data-drupal-link-query
attribute which holds the json-serialized data of the infos from the query string.Feels like doing XQuery with data attributes and
querySelectorAll
:þAn example of what selectors the script is looking for to add the active class:
And with a query string:
this is direcly sent to
context.querySelectorAll()
and voila. Performance-wise it's fine. Need to see what that does with hundreds of links on the page, but for now It's good enough.This will need some love to work on IE8, tagging.
Comment #47
thedavidmeister CreditAttribution: thedavidmeister commentedUnless I misunderstand what's happening here, I don't think that #46 is right.
If I generate a link through l() like this:
$link = l('text', 'path', array('query' => array('foo' => 'bar', 'baz' => 'bing'));
and I visit
http://mysite.com/path?baz=bing&foo=bar
then it won't work will it? the serialised data becomes sensitive to the ordering of parameters.This is a situation we have already written tests to avoid, and I tested for manually in the patch that I provided in #40, so to introduce this in js would be a regression.
Comment #48
thedavidmeister CreditAttribution: thedavidmeister commentedI had also considered adding the query as a serialised data string, but I wasn't really convinced that this was OK when we'd be doubling up on information that's already available in the href attribute.
I feel we should work with the information we already have wherever possible, and only add new overhead to l() where we have no other choice - for example, distilling a system path directly from a url in js without AJAX is impossible, so we need to store that somewhere.
Comment #49
nod_Same thing is done in PHP, in the current l function:
and I asked on IRC, that's sensitive to key order too, so it's nothing new.
I would disagree to work with existing data because it's just too slow. With serialized data it's just a matter of selecting the right elements, not processing needed.
Leaving NW because I'm sure there is a new way of doing
json_encode
that I don't remember and it doesn't work on IE9 yet because of the use ofclassList
in the JS (easy to address).Comment #50
nod_Also, this patch needs #1691394: Drupal settings gets broken by AJAX requests to work properly with ajax requests.
Comment #51
thedavidmeister CreditAttribution: thedavidmeister commentedNo, it isn't === is sensitive to order == is not. See https://drupal.org/node/1922454#comment-7187362 for a discussion of this and where the existing tests are in testLinkActiveClass().
Is it really that slow?
By the time we pick all the "active candidates" we may have 2 or 3 links, tops, in any "real" site. Then we filter for language, and we ignore anything that doesn't have a '?' character. By the time we're processing the query string, we should really only be dealing with one or two links - unless someone has built a site in a very strange way that uses query strings instead of Drupal paths/routes for a significant amount of their navigation, but we already identified that this would be a definite "edge case".
Comment #52
nod_It's slower than selecting the right elements right away.
Here is a new version, basically I added a couple of ksort() so that they are in the same order. Reordering that doesn't matter because it's only used in data-drupal-link-query attributes and won't interfere with the URL.
At the same time i understand about the query thing. But that'd make us loop over the elements twice: first to filter by checking the href attribute, then to add the active class. Accessing attributes isn't the fastest thing in the world.
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commentedThis is starting to look pretty good I think :)
This comment is now wrong.
what do we think of just doing a ksort directly on $variables['options']['query'] rather than creating a whole new variable for it? I don't see the harm in that. The order of parameters in a URL shouldn't matter and it will make everything that little bit more consistent.
This whole change of structure, moving the path related properties of Drupal.settings into Drupal.settings.path - is this the right place to do that?
// Links with hreflang equals to the current language.
equal to.
What is this doing?
Comment #54
thedavidmeister CreditAttribution: thedavidmeister commentedSince checking the query is the last thing we do, we could add the active class in the same loop if it helped.
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commentedalso, we still need to delete all the PHP tests that no longer make sense.
Comment #56
nod_I don't mind ksorting
$variables['options']['query']
but i'll leave that to someone who knows the implications.It's not actually
Drupal.settings.path
, it'sdrupalSettings.path
now :) and yes, it's the right place, will make #1691394: Drupal settings gets broken by AJAX requests cleaner.Drupal.attachBehaviors(new_content[0], settings);
Ajax-framework related mess.context
should be a DOM element, not a jQuery object, not an array.( edit ) and checking and adding the class in the same loop isn't good, performance would suffer. Reading stuff from the DOM and writing stuff to the DOM needs to be grouped separately.
Comment #57
thedavidmeister CreditAttribution: thedavidmeister commentedmind. blown.
Comment #58
nod_https://drupal.org/node/1793334
Comment #59
attiks CreditAttribution: attiks commentedSo this means every visitor has to do at least 1 extra http request to fetch the javascript and we're sending at least one data attribute for every a tag?
Will the option discussed in #24 to disable this be added as well?
Comment #60
thedavidmeister CreditAttribution: thedavidmeister commentedThe js should be concatenated, there should be no extra http request. Yes, we're sending at least one data attribute for each a tag generated by l(), is this a problem that we're transferring a little extra markup?
atm it doesn't seem to be a high priority for core and I'm kind of over it personally - pretty sure the conclusion was to make this a job for contrib. Disabling in contrib would be a simple matter of removing the js file, or overwriting Drupal.behavior.l (and then dealing with the inevitable fallout of stale cache data somehow)
Comment #61
attiks CreditAttribution: attiks commented#60 For pages that could live without javascript before, this will mean an extra http request, it also means that the call will probably be blocking resulting in a less than optimal experience for mobile users.
Hoping that contrib will solve this, is - IMOH - not the best approach, Drupal out-of-the-box should be able to solve this. Removing the js files will be easy, but this means that you loose the ability the mark a certain link as 'active', because this patch removes this, unless I'm missing something.
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commentedWhat you're saying is actually that at *most*, the visitor will have to do 1 extra http request in the case that js aggregation is disabled or you are visiting a page that previously had no js.
Honestly though:
1. What is a use-case of a page that needs the active class and does not need Drupal to build javascript files for anything else and one http request to get a tiny js file is unacceptable? Even one concrete use-case is something we can work with, a hypothetical "what if X could become a use case" is less useful as it's a step removed from anything that actually happens.
2. I'd be happy to work on a follow-up that only provides this processing for "navigation" links, as outlined in #16. Any page without navigation links would not need this javascript. We're not doing that here because it would be an API break (l() did not discriminate) and therefore make this patch much harder to get over the line.
What you're missing is that l() now is alterable so of course you can add back in the '.active' class logic in contrib.
@attiks, you've kind of just re-hashed the same "javascript is bad" argument about 4 times now, without providing an alternative or explaining how we can solve the following issue in PHP:
- Content containing links can be cached on more than a per-page basis
- l() remains as fast as possible
- No regression in "active" class functionality
I would love to hear an alternative, but AFAICS nobody has proposed one yet.
If you want to forge ahead with the "magic checkbox" approach, please file a patch, but I think it might end up being rather hard to implement without making it really clunky to work with from a DX perspective.
Comment #63
attiks CreditAttribution: attiks commented#62 Point taken and I didn't intend to keep repeating myself.
The 'problem' with forcing the use of javascript to solve this are:
All of this will slow down the rendering of the page with 100-200 msec. For the moment we're building Drupal 7 sites without any javascript, so the render fast regardless of the device or connection.
For the moment I have no idea on how to solve this on the PHP side, but the narrow the scope, I'm only talking about the links you mentioned in #16:
So I guess we need a solution that can differentiate between 'navigation' links and other links, and a way to indicate that this will invalidate the caching of that particular block. How? No idea, sorry.
PS: javascript isn't bad at all, but should only be used when needed.
Comment #64
nod_Or
Comment #66
webchickPlease see #12. We can't do away with this feature altogether. It's table-steaks for creating navigation.Ok, I guess subsequent replies said we can remove this feature from l() if we still make it possible to do an active class for navigation (menu) links. That should probably be included in this patch, then, so we don't have to open up a critical follow-up.
Comment #67
Damien Tournoud CreditAttribution: Damien Tournoud commentedTo make this simpler, as the expense of having some non-semantic component in our HTML, I suggest that during the generation of a link pointing to an internal page, we generate a hash of the internal path and add it as a
data-
parameter to the link.The result will look like:
When generating the enclosing page, we generate the hash of the internal path of the page and add it to the HTML header (either on the
<html>
tag itself, or on the<body>
tag, or as a custom<meta>
):This way we have a 100% foolproof solution that works in 100% of the cases, without having to disclose information that some might consider sensitive (the unaliased path of a page, etc.).
Comment #68
jibranI think #67 is good approach.
Comment #69
thedavidmeister CreditAttribution: thedavidmeister commented100-200ms? Well, this issue already has the "Needs profiling" tag, so I can't add it again, this does seem like a large amount of time to add 0 - 3 classes to DOM elements. The "extra request" you're talking about only exists in rather edge cases on production sites... Still no use-case for a page with zero Drupal javascript on it already and navigation links. I'm not sure how many people really are building Drupal 7 sites without any javascript at all, could you point me to an example site so I can get an idea of what we should be profiling as a "worst case" scenario?
Hah, I was avoiding this because I thought there would be an issue in breaking the API (even if it's a useless part of the API), but if @webchick says we can do this here I agree we should.
Yeah, this sounds good to me. It does seem to be the logical extension of the approach we've been taking so far with combinations of data attributes. However, it looks to me like we can't include the language in the hash, as it may or may not be set we'd end up with two hashes, that will have to be dealt with separately.
Comment #70
thedavidmeister CreditAttribution: thedavidmeister commentedComment #71
attiks CreditAttribution: attiks commented#69 To clarify: the 100-200 ms is on the client side not the server side. You're probably right the most people aren't building Drupal 7 sites without javascript, the reason is straight forward, Drupal 7 includes javascript on every single page no matter what, see #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page
Comment #72
thedavidmeister CreditAttribution: thedavidmeister commented#71 - exactly, I can't think of a single site I've built in D7 without Drupal js in the markup. I still think 100-200ms on the front end is an exaggeration, but we won't know until we have a patch and a use-case/scenario we can profile.
Comment #73
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe situation is symmetric: each link has a language (that can be the default language), and each page has a language (that can be the default language). So just hashing the language with the path should be just fine.
Comment #74
catch@attiks - if this was known to be the only js on the page, it could be altered to be inline, then there'd be no additional http request no? Even if there's a 200ms client-side regression here, enabling render caching for entities is going to be a more than 200ms server side saving.
I think we should at least allow (if not provide) a PHP implementation of the post-processing stuff we're doing in this and other issues, for sites that really want to avoid js altogether, but that needs its own issue opened.
Comment #75
thedavidmeister CreditAttribution: thedavidmeister commentedActually, the way that I wrote this is that hreflang is only added to the link if $variables['options']['language'] is not empty inside l(). You have to explicitly pass a language to l() for the hreflang to appear. This means that *currently* we have most of the links being generated by l() having no hreflang at all until you enable and configure some internationalization modules. Even when you enable internationalization, many links have no hreflang attribute added.
If the language is not passed to l(), this does not mean that the link is pointing to a URL in the current or default language, it just means that we don't know.
This could potentially be changed to work the way you describe, but that's not how it works right now.
Comment #76
Damien Tournoud CreditAttribution: Damien Tournoud commented@thedavidmeister: actually, the default language is always used when not passed:
(in
PathProcessorLanguage::processOutbound()
)Which means that (a) every link has a language, (b) we need to generate the hash after or as part of the path processing.
Comment #77
thedavidmeister CreditAttribution: thedavidmeister commentedThere's simply not a demonstrated 200ms client-side regression here... We're finding 0-3 links (usually 0-1) based on a selector and then adding one class to them, using native javacript (not even incurring the overhead of using jQuery). If javascript was so slow that adding/removing one class to a couple of elements takes 100's of ms nobody would be using it.
To achieve anything close to a 200ms regression you need to:
A: Have no CDN, not even a free service like CloudFlare
B: live in a different hemisphere of the globe to your host, bringing your latency into the 100's of ms
C: Have javascript aggregation disabled (unlikely in a production environment, and do we really want to optimise for this case anyway?) OR be comparing the performance to a page that previously had zero javascript (we still don't have a single example of this)
It sounds to me like you have to actually go out of your way as a developer to introduce this performance regression, and anyone who knows enough to turn on the core aggregation and page caching on when they push a site to production has already avoided the problem.
With aggregation on, I think the overhead will be like 1-2ms tops, but we're profiling anyway, so we'll see what it looks like then and re-open this discussion if warranted.
As pointed out previously, it's entirely possible to remove the js file and use hook_link_alter() to put the active class back server-side if you wanted to.
If you're worried about a slight FOUC, that's usually pretty easy to work around with CSS and a little JS.
I'm open to being proven wrong, I always am, but AFAICS there is no proof, evidence or even example scenario producing a 200ms regression visible through profiling.
Comment #78
nod_I did profile the front-end. Here is what I found on the time it takes to load and render the DOM (excluding all network time).
Solution #64 without JS:
Chrome: 24ms
Firefox: 110ms
Solution #52 with JS:
Chrome: 35ms
Firefox: 120–135ms.
As you rightly said, the js script in itself, not much overhead. I actually had
drupal.js
loaded as well, but this one is just declaring stuff, nothing else going on. I got those number usingperformance.timing.domComplete - performance.timing.domLoading
So the number says, on desktop, you got at least 10ms overhead for even a little JS script, inlined or not. On mobile that means at least 100ms and in the case of FF, 200ms is not a crazy number either. I'd like to remind that performance is feature #0 of the mobile initiative.
When you have no JS on the page, the browser doesn't even have to do anything for the JS, no need to load the compiler or whatever.
This is just to show that attiks is not talking crazy unrealistic numbers. I'm in favor of an all-css solution but it causes other issues and what might end up is that we go with the solution damz outlined and an even smaller piece of JS. Contrib could still get rid of it pretty easily to go for an all-css solution.
Comment #79
attiks CreditAttribution: attiks commented#77 I was going to reply, but thanks to #78 I hope it's clear I'm not imagining things, thanks @nod_ for clarifying.
PS: On my wifi here (France) on this page as an anonymous users the numbers for the js file are:
193ms blocking
+405ms waiting
+708ms receiving
+ 1250ms done parsing, compiling, ...
Total load time of the page 4.74s, without the js request still could almost save a second, which is almost 25%, on a mobile device and/or connection this will be even more significant.
PSS: The fact that the request is using SSL isn't helping either, but that's something that SPDY probably can solve.
Comment #80
attiks CreditAttribution: attiks commented#74 Inlining might solve this a bit, but I have to admit I have no idea how easy it will be to do it. The browser still has to parse and process the javascript, but we will save a blocking request. Might be a good approach if we can not solve it on the PHP side. Did you already create an issue for it?
Comment #81
thedavidmeister CreditAttribution: thedavidmeister commented#76 - You're saying include the language in the hash regardless of what hreflang ends up being? that could work :) I think I'll take a crack at a patch with the hashing in it this weekend unless someone beats me to it.
#78 - The CSS solution looks cool, but I don't think everyone wants
outline: 5px solid blue;
for their active links ;) How would you integrate this approach with a "real world" theming workflow?That profiling is just a comparison of a page with no js and a page with js, it's not profiling the time taken to execute the new code introduced by this patch. There's still no explanation of how we get a page rendering nav elements without at least one other javascript file already on the page and aggregated with the active link js, or a live example of a CMS based site doing this that I could look at - it still all sounds a bit theoretical/edge case to me.
The time lost executing the js in this patch when there's already js on the page is much smaller than the time that can be gained by caching rendered HTML server side. As @catch said, enabling render caching for entities will likely beat even the 200ms loss in an *absolute worst case scenario for js* on most sites.
There's no evidence here of a mobile device taking 100ms minimum to parse a tiny js file, or are we talking about network latency here? the profiling shows FF taking 10-25ms to parse the js, not anything like 200ms, not sure where that claim comes from, that FF will ?randomly? take 10x as long as we see in profiling to parse a js file.
Please, this is becoming a discussion as to whether javascript in general is overused or not, rather than a discussion of a patch that introduces a really tiny amount of DOM manipulation. Also, d.o. is running on Drupal 6, so it's not a good example for demonstrating how Drupal 8 will perform.
Comment #82
Damien Tournoud CreditAttribution: Damien Tournoud commentedYes. All URLs we generate have a language, even if there is no hreflang.
Comment #83
thedavidmeister CreditAttribution: thedavidmeister commentedl() doesn't see what url() is doing internally though, it would have to figure out the default language on its own.
Comment #84
attiks CreditAttribution: attiks commented#81 Regarding the profiling - and it probably is me - but you have to profile against both scenarios: a page using some js already and a page using no js at all.
Take for example this page (https://drupal.org/node/1979468) as an anonymous user: what is the use case to add any javascript? Drupal.behaviors contains the following:
So for this page it is the search box and GA, if you remove these there's no need for any js whatsoever. Why remove them? You gain at least a full second (including dns lookup, blocking other requests, waiting, fetching, parsing, processing) for the page load/ready. To be honest, I would love to see d.o. load a lot faster on mobile using 3G
It includes everything.
If only, this patch is forcing all sites to output js, just to support an active class on a menu item, so yes, this is related to the patch in this issue.
We (as a community) are spending lots of hours to improve the speed of all js added to core, to make sure it behaves quickly on all (most) devices, forcing js on all pages will almost obsolete all those efforts. This will probably be the first drupal version that finally gets the much needed attention for front-end performance, and maybe this patch is jeopardizing most of it.
Regarding d6 versus d8, you're right, they cannot be compared.
PS: If you want to believe my use case is an edge case, feel free to do so, I have no intention/energy to convince you.
Comment #85
jcisio CreditAttribution: jcisio commentedPatch #52 uses inefficient selector (without tag name). The JS execution could be much faster. Also, it's quite easy to remove this JS file if we don't want to add the "active" class. I personally never see it useful.
That said, I'm with attiks. If a website is designed for mobile, it is not unusual that there is no JS.
Comment #86
dawehnerI guess we could just use the $request->attributes->get('_route_name') etc and the url generator to generate the url of the current page and based upon that build the hash.
I really like the simplicity of the approach.
Comment #87
thedavidmeister CreditAttribution: thedavidmeister commentedOk, let me come up with a list of things I think we should profile so we can make an informed decision on performance, we're *all* speculating right now, myself included, and that's just unproductive. The profiling done so far is good but doesn't show the "bigger picture", it's focussed on the worst case scenario, doesn't cover mobile devices and doesn't show the speed changes server-side (which may be faster or slower at this point based on the changes we made to l()).
I'll have a think this weekend and try the hashing approach as well as a list of things we can profile.
Comment #88
tstoecklerPer #16 of this issue I think we should investigate making the adding of the active class opt-in by default. As @thedavidmeister points out there we generate a lot of links which conceptually will never be active. The edit link for ConfigEntities to name just one example. We could then make theme_menu_tree() and theme_local_tasks() etc. to set the 'set_active_class' option to TRUE, so that there would be no actual regression in functionality in 98% of the cases.
Comment #89
thedavidmeister CreditAttribution: thedavidmeister commented#88 - we could do this without javascript. This is saying that we only need to apply the active class for groups of links with a parent that could be sensibly given the
role="navigation"
attribute, yeah?http://www.w3.org/TR/wai-aria/usage#usage_intro
http://www.w3.org/TR/2011/WD-role-attribute-20110113/
Comment #90
thedavidmeister CreditAttribution: thedavidmeister commentedA rough proof of concept patch for #88. Only setting needs review for the testbots.
Comment #92
JohnAlbinI've been following this issue for about a week. I'm not sure why no one reviewed Nod's patch in #64. In it, he threw out all the JS and just injected an inline CSS rule to apply the active styling via a CSS3 selector. For example, on the /user/1 page, this CSS is injected:
(The outline property is just used for testing purposes; makes it really easy to see the styling apply.)
The attribute CSS selector, and the :not() pseudo-class are part of the CSS Selectors Level 3 spec. And it has very good support in browsers. http://caniuse.com/#feat=css-sel3 Basically, the CSS his patch uses is only lacking IE8 support (which we don't support anyway.)
The only issue I see right now is that the styling (meaning the specific set of properties) has to be hard-coded into the inline CSS. You can't (yet) specify that this selector should use that selector's styling. So we would need a way to override the properties used by default in core. An alter hook of some sort should work.
I really like the patch in #64.
Can we refine that approach?EDIT: Actually, let me clarify. I would either want a CSS-only approach (with no JS), like #64, or go back to the D6-style of just applying the active class to menu links. Given that menu blocks are already limiting the ability to cache as they expand/collapse based on the URL.
Comment #93
thedavidmeister CreditAttribution: thedavidmeister commentedI did ask in #81 how it would fit into a themer's "real world" workflow and didn't get a response so I assumed that was the end of it, but if we really want a review of #64...
An alter to modify hardcoded inline CSS rules for one selector something that is very commonly used and will never be correct "out of the box" is just about the worst themer DX I've ever heard.
- Not compatible with CSS preprocessors SASS/LESS
- Requires themer breaking up their workflow to perform common tasks and fragments stylesheets arbitrarily
- Breaks backwards compatibility as existing .active rules will not magically be ported to the inline styles
- Is nowhere near as flexible as using .active, you couldn't do
:not('.active')
, or even.active img
, for example- Raises the bar of minimum knowledge required to build a theme from basic CSS to reasonably advanced PHP + Drupal familiarity
- Is completely undocumented
- Lacks IE8 support
- Mixes logic (alters) with presentation (CSS)
- Breaks the C (cascading) in CSS by forcing all active link styling to sit in one place - can't override these styles in later stylesheets
- No ability to modify the specificity of the rules, so we're looking at !important or nothing in the inline CSS
- Is not aware of which theme is currently active
- We're basically heading down the path of inventing a Drupal-only CSS preprocessor/generator that's raison d'etre is to workaround a few bugs, which is inevitably going to be worse than LESS/SASS
Comment #94
thedavidmeister CreditAttribution: thedavidmeister commentedHave a look at building on #90 for this.
Comment #95
pwolanin CreditAttribution: pwolanin commentedI think all tabs need the active class also? I guess there is also an active class there put on the parent element that's used for styling? In any case I don't see why that would affect caching since I think that's outside the page content.
This has the language block add the active class, since that seems fairly central to its behavior.
Also, puts the option to add the class into Url test code that's expecting it.
Comment #96
nod_Comment #97
nod_Sorry didn't mean to change status.
Comment #99
pwolanin CreditAttribution: pwolanin commentedShould fix the other test fails.
Comment #100
pwolanin CreditAttribution: pwolanin commentedComment #101
jessebeach CreditAttribution: jessebeach commentedThere's quite a number of impressive solutions and cleverness in this thread.
In order to complete #1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient (Critical), we've been working on #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees (Major). This is issue here has implications for 1805054. Namely, what we're trying to do in 1805054 is eliminate the cache-busting behavior of the active path on a rendered menu tree. Menus have a per-link toggle for expand/collapse of children. A collapsed parent node in a tree with be expanded if it part of the active trail of the current page path. In order to prevent this cache-busting, we've proposed to render the full menu tree (to the requested depth). The active path qualification in the markup would need to be applied in the client, which is why this issue is a soft dependency for 1805054.
Having read through this issue, I've attempted to pull together several of the key ideas strewn about the various patches. Those that I could identify are:
hook_library_info_alter
and the legacy active link behavior reintroduced.To that end, I'm attempting to combine #99, #64 and #52, then work in Damien's hashing suggestion from #67 and tweak the JavaScript to use those hash values to do that active and active trail markup. I have a patch in progress that I'll hopefully post tomorrow.
Getting client-side active link and trail marking will let us proceed with 1805054. Does that plan seem reasonable?
Comment #102
attiks CreditAttribution: attiks commented#101 If I understand correctly the problem is for the admin menu, I think we should treat this separately from regular menus. The admin menu is a huge menu and I think it's save to assume that people with access to the admin menu will be using javascript.
I think the ideal would be to cache the menu in an intermediate form (not rendered, page agnostic) so we can process it to add the active, collapse info before rendering it into HTML/json. If that doesn't work for admin menu, we might allow the user to choose to use a full json solution with all rendering done on the client side.
I'll try to think a bit more about this and to have a look at all patches in the linked issues to get a better understanding.
Comment #103
Gábor Hojtsy@attiks: indeed the page agnostic caching/rendering is happening in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.
Comment #104
thedavidmeister CreditAttribution: thedavidmeister commented#102 - just a minor point, but it's not just the menu, we've also identified pagers, breadcrumbs, tabs, and language switching as links that definitely need active link processing.
Are we going back to javascript now? lol, I've lost track of what the plan is now >.<
I was under the impression that what we're working towards is trying to enable a render cache that works across both pages and users. Allowing *global* caching for links, so simply having a link in content doesn't affect the entire content's cache-ability.
I don't see how authenticated/unauthenticated has anything to do with this? That would just replace a per-page cache limit with a per-user cache wouldn't it? That's a side-grade, not an upgrade :/
Anyway, I think I'm just confused now, I'll have a look again when there's a patch :)
Comment #105
jessebeach CreditAttribution: jessebeach commentedI'm looking into this approach. I agree that the admin menu is a special flower and might warrant a singular approach.
Comment #106
thedavidmeister CreditAttribution: thedavidmeister commentedSo we now plan to split the active class handling between l() and js?
Comment #107
jessebeach CreditAttribution: jessebeach commentedNo, sorry if that's what I implied. I don't want to hijack or derail this issue. After reading through it, I simply wanted to explore the idea of resolving this critical #1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient without finding resolution to this issue here, which is still under debate.
Comment #108
pwolanin CreditAttribution: pwolanin commented#99: 1979468-99.patch queued for re-testing.
Comment #110
pwolanin CreditAttribution: pwolanin commentedre-roll for conflict in ImageFieldDisplayTest
Comment #111
thedavidmeister CreditAttribution: thedavidmeister commentedThere's also an active class on li elements produced by #2102777: Allow theme_links to use routes as well as href that needs to be dealt with in the same way as the a tags.
Comment #112
mgifford#110: 1979468-110.patch queued for re-testing.
Comment #114
moshe weitzman CreditAttribution: moshe weitzman commentedPerhaps someone can update the Issue Summary? Kind of hard to know where this one is at. Thanks.
Comment #115
thedavidmeister CreditAttribution: thedavidmeister commentedHey, in another attempt to curry favour for the js side of things, can I just point out that putting the active class in js allows you to do this in your Twig templates:
<a href="/some/url">foo</a>
Whereas, currently you can't do that and output a link functionally consistent with the rest of Drupal.
So, there's actually benefits beyond the render caching discussion.
Additionally, the theme_links() thing that I linked to just added another layer of complexity to the "server side opt-in" thing...
I really think the js patches should be strongly considered as a resolution here.
Comment #116
jwilson3That's a good point. It would also let hand curated HTML anchor tags inside textareas (eg in the body of a node or in a sidebar block list of links) get the active class added, which could be cool, but how would anchors to another part of the current page, potentially with or without a full root-relative path before the '#' fragment work? Eg. would JS add the class regardless of the '#' fragment, and leave it up to the themer whether to theme the active class inside a body field or sidebar differently?
Comment #117
thedavidmeister CreditAttribution: thedavidmeister commented#116 - I assume the logic would stay the same as it is server-side, at least initially. Which is... fragments do nothing...
Comment #118
thedavidmeister CreditAttribution: thedavidmeister commentedBumping the priority of this in light of profiling results like https://drupal.org/node/2102777#comment-8003157
Comment #118.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #118.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #118.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #118.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #119
thedavidmeister CreditAttribution: thedavidmeister commentedI had a go at updating the issue summary, trying to incorporate all the relevant discussion from the thread here.
Comment #119.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #119.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #120
thedavidmeister CreditAttribution: thedavidmeister commentedHere's a cross post of profiling demonstrating how the "is active" logic for theme_links() with the new routes system adds much more overhead server-side than we've seen historically with functions like l().
With "is active" logic server-side, converting theme_links() to use routes - https://drupal.org/node/2102777#comment-8003157
Without "is active" logic server-side, converting theme_links() to use routes - https://drupal.org/node/2102777#comment-8004867
We can see that using routes for "is active" logic leads to a 1880% increase in wall time for theme_links(), or 62ms to 1.24 *seconds* for the test case.
Put another way, for theme_links(), the "is active" logic represents around 95% of the total server-side render time.
Using JavaScript seems like it could end up being faster even before we consider render caching, but of course, that would come down to how performant the "link hash" process suggested would be.
Comment #121
thedavidmeister CreditAttribution: thedavidmeister commentedminor update: the profiling in #120 was for 200 links.
Comment #122
thedavidmeister CreditAttribution: thedavidmeister commentednm, false alarm on the profiling. Looks like we've managed to come up with something much less expensive for the "is active" logic:
https://drupal.org/node/2102777#comment-8014741
I think we should forge ahead with the "opt-in" approach to generating the .active class for now, so we can get *something* in ASAP, and then go back to the client-side/server-side discussion in a followup.
Comment #123
Wim Leers#122: wow, very happy that that was a false alarm, because that was terrifying!
#115: I don't know Twig well enough, but wouldn't that fail because the required
data-
attribute — no matter whether that's a hash or system path or something else — (to identify those links that need the"active"
class) are missing?Overall issue analysis
I've read through the entire issue and want to see it moving forward again instead of stagnating. A lot of different facts, opinions and strategies have been weighed. But so far, it's only been about "the JS solution" vs. "the PHP solution" (the CSS solution seems to have been shot down).
If not for the "JS solution will cause perf problems for some sites" reasoning, then the JS solution would probably have been picked already.
I want to add just one mere fact to that: we worked hard in Drupal 8 to in fact guarantee that out of the box zero JS will be served to anonymous users, because this does improve front-end performance. And since a few days, that is now being guaranteed through test coverage: #2120457: Add test to guarantee that the Standard profile does not load any JavaScript for anonymous users on critical pages :)
Proposal
Now, why not use the best of both worlds? We don't want JS because it'll make no-JS sites slower. We don't want PHP because it still doesn't allow for global caching. So why don't we:
Comment #124
pounard#123 sounds like a good compromise.
EDIT: It would even be better if that was purely configurable (allow the site builder to choose the method for both).
Comment #124.0
pounardUpdated issue summary.
Comment #125
thedavidmeister CreditAttribution: thedavidmeister commented@Wim Leers - with regard to the Twig stuff, I was oversimplifying in #115, you also "can't" spit out a href attribute like that and it be "Drupal safe" - #2073811: Add a url generator twig extension - the point was more that we wouldn't be *blocked* by active classes in Twig and could start making steps towards some final resolution than the problem would be 100% solved here. I should have been a lot clearer about that, sorry.
#123 seems like a good compromise to me too. We can actually combine a lot of the work that has already been done in this thread to achieve it.
The question I have though, is that we want to achieve a *global* cache granularity for links, but this introduces a new kind of cache, which is global anon/auth - better than per-user or per-role, but not truly global, unless I'm missing something? Is there an example elsewhere in core of caching that works like this?
Comment #126
Crell CreditAttribution: Crell commentedWhile by and large I dislike the whole "do X for the special case of anonymous, and do Y for everything else", the proposal in #123 seems to be the best bet here. My only concern is what that might imply for partial page caching / block caching. If that's not going to be a problem, then I'm comfortable moving forward with that.
Comment #127
Wim Leers#125 and #126 seem to voice the same concern: "The #123 proposal means we won't have global caching, but just role-based caching, is this a problem?".
AFAICT, the answer is: no, that is not a problem. Global caching would in theory result in higher cache hit ratios/less cache entries. However, in practice, pretty much everything that gets render cached is cached per combination of user roles already anyway. The current issue title says " forces an upper limit of per-page caching for all content containing links"; this would go away and would change into "requires per-role caching granularity for all content containing links, but pretty much all contents already has such caching granularity anyway".
And indeed, as #125 already indicates: we already have all the crucial pieces, we just need to combine them into a single patch :)
Comment #128
Wim LeersAs #125 suggests, this takes the work already done in this issue (#64 for JS and #110 for PHP), and merges the two.
Enhancements over those patches:
l()
will now adddata-
attributes for authenticated users and generate the"active"
class for anonymous users — as proposed in #123.data-
attributes for every invocation ofl()
. This patch takes advantage of the work done by #110 to only add them ifset_active_class === TRUE
.drupal
library and assumed that would always be loaded, which is not true. This patch updatessystem_page_build()
to add the newdrupal.active-link
library if the current user is authenticated. It must happen in a stand-alonehook_page_build()
implementation simply becausel()
does not go throughdrupal_render()
, so we can't#attach
anything insidel()
.Comment #129
Wim LeersComment #131
Wim LeersThis should make all tests pass.
Comment #132
catchThe different logic for anon vs. auth works for me.
While hook_page_build() is OK for now I think could we potentially do something with #type => 'link'?
Comment #133
Wim Leers#132: Two questions:
l()
call much slower then? I don't want to make claims without data to back it up thoughl()
and requiring developers to always create a render array and calldrupal_render()
on it a DX regression?However, as you say, that would be a follow-up (and a pretty large API addition/change).
So, focusing on the task at hand: what else needs to happen here to get to RTBC?
Comment #134
moshe weitzman CreditAttribution: moshe weitzman commentedI think that this is a good stopping point and work can go into follow-ups
Comment #135
larowlan131: active_link-1979468-131.patch queued for re-testing.
Comment #137
dawehnerSo we do touch the l() method but we do not touch the link_generator, which is actually recommened compared to l()? Is this something for a follow up?
Comment #138
larowlanNo longer applies, re-roll
Comment #139
larowlanComment #141
Wim Leers138: active-class-1979468.136.patch queued for re-testing.
Comment #142
Wim Leers#137: Sigh. You're right :( I guess we all forgot about the mess that link generation is. I'll try to reroll ASAP.
Comment #144
dawehnerThere should be for sure a follow up to move the logic of l() into the link generator to at least keep the logic in one file. Damn I planned on this months ago.
Comment #145
Wim LeersThis reroll adds the same
$options['set_active_class']
support toLinkGenerator::generate()
as it did previous tol()
.Includes comprehensive unit test coverage.
This may come back red: if there are any systems that are using
LinkGenerator
and are testing the presence of the "active" class. Testbot will tell — that should be an easy fix, then.Note: due to the updated + extended unit tests, this patch has nearly doubled in size.
Comment #147
dawehnerLet's inject the current user via a setter so we can synchronize it.
We always use a "\" at the beginning.
Let's not call that a service.
It seems to be that this test could be simplified by using a dataProvider.
Comment #148
Wim LeersHEAD changed; straight reroll.
Comment #150
Wim LeersApparently I didn't merge the conflict correctly, that's what's causing those thousands of exceptions. It's also causing at least part of the fails, possibly all. Fixed in this reroll.
#147:
Hopefully this will come back green.
Comment #151
Wim LeersComment #152
dawehnerYou need an "=" at the end to set it to be synchronized.
Yeah, let's go with the least currently used alternative, so we have more standards :P
Should be "\"
Maybe this has been asked before: Why do we not use the route name + parameters instead of the path? Additional we should add a @todo to allow url generators to disable its path processing completely, as this is what we want to do here?
Oh this looks odd. Any reason we use multi-line comments here? If you need some of those extra documentation you might move it to an extra doTestLanguageLinksActiveClass or something like that?
<3 <3 <3 <3
Comment #153
Wim LeersThe reason is simple: lowest common denominator. A
(route, parameters)
pair can be converted into apath
, but not the other way around. If we don't want to make the JS slower (we don't) or more complex (we don't), then this is the only sane choice AFAICT.Once we don't use any paths anymore, i.e. once everything is route-based, we can get rid of
data-drupal-link-system-path
and replace it withdata-drupal-link-route
anddata-drupal-link-route-parameters
.Comment #154
Wim LeersBlocks #2151459: Enable node render caching, which is critical, hence marking this critical.
Comment #155
xjmBeta blocker as a blocker for #2151459: Enable node render caching.
Comment #156
moshe weitzman CreditAttribution: moshe weitzman commentedI propose we close the critical, and deal with 2) in a follow-up
Comment #157
webchick/me tosses this one to catch
Comment #158
thedavidmeister CreditAttribution: thedavidmeister commentedDare I ask about theme_links() support? It currently has .active class logic identical to l() and is also explicitly mentioned in the issue title and summary.
Comment #159
Wim Leers#158: ARGH! What's with this duplication of the same logic in not 2, but 3 places!?
Working on a reroll.
Comment #160
Wim LeersI probably spent about 9 hours on this today, to get
theme_links()
support done, including test coverage.Notes:
"active"
class to live on<li>
elements rather than the<a>
child elements, because in the latter casetheme_links()
could just reuse the existing logic inl()
andLinkGenerator::generate()
.But, unfortunately, there is a good reason: it must be set on the list item elements because otherwise it's impossible to write CSS that targets the list items of active links only. After all, it's impossible to write a CSS selector equivalent to this one:
jQuery('li:has("a.active")')
(that will only become possible in CSS4, implemented by zero browsers today). So, alas, we're stuck for now with the silly notion of having to calculate the"active"
class for list items containing links rather than for links themselves.This explanation has been included in
theme_links()
's docblock for posterity, in case they also think "WTF is this nonsense?" — it's sensible nonsense :)theme_links()
is weak: there's only\Drupal\system\Tests\Theme\FunctionsTest::testLinks()
and that's hardly proper test coverage: it's aWebTestBase
that should at least beDrupalUnitTestBase
. However, improving that is out of scope for this issue. The updated patch provides sufficient test coverage.l()
, it'll be trivially easy to get rid of the code path for "active class checking on paths" because of the way I've included the code. There's slight duplication, but that's good, because that makes this future clean-up (oncel()
is gone) simpler.active-link.js
set the"active"
class on other HTML elements than<a>
. So I've updated the JS to be element-agnostic where it wasn't already.Comment #162
Wim LeersThe two fails both occurred in
LanguageSwitchingTest
: the language switching block usestheme_links()
, but 1) it wasn't setting#set_active_class = TRUE
yet, 2) the tests weren't updated accordingly. Now they are.I also slightly simplified the changes I introduced in #160.
Comment #163
Wim LeersComment #165
Wim Leers162: active_link-1979468-162.patch queued for re-testing.
Comment #167
Wim LeersEither HEAD or testbot is broken ATM.
Comment #168
Wim Leers162: active_link-1979468-162.patch queued for re-testing.
Comment #169
moshe weitzman CreditAttribution: moshe weitzman commentedThe theme_links() changes are solid. Back to RTBC.
Comment #170
webchickThis issue feels catch-ish.
Comment #171
catchThis is a bit contradictory. Either we're handling active via js for auth users and it's fine to use this for stuff that can be cached better than per-page. Or we're not fixing that and it should be used sparingly. We could say use it sparingly anyway, since it's extra processing that's not necessary when its not needed, but that's not the current state of the comment.
Also, content for anonymous users, that's got the active class calculated in PHP, two things can happen:
1. If it's the page cache, that's fine - since that's per page.
2. If it's an element that's rendered cached, appears on multiple pages, then it's going to need to be per-page caching (or not have the active class, or have the active class calculated via js, or use post_render_cache so that it can be calculated per-page with the rest cached). Just because something is cached for an anonymous user, doesn't mean that it's per-page.
Comment #172
Wim LeersAfter an additional discussion with catch about this, we concluded that we can only solve this by using
#post_render_cache
for anonymous users. That still means that when the page cache is used, no#post_render_cache
callbacks need to happen! :)I'll work on this next.
Comment #173
Wim Leersl()
,LinkGenerator::generate()
andtheme_links()
all return strings, not render arrays. Consequently, it's impossible to use#post_render_cache
directly.We could use it indirectly though: we can implement
hook_page_alter()
and then set a#post_render_cache
callback that applies to the entire page array.At first I thought that would be awful, but upon trying it, it's actually very nice! It's effectively a PHP implementation of the JS library: it users PHP's
DOMDocument
to reliably find the relevant links. By first doing a simple string-based check to see whether any link on the page is a potential match, we reduce the overhead on pages without active links to quasi zero (significantly less than a tenth of a millisecond). The cost when there are active links is about 3 milliseconds, which feels acceptable.Instead of having duplication of complex auth-vs-anon-switching logic in
l()
,LinkGenerator::generate()
andtheme_links()
, we now just always do the same simple thing: we adddata-drupal-link-*
attributes. Then, insystem_page_build()
, Drupal chooses to apply the JS solution for authenticated users and the PHP solution for anonymous users. But because that is now being declared declaratively, it's trivial to change that with a simplehook_page_alter()
, to either:Consequently, all link generation functions are now idempotent: their output no longer depends on the current path or the active user. They always generate the same output, and it's up to post-processing (in either JS or PHP) to modify them to fit a certain context.
I'm very happy with this change. I hope others will share my enthusiasm, and I think they will :)
P.S.: I also changed two things in
ajax.js
that fix two incorrectDrupal.(attach|detach)Behaviors
calls, which caused the JS added in this page to trigger a JS error.Comment #174
Crell CreditAttribution: Crell commentedAll link generation being idempotent suits me just fine.
My one concern is that there are still other patches in the queue working toward eliminating hook_page_alter. Ideally, each block will render in isolation (which precludes hook_page_alter). Can the same logic be done in hook_block_view_alter (or whatever it's called), so it's done on a per-block level rather than page-level?
Comment #176
Wim LeersRerolled, patch should come back green(er).
Also changed: two 80-col-rule violations and a case-sensitive check that should be case-insensitive (thanks to nod_ for pointing that out).
EDIT: And while working on this, I found that batch system's tests were broken: #2158259: Batch system's ProcessingTest has some broken tests.
#174:
First, please know that the patch is not using
hook_page_alter()
, buthook_page_build()
.I merely said contrib modules would probably use
hook_page_alter()
to change the defaults. But I guess contrib modules could also just usehook_page_build()
+hook_module_implements_alter()
to ensure their code runs after Drupal core's.Second, it can work at the per-block level just fine. But then we need to be entirely sure that by looking at all blocks, we still see ALL the HTML markup. If we're generating a page and then we're applying the same replacements for each block, then we're effectively slowing down things unnecessarily.
So: yes, that's possible, but it's out of scope for this issue.
Comment #177
catchThe requirement for this is for it to run during drupal_render() at the highest possible level. Exactly which insertion point is the highest possible level is out of scope here, so it should just use the most sensible one, which is hook_page_build() at the moment (and the same for adding page level assets too).
Comment #179
Wim LeersThe sole exception that caused the patch to come back red:
That's most definitely unrelated. Let's try again.
Comment #180
Wim Leers176: active_link-1979468-176.patch queued for re-testing.
Comment #181
Wim Leers176: active_link-1979468-176.patch queued for re-testing.
Comment #183
webchickLazy testbot.
Comment #184
moshe weitzman CreditAttribution: moshe weitzman commentedInterdiff looks good and new test coverage is swell. I will RTBC this once it comes back green.
Comment #185
Wim LeersStraight reroll.
Comment #186
Wim LeersIntroduced a small mistake in the straight reroll. Fixing.
Comment #189
Wim Leers186: active_link-1979468-186.patch queued for re-testing.
Comment #191
Wim LeersDamn, it's hard to get & keep this patch green! This time, it was broken by #2068471: Normalize Controller/View-listener behavior with a Page object.
However, #2068471 has actually made this patch a little bit cleaner: before there were two
drupal_render()
calls per page load: one for the entire HTML document, one for the "main content". Now only the latter remains, so the ugly if-test inSystemController::setLinkActiveClass()
could be removed. It also means that the changes toDefaultMobileMetaTagsTest
could be reverted, making this patch a little bit smaller.Comment #192
moshe weitzman CreditAttribution: moshe weitzman commentedBack to RTBC
Comment #193
webchickWicked!!
Since this is the last blocker to node render caching, seems like it's fitting for catch to do the honours here. :)
Comment #194
catchChoirs sing etc.
Committed/pushed to 8.x, thanks!
Needs a short change notice for the change to l().
Comment #195
Wim LeersChange notice created: https://drupal.org/node/2167077.
Comment #196
Wim LeersComment #197
jibranNot pushed yet.
Comment #198
jibranPushed to 8.x 624296e.
Comment #199
nod_Use of classList in the JS was to avoid depending on jQuery but is not available on IE9, hence we should introduce the classList polyfill #2167431: Add classList polyfill for IE9
Comment #200
nod_The change in structure of
drupalSettings.path
broke back to site button: #2167611: Back to site button brokenComment #201
ianthomas_ukThis patch has broken tabledrag, see #2168233: tabledrag javascript does not update weight fields when dragging rows
Comment #202
longwaveThis patch also breaks any non-ASCII characters when rewriting the output, as
$dom->saveHTML()
returns malformed HTML entities when given UTF-8 input.Comment #203
catchOK three criticals is a bit too much, I've rolled this back for now.
Comment #204
nod_Added those patches on top of #191
#2167431: Add classList polyfill for IE9
#2168233: tabledrag javascript does not update weight fields when dragging rows
#2168171: ajax.js passes context as jQuery object to attachBehaviors and detachBehaviors, should pass plain object
With the rollback, back to site button is re-broken. No JS errors left with this patch.
peterpoe should get commit credit for his work on #2168171
Comment #205
longwaveThis is #204 with an addition to demonstrate the UTF-8 bug I mentioned in #202.
Comment #207
jessebeach CreditAttribution: jessebeach commentedThe original patch was not rolled back yet in 8.x. Here's the patch from #205 on top of HEAD (c241c1ba).
Comment #208
jessebeach CreditAttribution: jessebeach commentedDerpy derp. Ignore #207. Apparently "pull" is merely a suggestion to my version of git :/ I see it was reverted in e4124f0a.
Comment #209
longwave205: core-js-active-links-1979468-205.patch queued for re-testing.
Some of the fails in #205 were unrelated, testbot ran out of disk space.
Comment #211
Wim LeersTurns out the solution was much simpler than expected :)
Comment #212
nod_That's RTBC if it comes back green.
Comment #213
webchickBack to catch-o-rama
Comment #215
webchickHeh, nevermind. :)
The failure's in JavaScriptTest which sounds like it might be legit.
Comment #216
longwavedrupal_add_js() => _drupal_add_js()
Comment #217
Wim Leers#216: D'oh! Thanks for the reroll :)
Comment #218
nod_All right, Green.
Comment #219
webchickOne more time. :)
Comment #220
catchThis actually needs Dries for the new library no?
I already committed this, so happy for anyone else to commit it now it's green again.
Comment #221
catchComment #222
Wim LeersExtra context: the library that catch is referring to is the one added by nod_ in #204, which initially was in this follow-up issue: #2167431: Add classList polyfill for IE9 (but because this patch got rolled back, that also got rolled back).
Comment #223
longwaveDoes this have the same concerns as #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5, as it also uses DOMDocument to parse and rewrite the entire markup?
Comment #224
Wim Leers#2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses broke this patch in a single hunk.
Comment #225
webchickOops, sorry! Too hasty on the assigned-to field. :) Dries should be able to look at this on Friday.
Comment #226
Wim Leers#223: I had no idea that issue existed, and in all my searches regarding PHP's
DOMDocument
, I did not find out that about the HTML5 example that mfer cites in #1333730-48: [Meta] PHP DOM (libxml2) misinterprets HTML5 to be problematic.Awesome catch! Even though this means this is again no longer RTBC :(
I've spent hours looking for alternative implementations. There isn't any elegant one. There are essentially 3 ways to implement this without using an user-space HTML5 parser library:
DOMDocument
to find and update the affected HTML elements.DOMDocument
to find the affected HTML elements, call$node->C14N()
on them to get their HTML representations and then usestr_replace()
to replace them in the HTML string — that way we avoid callingDomDocument::saveHTML()
which can break HTML5 as described above.Unfortunately, this can't work because
->C14N()
does not retain the same attribute order as the original HTML — there's nothing wrong with that, but it just means we can't useDOMDocument
to find matches and then use string manipulation to update the HTML.strpos()
) to find matching HTML, then find the opening and closing brackets for that tag, load the detected HTML (e.g.<a href="/" data-drupal-link-system-path="<front>">
in a
DOMDocument
, parse it to check whether the language and query string match, and if so, also use it to add the "active" class. Then save that back into HTML usingDOMDocument::saveHTML()
. Since we're then usingDOMDocument
to only manipulate a single tag at a time, we avoid the lack of HTML5 support: there's no problem when manipulating a single tag!Option 1 is what we had and #223 shows why we can't use that (yet). Option 2 is inherently broken.
Therefor I implemented option 3. It works. This is a rough implementation. We'd want additional test coverage, even though all tests pass. I want to make sure it's an acceptable option before I work on additional test coverage.
Comment #228
dawehnerThere are MANY examples in core which uses "Defaults to FALSE." instead of using parenthesis
I like this cross-linking.
Oh I thought we would have killed odd/even from core, as CSS3 should be able to do the same.
If we care so much of performance I wonder whether calling a lot of code from the url generator twice is a good idea. Beside from that I think that getPathFromRoute (in contrast to generateFromRoute should never process the path aka. produce a path alias, so not really need this additional dependency.
It would be also interesting to have some similar functionaliy (skip path processing) into generateFromRoute as well, given that it existed in D7.
Oh I thought we don't add var i into the for loop. Is there any reason we cannot use i++ here? At least javascript allows that.
We use elseif, but please tell me why if you know.
I wonder whether we should split this up into multiple lines to make it more readable.
We certainly should add some performance testing how well this performs.
This seems to be potentially unit-testable and is certainly complex enough to do so.
Comment #229
Wim Leers#228: I appreciate the review. But not really right now. This is a nitpick review. Please see #226. This was RTBC for the umpteenth time, but needs work again because of PHP
DOMDocument
suckiness. Please review the interdiff of 226 instead.First: we care about how the generated links affect the render cache, not necessarily about the performance of link generation itself.
Second:
URLGenerator::getPathFromRoute()
is called only ifset_active_class === TRUE
, which is only rarely the case.Third: I don't get the part about alias handling at all — if it helps:
URLGenerator::getPathFromRoute()
gets the internal path, not thepath alias
!Fourth:
generateFromRoute()
is onURLGenerator
, which we don't touch at all. Hence adding "skip path processing" to that is off-topic.Reroll fixes the remaining test failures and the nitpicks in #228.
Comment #230
amateescu CreditAttribution: amateescu commented#1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors has landed :)
Comment #231
longwaveI am not sure this is multibyte-safe; there might be some edge cases where this incorrectly matches part of a multibyte character, for example if the <a> tag has a "title" or other attribute containing UTF-8 text?
Similarly I think this might need to use drupal_substr() (and drupal_strlen()) to be multibyte-safe.
Comment #232
Wim Leers#230: hehe :) Only after I posted that patch though! :D
#231: yes, that's the stuff I'm not sure about. That's the stuff we want test coverage for. I'm aware of all that.
What I need to know is: is this approach acceptable in the first place?
Comment #233
longwaveAs you described in #226 and as noted in #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 there is no good solution to rewriting HTML tags after they have been output as strings. When I first saw the earlier patches I didn't like the look of using DOMDocument to rewrite the entire output, but as DOMDocument is now known to be buggy for our case, we have no PHP HTML5 parser available, and we have no way of injecting the class when the string is initially built, I see no other way to handle this except for substring manipulation and/or regular expressions.
Comment #234
catchTo some extent I'd be tempted to leave this to #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 since we'll need a fix there anyway.
However, DOMDocument::saveHtml() is really ropey, not just with HTML5 so not using it at least to write out seems worth doing in general.
Comment #235
larowlanI've heard good things about http://masterminds.github.io/html5-php/, it has composer support, perhaps worth a look?
Comment #236
Wim LeersSo… larowlan and Crell (at #1333730-55: [Meta] PHP DOM (libxml2) misinterprets HTML5) think we should use the user-space HTML5 parser, longwave appears to favor string manipulation (though only because we don't have a HTML5 parser handy) and catch is somewhere in between.
That leaves me with the impression that we actually do want to use
html5-php
?Comment #237
longwaveI have no knowledge of html5-php, but it looks like a viable solution to this problem and more reliable than string manipulation - but what would be the performance impact of using it? Parsing and re-serializing an entire HTML document from pure PHP feels like it would be inherently slow, but perhaps I am wrong.
Comment #238
Wim Leers#237: It would not affect all of the HTML: only the "content" part of the HTML, which is the majority of the HTML though, so your concerns *do* apply.
Test coverage
I've now written a unit test to ensure that the code I wrote in #226/#229 yields correct results, in all cases: multi-byte strings, HTML5 markup, content with both UTF-8 and HTML entities for the same symbols does not change either, and so on.
In short: <350 LoC for the unit test, but it generates 2808 assertions. If you can think of more edge cases to be covered, feel free to suggest them, but unless I made glaring mistakes, this should give us sufficient guarantees that this string manipulation-based implementation works correctly.
Performance
I've also done some
microtime(TRUE)
-based profiling. My current implementation takes 1–3 milliseconds on my very much non-optimized dev env. Typically 2 ms. Adding to that the fact that this will only be done for anonymous users, whose pages should be cached, this should be acceptable.I've compared this to the previous
DOMDocument
-based implementation. that took 2–7 milliseconds on the same site/dev env. Typically 3.5ms. And that was using the built-inDOMDocument
, not a user-space HTML5 parser! It seems rather unlikely thathtml5-php
could deliver acceptable performance then. Which means that until PHP ships with a high-performance HTML5 parser, this is the solution we'll have to stick with.Attached is the updated patch with unit test coverage, and a text file that lists all the currently included test cases.
Comment #240
Wim Leers#1862202: Objectify the language system was committed and broke this patch. #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses was reverted and also broke this patch.
Reroll.
Comment #242
Wim LeersOne silly thing I forgot to change to adapt to the API changes made by #1862202: Objectify the language system. That fixes the thousands of exceptions and some of the fails.
Oddly, it seems something has changed subtly which requires another change. And in looking into that, I found one dead line of code, so I removed that.
Comment #245
webchick...
Comment #246
webchickFor the next re-roll, apparently this patch broke the back to site link the last time, so maybe factor #2167611: Back to site button broken in as well.
Comment #247
Wim LeersSomething had indeed subtly changed, but I fixed it incorrectly (I'd even say: incredibly moronically). The subtle change was that instead of a single way of generating the language switching links, there are now multiple, and there's a new level of indirection.
Fixed. That should fix the last test failure.
#246: Until that patch is reverted, this patch can't do anything. Right now, HEAD is already "prepared" for the changes that will be introduced in this patch: the changes that this patch should have to apply, are already made, precisely because that patch is not yet reverted.
Tagging to avoid commit conflicts.
Comment #248
nod_Last little problem:
Before
<li class="menu-99 active"><a href="/" class="active">Home</a></li>
After
<li class="menu-99 active" data-drupal-link-system-path="<front>"><a href="/">Home</a></li>
a tag missing active class.
I've looked at the HTML processing mess, couldn't find anything wrong with it. Beside this last issue it's RTBC for me.
Comment #249
nod_#2167611: Back to site button broken committed.
Need to add this to the current patch:
Let's not break the back to site button a third time :)
Comment #250
Wim LeersUnfortunately no test coverage for that elementary functionality. But: great catch, nod_. You're the first to notice this, even though this problem dates back to #110, from before I started working on this in November 2013 :)
In this reroll:
$variables['set_active_class']
on the$variables
passed totheme_links()
will now automatically also set the active class on all contained links. That simplifies things.Since it's my birthday, I beg the testbotgods for zero failures and exceptions and hope that my fellow community members think this one is finally ready to be worthy of the
Drupal godscore committers' attention again, so that this patch may be united with Drupal core.Comment #252
webchickSniff. No birthday candles for Wim. :(
Comment #253
longwaveFailure seems unrelated: "Table 'drupaltestbotmysql.simpletest219785key_value' doesn't exist", and in a test that is not touched by this patch.
250: active_link-1979468-250.patch queued for re-testing.
Happy birthday Wim!
Comment #254
webchickOops. :) Back to needs review to kick testbot again.
Comment #255
Wim LeersDidn't apply anymore due to a change in
core.services.yml
. The only change in this reroll is manually replacing a single line in the patch (- { name: persist }
→- [setContext, ['@?router.request_context']]
).Comment #256
nod_Everything still works fine, the active class problem was solved. There are no JS errors when doing Ajax stuff and the back to site button is not broken :)
Comment #257
nod_On closer inspection, there were a few
drupalSettings.basePath
that were not replaced. Updated those.Comment #258
catchGetting this in again, before it breaks again (again). Committed/pushed to 8.x, thanks!
Comment #259
Wim LeersTHANK YOU!
/me rejoices :)
Comment #260
thedavidmeister CreditAttribution: thedavidmeister commentedwow, fixed! amazing!