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:

  1. 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.
  2. 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.

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.

Files: 
CommentFileSizeAuthor
#257 interdiff.txt2.34 KBnod_
#257 core-active-class-1979468-257.patch103.66 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 62,984 pass(es). View
#238 testcases.txt621.27 KBWim Leers

Comments

andypost’s picture

I case different roles markup would be different. Also active trail could depend on other conditions so menus could have different render

catch’s picture

Roles 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().

nod_’s picture

Issue tags: +JavaScript
FileSize
5.12 KB
FAILED: [[SimpleTest]]: [MySQL] 55,482 pass(es), 11 fail(s), and 2 exception(s). View

initial patch, doesn't work when page has aliases and probably not with l18n either.

webchick’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, core-js-active-class-1979468-1.patch, failed testing.

thedavidmeister’s picture

I'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.

attiks’s picture

I 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

moshe weitzman’s picture

#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.

attiks’s picture

#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:

  • fewer http requests
  • no parsing / compiling of js

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

catch’s picture

I think we could make this optional too - I'm sure there's plenty of sites that don't use active class anyway.

thedavidmeister’s picture

#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.

webchick’s picture

Issue tags: +Usability

Well, 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.

thedavidmeister’s picture

#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?

catch’s picture

@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.

attiks’s picture

#14 I think limiting it to menu's is reasonable and moving the logic out of l() is a big + as well

thedavidmeister’s picture

#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

catch’s picture

Title: Add active class to links via js instead of directly in l() » Stop adding the active class in l()

Re-titling. thedavidmeister's list looks like a good one to me.

thedavidmeister’s picture

I 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.

nod_’s picture

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 be data-drupal-links or something.

thedavidmeister’s picture

#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 :)

nod_’s picture

Not on the links themselves, on the container having the links to check for.

thedavidmeister’s picture

#21 - why not on the link itself?

thedavidmeister’s picture

@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?

thedavidmeister’s picture

Hey, 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.

catch’s picture

Issue tags: +D8 cacheability

Tagging.

thedavidmeister’s picture

I 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.

thedavidmeister’s picture

nod_’s picture

proper attribute for that is hreflang. Which we should probably have added a long time ago anyway :)

thedavidmeister’s picture

@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.

thedavidmeister’s picture

@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 :)

pwolanin’s picture

Title: Stop adding the active class in l() » Stop adding the active class in l() and instead add it in JS

clarify title

pwolanin’s picture

beejeebus’s picture

i 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').

thedavidmeister’s picture

So, 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.

nod_’s picture

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?

thedavidmeister’s picture

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.

kk, obviously a miscommunication in chat.

Since we don't have ?q=
that use case should be marginal no?

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.

thedavidmeister’s picture

some ideas/discussion for "lite" ways to parse the query string without libraries - http://stackoverflow.com/questions/901115/how-can-i-get-query-string-val...

thedavidmeister’s picture

thedavidmeister’s picture

Assigned: Unassigned » thedavidmeister

I'm going to see if i can make a little progress on this.

thedavidmeister’s picture

Assigned: Unassigned » thedavidmeister
FileSize
6.05 KB
FAILED: [[SimpleTest]]: [MySQL] 57,894 pass(es), 89 fail(s), and 10 exception(s). View

This 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.

thedavidmeister’s picture

Assigned: thedavidmeister » Unassigned
Status: Needs work » Needs review
thedavidmeister’s picture

Assigned: thedavidmeister » Unassigned
Issue tags: +Needs profiling

We should profile the changes made to l() while we're at it.

Status: Needs review » Needs work

The last submitted patch, 1979468-40.patch, failed testing.

dawehner’s picture

@@ -1321,35 +1321,17 @@ function l($text, $path, array $options = array()) {
+    $variables['options']['attributes']['data-drupal-link-system-path'] = Drupal::service('path.alias_manager')->getSystemPath($path);

This should be 'path.alias_manager.cached'

thedavidmeister’s picture

@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().

nod_’s picture

Status: Needs work » Needs review
Issue tags: +ie8
FileSize
9.39 KB
7.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,952 pass(es), 90 fail(s), and 10 exception(s). View

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:

[
  "[data-drupal-link-system-path="node"]:not([hreflang]):not([data-drupal-link-query])", 
  "[data-drupal-link-system-path="<front>"]:not([hreflang]):not([data-drupal-link-query])", 
  "[data-drupal-link-system-path="node"][hreflang="en"]:not([data-drupal-link-query])", 
  "[data-drupal-link-system-path="<front>"][hreflang="en"]:not([data-drupal-link-query])"
]

And with a query string:

[
  "[data-drupal-link-system-path="node"]:not([hreflang])[data-drupal-link-query='{"page":"1"}']", 
  "[data-drupal-link-system-path="<front>"]:not([hreflang])[data-drupal-link-query='{"page":"1"}']", 
  "[data-drupal-link-system-path="node"][hreflang="en"][data-drupal-link-query='{"page":"1"}']", 
  "[data-drupal-link-system-path="<front>"][hreflang="en"][data-drupal-link-query='{"page":"1"}']"
]

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.

thedavidmeister’s picture

Unless 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.

thedavidmeister’s picture

Status: Needs review » Needs work

I 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.

nod_’s picture

Same thing is done in PHP, in the current l function:

($variables['options']['query'] == $active['query'])

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 of classList in the JS (easy to address).

nod_’s picture

Also, this patch needs #1691394: Drupal settings gets broken by AJAX requests to work properly with ajax requests.

thedavidmeister’s picture

and I asked on IRC, that's sensitive to key order too, so it's nothing new.

No, 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().

I would disagree to work with existing data because it's just too slow.

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".

nod_’s picture

Status: Needs work » Needs review
FileSize
7.51 KB
FAILED: [[SimpleTest]]: [MySQL] 57,720 pass(es), 90 fail(s), and 10 exception(s). View

Is it really that slow?

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.

thedavidmeister’s picture

Status: Needs review » Needs work

This is starting to look pretty good I think :)

+  // Add a data-drupal-link-type attribute to let JavaScript know this link is
+  // used for navigation (as opposed to?) and so is potentially eligible for an
+  // active class.

This comment is now wrong.

+  if (!empty($variables['options']['query'])) {
+    $query = $variables['options']['query'];
+    ksort($query);
+    $variables['options']['attributes']['data-drupal-link-query'] = Json::encode($query);
   }

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.

+          $current_query = Drupal::service('request')->query->all();
           url('', array('script' => &$scriptPath, 'prefix' => &$pathPrefix));
-          $javascript['settings']['data'][] = array(
+          $path = array(
             'basePath' => base_path(),
             'scriptPath' => $scriptPath,
             'pathPrefix' => $pathPrefix,
             'currentPath' => current_path(),
+            'isFront' => drupal_is_front_page(),
+            'currentLanguage' => Drupal::languageManager()->getLanguage(Language::TYPE_URL)->id,
           );
+          if (!empty($current_query)) {
+            ksort($current_query);
+            $path['currentQuery'] = (object) $current_query;
+          }
+          $javascript['settings']['data'][] = array('path' => $path);

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.

-      Drupal.attachBehaviors(new_content, settings);
+      Drupal.attachBehaviors(new_content[0], settings);

What is this doing?

thedavidmeister’s picture

But that'd make us loop over the elements twice: first to filter by checking the href attribute, then to add the active class.

Since checking the query is the last thing we do, we could add the active class in the same loop if it helped.

thedavidmeister’s picture

also, we still need to delete all the PHP tests that no longer make sense.

nod_’s picture

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's drupalSettings.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.

thedavidmeister’s picture

it's drupalSettings.path now :)

mind. blown.

nod_’s picture

attiks’s picture

So 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?

thedavidmeister’s picture

So 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?

The 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?

Will the option discussed in #24 to disable this be added as well?

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)

attiks’s picture

#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.

thedavidmeister’s picture

So this means every visitor has to do at least 1 extra http request to fetch the javascript

For pages that could live without javascript before, this will mean an extra http request

What 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.

unless I'm missing something.

What you're missing is that l() now is alterable so of course you can add back in the '.active' class logic in contrib.

Drupal out-of-the-box should be able to solve this

@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.

attiks’s picture

#62 Point taken and I didn't intend to keep repeating myself.

The 'problem' with forcing the use of javascript to solve this are:

  • the extra request.
  • the request will be blocking other requests.
  • once the classes are added, some parts of the screen will need a repaint, and worst case might trigger a reflow as well.

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:

  • Navigation menus: depending on the use case this might be completely the same for all pages (eg. site with a single level main menu), or almost page specific (multi level menu)
  • Pagination: page specific since the content changes for every page
  • Tabs: page specific because the links of the tab will be page specific (eg. edit on a node page)
  • Breadcrumbs: mostly page specific

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.

nod_’s picture

Title: Stop adding the active class in l() and instead add it in JS » Stop adding the active class in l()
Status: Needs work » Needs review
Issue tags: -JavaScript
FileSize
4 KB
FAILED: [[SimpleTest]]: [MySQL] 57,993 pass(es), 99 fail(s), and 10 exception(s). View

Or

Status: Needs review » Needs work

The last submitted patch, core-active-class-1979468-64.patch, failed testing.

webchick’s picture

Please 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.

Damien Tournoud’s picture

To 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:

<a href="http://drupal.org/handbook" data-page-id="ab9fcafdb8">Handbook</a>

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>):

<body data-page-id="ab9fcafdb8">

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.).

jibran’s picture

I think #67 is good approach.

thedavidmeister’s picture

The 'problem' with forcing the use of javascript to solve this are:

the extra request.
the request will be blocking other requests.
once the classes are added, some parts of the screen will need a repaint, and worst case might trigger a reflow as well.
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.

100-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?

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.

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.

To 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.

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.

thedavidmeister’s picture

Title: Stop adding the active class in l() » The active class being added by l() forces an upper limit of per-page caching for all content containing links
attiks’s picture

#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

thedavidmeister’s picture

#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.

Damien Tournoud’s picture

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.

The 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.

catch’s picture

@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.

thedavidmeister’s picture

The 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.

Actually, 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.

Damien Tournoud’s picture

@thedavidmeister: actually, the default language is always used when not passed:

    // Language can be passed as an option, or we go for current URL language.
    if (!isset($options['language'])) {
      $language_url = $this->languageManager->getLanguage(Language::TYPE_URL);
      $options['language'] = $language_url;
    }

(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.

thedavidmeister’s picture

Even if there's a 200ms client-side regression here

There'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.

nod_’s picture

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 using performance.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.

attiks’s picture

#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.

attiks’s picture

#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?

thedavidmeister’s picture

#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.

Damien Tournoud’s picture

#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.

Yes. All URLs we generate have a language, even if there is no hreflang.

thedavidmeister’s picture

Yes. All URLs we generate have a language, even if there is no hreflang.

l() doesn't see what url() is doing internally though, it would have to figure out the default language on its own.

attiks’s picture

#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:

  • Drupalorg - not used/needed on this page
  • collapse - not used/needed on this page
  • drupalorgCompany - not used/needed on this page
  • drupalorgMarketplace - not used/needed on this page
  • drupalorgSearch - used on this page for the search box at the top, but in reality it is just a nice to have
  • drupalorgSetHome - not used/needed on this page
  • html5UserGeolocation - not used/needed on this page
  • tableHeader - not used/needed on this page
  • It uses the GA module, so it needs access to the drupal settings.

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

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?

It includes everything.

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

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.

jcisio’s picture

Patch #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.

dawehner’s picture

I 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.

thedavidmeister’s picture

Ok, 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.

tstoeckler’s picture

Per #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.

thedavidmeister’s picture

#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/

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,173 pass(es), 16 fail(s), and 1 exception(s). View

A rough proof of concept patch for #88. Only setting needs review for the testbots.

Status: Needs review » Needs work

The last submitted patch, 1979468-90.patch, failed testing.

JohnAlbin’s picture

I'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:

[data-drupal-link-system-path="user/1"]:not([hreflang]):not([data-drupal-link-query]),
[data-drupal-link-system-path="user/1"][hreflang="en"]:not([data-drupal-link-query]) {
  outline: 5px solid blue;
}

(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.

thedavidmeister’s picture

I'm not sure why no one reviewed Nod's patch in #64.

I 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

thedavidmeister’s picture

or go back to the D6-style of just applying the active class to menu links

Have a look at building on #90 for this.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.14 KB
12.04 KB
FAILED: [[SimpleTest]]: [MySQL] 57,842 pass(es), 4 fail(s), and 0 exception(s). View

I 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.

nod_’s picture

Status: Needs review » Needs work
nod_’s picture

Status: Needs work » Needs review

Sorry didn't mean to change status.

Status: Needs review » Needs work

The last submitted patch, 1979468-95.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1979468-99.patch. Unable to apply patch. See the log in the details link for more information. View

Should fix the other test fails.

pwolanin’s picture

jessebeach’s picture

There'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:

  1. Requiring JavaScript to identify the active trail links for a non-authenticated user is not ideal.
  2. Determining the active trail through a menu tree on the server side results in an unacceptable cacheing profile (potentially per-page for a menu entity).
  3. We can represent the current page path in a hashed identifier, represent each menu link href as a hashed identifier, and compare these values to find active links, then trace up the DOM to mark the active trail. (#67)
  4. Include the JavaScript that finds the active item and marks the active trail as a library. This library can be knocked out by a module with 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?

attiks’s picture

#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.

Gábor Hojtsy’s picture

@attiks: indeed the page agnostic caching/rendering is happening in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

thedavidmeister’s picture

#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 >.<

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 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 :)

jessebeach’s picture

#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'm looking into this approach. I agree that the admin menu is a special flower and might warrant a singular approach.

thedavidmeister’s picture

So we now plan to split the active class handling between l() and js?

jessebeach’s picture

No, 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.

pwolanin’s picture

Status: Needs review » Needs work
Issue tags: +Usability, +Needs issue summary update, +ie8, +Needs profiling, +D8 cacheability

The last submitted patch, 1979468-99.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1979468-110.patch. Unable to apply patch. See the log in the details link for more information. View

re-roll for conflict in ImageFieldDisplayTest

thedavidmeister’s picture

Title: The active class being added by l() forces an upper limit of per-page caching for all content containing links » The active class being added by l() and theme_links() forces an upper limit of per-page caching for all content containing links
Status: Needs review » Needs work

There'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.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -Needs issue summary update, -ie8, -Needs profiling, -D8 cacheability

#110: 1979468-110.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +Needs issue summary update, +ie8, +Needs profiling, +D8 cacheability

The last submitted patch, 1979468-110.patch, failed testing.

moshe weitzman’s picture

Perhaps someone can update the Issue Summary? Kind of hard to know where this one is at. Thanks.

thedavidmeister’s picture

Hey, 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.

jwilson3’s picture

putting the active class in js allows you to put <a href="/some/url">foo</a> in your Twig templates.

That'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?

thedavidmeister’s picture

#116 - I assume the logic would stay the same as it is server-side, at least initially. Which is... fragments do nothing...

thedavidmeister’s picture

Title: The active class being added by l() and theme_links() forces an upper limit of per-page caching for all content containing links » ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links
Priority: Normal » Major

Bumping the priority of this in light of profiling results like https://drupal.org/node/2102777#comment-8003157

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

I had a go at updating the issue summary, trying to incorporate all the relevant discussion from the thread here.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Here'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

Run #526bd141c9fe0 Run #526bd15a33d10 Diff Diff%
Number of Function Calls 10,852 194,833 183,981 1695.4%
Incl. Wall Time (microsec) 62,489 1,239,675 1,177,186 1883.8%
Incl. CPU (microsecs) 61,801 1,156,017 1,094,216 1770.5%
Incl. MemUse (bytes) 1,235,488 4,921,912 3,686,424 298.4%
Incl. PeakMemUse (bytes) 1,225,976 5,064,920 3,838,944 313.1%

Without "is active" logic server-side, converting theme_links() to use routes - https://drupal.org/node/2102777#comment-8004867

Run #526bd141c9fe0 Run #526cf82f1fd24 Diff Diff%
Number of Function Calls 10,852 11,216 364 3.4%
Incl. Wall Time (microsec) 62,489 63,349 860 1.4%
Incl. CPU (microsecs) 61,801 62,600 799 1.3%
Incl. MemUse (bytes) 1,235,488 1,386,944 151,456 12.3%
Incl. PeakMemUse (bytes) 1,225,976 1,377,736 151,760 12.4%

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.

thedavidmeister’s picture

minor update: the profiling in #120 was for 200 links.

thedavidmeister’s picture

nm, 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.

Wim Leers’s picture

#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:

  1. Use the PHP solution for anonymous users, which means most no-JS sites won't be affected at all. As long as you're using the page cache (you should) or Varnish/nginx/… (even better), then this won't affect your cache hit ratios at all.
  2. Use the JS solution for authenticated users, which means we can maximally leverage the render cache for these much more expensive page loads.
pounard’s picture

#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).

pounard’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

@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?

Crell’s picture

While 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.

Wim Leers’s picture

#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 :)

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +JavaScript, +Performance, +Spark, +sprint

As #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:

  • Of course: l() will now add data- attributes for authenticated users and generate the "active" class for anonymous users — as proposed in #123.
  • The #64 patch used to add its data- attributes for every invocation of l(). This patch takes advantage of the work done by #110 to only add them if set_active_class === TRUE.
  • The #64 patch used to piggyback its JS on the drupal library and assumed that would always be loaded, which is not true. This patch updates system_page_build() to add the new drupal.active-link library if the current user is authenticated. It must happen in a stand-alone hook_page_build() implementation simply because l() does not go through drupal_render(), so we can't #attach anything inside l().
Wim Leers’s picture

FileSize
20.43 KB
FAILED: [[SimpleTest]]: [MySQL] 59,702 pass(es), 7 fail(s), and 1 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 129: active_link-1979468-128.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
30.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch active_link-1979468-131.patch. Unable to apply patch. See the log in the details link for more information. View
10.78 KB

This should make all tests pass.

catch’s picture

The different logic for anon vs. auth works for me.

simply because l() does not go through drupal_render(), so we can't #attach anything inside l()

While hook_page_build() is OK for now I think could we potentially do something with #type => 'link'?

Wim Leers’s picture

#132: Two questions:

  1. Wouldn't we then make every l() call much slower then? I don't want to make claims without data to back it up though
  2. Isn't getting rid of l() and requiring developers to always create a render array and call drupal_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?

moshe weitzman’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community

I think that this is a good stopping point and work can go into follow-ups

larowlan’s picture

131: active_link-1979468-131.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 131: active_link-1979468-131.patch, failed testing.

dawehner’s picture

So 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?

larowlan’s picture

FileSize
30.02 KB
FAILED: [[SimpleTest]]: [MySQL] 59,070 pass(es), 1 fail(s), and 0 exception(s). View

No longer applies, re-roll

larowlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 138: active-class-1979468.136.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

#137: Sigh. You're right :( I guess we all forgot about the mess that link generation is. I'll try to reroll ASAP.

Status: Needs review » Needs work

The last submitted patch, 138: active-class-1979468.136.patch, failed testing.

dawehner’s picture

#137: Sigh. You're right :( I guess we all forgot about the mess that link generation is. I'll try to reroll ASAP.

There 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.

Wim Leers’s picture

Assigned: catch » Wim Leers
Status: Needs work » Needs review
FileSize
54.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch active_link-1979468-145.patch. Unable to apply patch. See the log in the details link for more information. View
25.54 KB

This reroll adds the same $options['set_active_class'] support to LinkGenerator::generate() as it did previous to l().

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.

The last submitted patch, 145: active_link-1979468-145.patch, failed testing.

dawehner’s picture

  1. +++ b/core/core.services.yml
    @@ -270,7 +270,7 @@ services:
    +    arguments: ['@url_generator', '@module_handler', '@language_manager', '@path.alias_manager.cached', '@current_user']
    

    Let's inject the current user via a setter so we can synchronize it.

  2. +++ b/core/includes/common.inc
    @@ -2214,13 +2249,21 @@ function drupal_add_js($data = NULL, $options = NULL) {
    +          $current_query = Drupal::service('request')->query->all();
    ...
    +            'currentLanguage' => Drupal::languageManager()->getLanguage(Language::TYPE_URL)->id,
    

    We always use a "\" at the beginning.

  3. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -50,6 +53,20 @@ class LinkGenerator implements LinkGeneratorInterface {
    +   * The current user service.
    

    Let's not call that a service.

  4. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
    @@ -416,6 +459,175 @@ public function testGenerateActive() {
    +  public function testGenerateActiveAuthenticated() {
    

    It seems to be that this test could be simplified by using a dataProvider.

Wim Leers’s picture

FileSize
54.35 KB
FAILED: [[SimpleTest]]: [MySQL] 59,026 pass(es), 25 fail(s), and 9,318 exception(s). View

HEAD changed; straight reroll.

Status: Needs review » Needs work

The last submitted patch, 148: active_link-1979468-147.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
53.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,142 pass(es). View
20.66 KB

Apparently 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:

  1. Done.
  2. Done.
  3. It's called a service in 9 other locations in Drupal core. Let's avoid a bikeshed over something as insignificant as that.
  4. Done.

Hopefully this will come back green.

Wim Leers’s picture

dawehner’s picture

+      - [setCurrentUser, ['@?current_user']]

You need an "=" at the end to set it to be synchronized.

It's called a service in 9 other locations in Drupal core. Let's avoid a bikeshed over something as insignificant as that.

Yeah, let's go with the least currently used alternative, so we have more standards :P

  1. +++ b/core/includes/common.inc
    @@ -2214,6 +2249,7 @@ function drupal_add_js($data = NULL, $options = NULL) {
    +          $current_query = Drupal::service('request')->query->all();
    

    Should be "\"

  2. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -110,30 +149,51 @@ public function generate($text, $route_name, array $parameters = array(), array
    +          $variables['options']['attributes']['data-drupal-link-system-path'] = $this->aliasManager->getSystemPath($path);
    

    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?

  3. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
    @@ -55,6 +55,52 @@ function testLanguageBlock() {
    +    /**
    +     * For authenticated users, the "active" class is set by JavaScript.
    +     */
    ...
    +    /**
    +     * For anonymous users, the "active" class is set by PHP.
    +     */
    
    @@ -105,6 +151,67 @@ function testLanguageLinkActiveClass() {
    +    /**
    +     * For authenticated users, the "active" class is set by JavaScript.
    +     */
    

    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?

  4. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
    @@ -303,39 +319,119 @@ public function testGenerateWithHtml() {
    +   */
    +  public function providerTestGenerateActive() {
    

    <3 <3 <3 <3

Wim Leers’s picture

FileSize
55.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,275 pass(es). View
4.91 KB
  1. Done. Also in two other places in the patch.
  2. Since this didn't consider routes before you noticed that that was missing, that hasn't been discussed :)
    The reason is simple: lowest common denominator. A (route, parameters) pair can be converted into a path, 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 with data-drupal-link-route and data-drupal-link-route-parameters.
  3. Now the anon vs. auth parts of the test functions are split into separate helper functions, as you requested.
  4. :)
Wim Leers’s picture

Priority: Major » Critical

Blocks #2151459: Enable node render caching, which is critical, hence marking this critical.

xjm’s picture

Issue tags: +beta blocker

Beta blocker as a blocker for #2151459: Enable node render caching.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I propose we close the critical, and deal with 2) in a follow-up

webchick’s picture

Assigned: Wim Leers » catch

/me tosses this one to catch

thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs work

Dare 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.

Wim Leers’s picture

Assigned: catch » Wim Leers

#158: ARGH! What's with this duplication of the same logic in not 2, but 3 places!?

Working on a reroll.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
70.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,464 pass(es), 2 fail(s), and 0 exception(s). View
18.25 KB

I probably spent about 9 hours on this today, to get theme_links() support done, including test coverage.

Notes:

  1. I've had to re-read all of #2102777: Allow theme_links to use routes as well as href because nowhere did it explain why we needed the "active" class to live on <li> elements rather than the <a> child elements, because in the latter case theme_links() could just reuse the existing logic in l() and LinkGenerator::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 :)
  2. The existing test coverage for theme_links() is weak: there's only \Drupal\system\Tests\Theme\FunctionsTest::testLinks() and that's hardly proper test coverage: it's a WebTestBase that should at least be DrupalUnitTestBase. However, improving that is out of scope for this issue. The updated patch provides sufficient test coverage.
  3. Also worth noting: when we eventually get rid of 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 (once l() is gone) simpler.
  4. This reroll effectively means that it needs to be possible to have 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.

The last submitted patch, 160: active_link-1979468-160.patch, failed testing.

Wim Leers’s picture

FileSize
71.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,430 pass(es). View
6.11 KB

The two fails both occurred in LanguageSwitchingTest: the language switching block uses theme_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.

Wim Leers’s picture

The last submitted patch, 162: active_link-1979468-162.patch, failed testing.

Wim Leers’s picture

162: active_link-1979468-162.patch queued for re-testing.

The last submitted patch, 162: active_link-1979468-162.patch, failed testing.

Wim Leers’s picture

Either HEAD or testbot is broken ATM.

Wim Leers’s picture

162: active_link-1979468-162.patch queued for re-testing.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

The theme_links() changes are solid. Back to RTBC.

webchick’s picture

Assigned: Wim Leers » catch

This issue feels catch-ish.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/common.inc
@@ -1214,12 +1214,25 @@ function drupal_http_header_attributes(array $attributes = array()) {
+ *     link is "active", and if so, apply an "active" class to the link. It is
+ *     important to use this sparingly as any content containing a link
+ *     generated by l() with active class processing enabled is incompatible
+ *     with render caching elements on more than a per-page basis.
+ *     For anonymous users, the "active" class will be calculated on the server,
+ *     because most sites serve each anonymous user the same cached page anyway.
+ *     For authenticated users, the "active" class will be calculated on the
+ *     client (through JavaScript), only data- attributes are added to links to
+ *     prevent breaking the render cache. The JavaScript is added in

This 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.

Wim Leers’s picture

  1. I'll fix the comment. It's indeed "sparingly anyway".
  2. Good point. This indeed assumes render caching happens only at the page level, which is of course wrong.
    After 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.

Wim Leers’s picture

Assigned: catch » Wim Leers
Status: Needs work » Needs review
FileSize
62.27 KB
FAILED: [[SimpleTest]]: [MySQL] 59,600 pass(es), 15 fail(s), and 30 exception(s). View
39.22 KB
  1. Fixed.
  2. l(), LinkGenerator::generate() and theme_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() and theme_links(), we now just always do the same simple thing: we add data-drupal-link-* attributes. Then, in system_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 simple hook_page_alter(), to either:
    1. always use the JS solution
    2. always use the PHP solution
    3. never apply any "active" classes and gain some back-end or front-end performance in the process

    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 incorrect Drupal.(attach|detach)Behaviors calls, which caused the JS added in this page to trigger a JS error.

Crell’s picture

All 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?

The last submitted patch, 173: active_link-1979468-173.patch, failed testing.

Wim Leers’s picture

FileSize
66.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch active_link-1979468-176.patch. Unable to apply patch. See the log in the details link for more information. View
7.37 KB

Rerolled, 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(), but hook_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 use hook_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.

catch’s picture

The 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).

The last submitted patch, 176: active_link-1979468-176.patch, failed testing.

Wim Leers’s picture

The sole exception that caused the patch to come back red:

Symfony\Component\DependencyInjection\Exception\RuntimeException: You have requested a synthetic service ("request"). The DIC does not know how to construct this service. in Symfony\Component\DependencyInjection\ContainerBuilder->createService() (line 922 of /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'request')
Symfony\Component\DependencyInjection\ContainerBuilder->get('request')
…

That's most definitely unrelated. Let's try again.

Wim Leers’s picture

176: active_link-1979468-176.patch queued for re-testing.

Wim Leers’s picture

176: active_link-1979468-176.patch queued for re-testing.

The last submitted patch, 176: active_link-1979468-176.patch, failed testing.

webchick’s picture

Status: Needs review » Needs work

Lazy testbot.

moshe weitzman’s picture

Interdiff looks good and new test coverage is swell. I will RTBC this once it comes back green.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
65.96 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

Straight reroll.

Wim Leers’s picture

FileSize
65.95 KB
FAILED: [[SimpleTest]]: [MySQL] 59,623 pass(es), 18 fail(s), and 4 exception(s). View
813 bytes

Introduced a small mistake in the straight reroll. Fixing.

The last submitted patch, 185: active_link-1979468-185.patch, failed testing.

The last submitted patch, 186: active_link-1979468-186.patch, failed testing.

Wim Leers’s picture

186: active_link-1979468-186.patch queued for re-testing.

The last submitted patch, 186: active_link-1979468-186.patch, failed testing.

Wim Leers’s picture

FileSize
64.79 KB
PASSED: [[SimpleTest]]: [MySQL] 59,633 pass(es). View
2.4 KB

Damn, 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 in SystemController::setLinkActiveClass() could be removed. It also means that the changes to DefaultMobileMetaTagsTest could be reverted, making this patch a little bit smaller.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

webchick’s picture

Assigned: Wim Leers » catch

Wicked!!

Since this is the last blocker to node render caching, seems like it's fitting for catch to do the honours here. :)

catch’s picture

Title: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links » Change notice: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Choirs sing etc.

Committed/pushed to 8.x, thanks!

Needs a short change notice for the change to l().

Wim Leers’s picture

Title: Change notice: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links » ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links
Assigned: catch » Wim Leers
Priority: Major » Critical
Status: Active » Fixed

Change notice created: https://drupal.org/node/2167077.

Wim Leers’s picture

Issue tags: -sprint, -Needs change record
jibran’s picture

Status: Fixed » Reviewed & tested by the community

Not pushed yet.

jibran’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.x 624296e.

nod_’s picture

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

nod_’s picture

The change in structure of drupalSettings.path broke back to site button: #2167611: Back to site button broken

ianthomas_uk’s picture

longwave’s picture

This patch also breaks any non-ASCII characters when rewriting the output, as $dom->saveHTML() returns malformed HTML entities when given UTF-8 input.

catch’s picture

Status: Fixed » Needs work

OK three criticals is a bit too much, I've rolled this back for now.

nod_’s picture

Status: Needs work » Needs review
FileSize
76.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,394 pass(es). View

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

longwave’s picture

FileSize
77.24 KB
FAILED: [[SimpleTest]]: [MySQL] 59,320 pass(es), 1 fail(s), and 0 exception(s). View
997 bytes

This is #204 with an addition to demonstrate the UTF-8 bug I mentioned in #202.

The last submitted patch, 205: core-js-active-links-1979468-205.patch, failed testing.

jessebeach’s picture

Status: Needs review » Needs work
FileSize
16.57 KB

The original patch was not rolled back yet in 8.x. Here's the patch from #205 on top of HEAD (c241c1ba).

jessebeach’s picture

Derpy derp. Ignore #207. Apparently "pull" is merely a suggestion to my version of git :/ I see it was reverted in e4124f0a.

longwave’s picture

205: 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.

The last submitted patch, 205: core-js-active-links-1979468-205.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
79.7 KB
FAILED: [[SimpleTest]]: [MySQL] 59,778 pass(es), 1 fail(s), and 0 exception(s). View
1.37 KB

Turns out the solution was much simpler than expected :)

nod_’s picture

Status: Needs review » Reviewed & tested by the community

That's RTBC if it comes back green.

webchick’s picture

Assigned: Wim Leers » catch

Back to catch-o-rama

The last submitted patch, 211: active_link-1979468-211.patch, failed testing.

webchick’s picture

Assigned: catch » Wim Leers
Status: Reviewed & tested by the community » Needs work

Heh, nevermind. :)

The failure's in JavaScriptTest which sounds like it might be legit.

longwave’s picture

Status: Needs work » Needs review
FileSize
77.47 KB
PASSED: [[SimpleTest]]: [MySQL] 59,833 pass(es). View
1.23 KB

drupal_add_js() => _drupal_add_js()

Wim Leers’s picture

#216: D'oh! Thanks for the reroll :)

nod_’s picture

Status: Needs review » Reviewed & tested by the community

All right, Green.

webchick’s picture

Assigned: Wim Leers » catch

One more time. :)

catch’s picture

Assigned: catch » Unassigned

This 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.

catch’s picture

Assigned: Unassigned » Dries
Wim Leers’s picture

Extra 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).

longwave’s picture

Does this have the same concerns as #1333730: [Meta] PHP DOM (libxml2) only understands XHTML4, misinterprets HTML5, but D8 must cope with HTML5, as it also uses DOMDocument to parse and rewrite the entire markup?

Wim Leers’s picture

FileSize
79.27 KB
PASSED: [[SimpleTest]]: [MySQL] 60,021 pass(es). View
webchick’s picture

Oops, sorry! Too hasty on the assigned-to field. :) Dries should be able to look at this on Friday.

Wim Leers’s picture

Assigned: Dries » Wim Leers
Status: Reviewed & tested by the community » Needs review
FileSize
81.12 KB
FAILED: [[SimpleTest]]: [MySQL] 60,045 pass(es), 11 fail(s), and 0 exception(s). View
6.25 KB

#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) only understands XHTML4, misinterprets HTML5, but D8 must cope with 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:

  1. As it is implemented in the patches above: use DOMDocument to find and update the affected HTML elements.
  2. Use DOMDocument to find the affected HTML elements, call $node->C14N() on them to get their HTML representations and then use str_replace() to replace them in the HTML string — that way we avoid calling DomDocument::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 use DOMDocumentto find matches and then use string manipulation to update the HTML.
  3. Use string matching (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="&lt;front&gt;">
  4. 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 using DOMDocument::saveHTML(). Since we're then using DOMDocument 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.

The last submitted patch, 226: active_link-1979468-226.patch, failed testing.

dawehner’s picture

  1. +++ b/core/includes/common.inc
    @@ -1214,12 +1214,24 @@ function drupal_http_header_attributes(array $attributes = array()) {
    + *   - 'set_active_class' (default FALSE): Whether l() should compare the $path,
    

    There are MANY examples in core which uses "Defaults to FALSE." instead of using parenthesis

  2. +++ b/core/includes/theme.inc
    @@ -1203,6 +1215,19 @@ function template_preprocess_status_messages(&$variables) {
    + * @see system_page_build()
    ...
    + * @see l()
    + * @see \Drupal\Core\Utility\LinkGenerator::generate()
    

    I like this cross-linking.

  3. +++ b/core/includes/theme.inc
    @@ -1249,16 +1273,16 @@ function theme_links($variables) {
           // Add odd/even, first, and last classes.
    -      $class[] = ($i % 2 ? 'odd' : 'even');
    +      $li_attributes['class'][] = ($i % 2 ? 'odd' : 'even');
           if ($i == 1) {
    

    Oh I thought we would have killed odd/even from core, as CSS3 should be able to do the same.

  4. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -110,30 +125,31 @@ public function generate($text, $route_name, array $parameters = array(), array
    +        $path = $this->urlGenerator->getPathFromRoute($route_name, $parameters);
    

    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.

  5. +++ b/core/misc/active-link.js
    @@ -0,0 +1,63 @@
    +      for (var i = 0, il = activeLinks.length; i < il; i += 1) {
    

    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.

  6. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -329,4 +330,114 @@ public function themeSetDefault() {
    +      else if ($context['front'] && $pos_front !== FALSE) {
    

    We use elseif, but please tell me why if you know.

  7. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -329,4 +330,114 @@ public function themeSetDefault() {
    +    while ((strpos($element['#markup'], 'data-drupal-link-system-path="' . $context['path'] . '"', $offset) !== FALSE || ($context['front'] && strpos($element['#markup'], 'data-drupal-link-system-path="&lt;front&gt;"', $offset) !== FALSE))) {
    

    I wonder whether we should split this up into multiple lines to make it more readable.

  8. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -329,4 +330,114 @@ public function themeSetDefault() {
    +      $node = $dom->getElementsByTagName('body')->item(0)->firstChild;
    ...
    +      $dom = new \DOMDocument();
    ...
    +      // attributes.
    ...
    +      $dom->loadHTML('<!DOCTYPE html><html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8" /></head><body>' . $tag . '</body></html>');
    

    We certainly should add some performance testing how well this performs.

  9. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -329,4 +330,114 @@ public function themeSetDefault() {
    +  public static function setLinkActiveClass(array $element, array $context) {
    

    This seems to be potentially unit-testable and is certainly complex enough to do so.

Wim Leers’s picture

FileSize
81.63 KB
PASSED: [[SimpleTest]]: [MySQL] 60,006 pass(es). View
6.19 KB

#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.

  1. Okay… fixed that nitpicky nitpick.
  2. Good :)
  3. Not yet, #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors hasn't landed yet.
  4. There are syntax errors in your sentence, but I tried to understand what you meant.
    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 if set_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 the path alias!
    Fourth: generateFromRoute() is on URLGenerator, which we don't touch at all. Hence adding "skip path processing" to that is off-topic.
  5. This JS was written by the JavaScript maintainer for Drupal core, not by me. I'm pretty sure this is intentional.
  6. Nitpicky nitpick fixed.
  7. I'll do that iff this approach is approved (again, see #226).
  8. See bullet 4, second point again: this is rarely called. Also: we parse a HTML document with an empty HEAD and a BODY that contains a single tag. Performance impact must be limited. Finally: this is the only way to do this without resorting to crazy regular expressions or string manipulation that I know of. If you have other ideas to make this work, please share! I don't like it either.
  9. … again, see #226, the very last paragraph.

Reroll fixes the remaining test failures and the nitpicks in #228.

amateescu’s picture

longwave’s picture

+        if ($element['#markup'][$i] === '<') {
+        if ($element['#markup'][$i] === '>') {

I 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?

+      $tag = substr($element['#markup'], $pos_tag_start, $pos_tag_end - $pos_tag_start + 1);

Similarly I think this might need to use drupal_substr() (and drupal_strlen()) to be multibyte-safe.

Wim Leers’s picture

#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?

longwave’s picture

As you described in #226 and as noted in #1333730: [Meta] PHP DOM (libxml2) only understands XHTML4, misinterprets HTML5, but D8 must cope with 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.

catch’s picture

To some extent I'd be tempted to leave this to #1333730: [Meta] PHP DOM (libxml2) only understands XHTML4, misinterprets HTML5, but D8 must cope with 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.

larowlan’s picture

I've heard good things about http://masterminds.github.io/html5-php/, it has composer support, perhaps worth a look?

Wim Leers’s picture

So… larowlan and Crell (at #1333730-55: [Meta] PHP DOM (libxml2) only understands XHTML4, misinterprets HTML5, but D8 must cope with 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?

longwave’s picture

I 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.

Wim Leers’s picture

Issue tags: -Needs profiling
FileSize
101.61 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch active_link-1979468-238.patch. Unable to apply patch. See the log in the details link for more information. View
21.9 KB
621.27 KB

#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-in DOMDocument, not a user-space HTML5 parser! It seems rather unlikely that html5-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.

The last submitted patch, 238: active_link-1979468-238.patch, failed testing.

Wim Leers’s picture

FileSize
102 KB
FAILED: [[SimpleTest]]: [MySQL] 62,748 pass(es), 21 fail(s), and 8,693 exception(s). View

#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.

Status: Needs review » Needs work

The last submitted patch, 240: active_link-1979468-240.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
102.1 KB
FAILED: [[SimpleTest]]: [MySQL] 62,944 pass(es), 1 fail(s), and 0 exception(s). View
1.69 KB

One 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.

The last submitted patch, 242: active_link-1979468-242.patch, failed testing.

The last submitted patch, 242: active_link-1979468-242.patch, failed testing.

webchick’s picture

Status: Needs review » Needs work

...

webchick’s picture

For 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.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Avoid commit conflicts
FileSize
102.74 KB
PASSED: [[SimpleTest]]: [MySQL] 62,959 pass(es). View
1.54 KB

Oddly, it seems something has changed subtly which requires another change.

Something 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.

nod_’s picture

Status: Needs review » Needs work

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="&lt;front&gt;"><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.

nod_’s picture

#2167611: Back to site button broken committed.

Need to add this to the current patch:

diff --git a/core/modules/toolbar/js/escapeAdmin.js b/core/modules/toolbar/js/escapeAdmin.js
index a602766..72a9fb8 100644
--- a/core/modules/toolbar/js/escapeAdmin.js
+++ b/core/modules/toolbar/js/escapeAdmin.js
@@ -7,7 +7,7 @@
 
 "use strict";
 
-var pathInfo = drupalSettings;
+var pathInfo = drupalSettings.path;
 var escapeAdminPath = sessionStorage.getItem('escapeAdminPath');
 
 // Saves the last non-administrative page in the browser to be able to link back

Let's not break the back to site button a third time :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
103.28 KB
PASSED: [[SimpleTest]]: [MySQL] 62,953 pass(es). View
7.64 KB

Unfortunately 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:

  1. Fixed the problem noted by nod_ in #248. Setting $variables['set_active_class'] on the $variables passed to theme_links() will now automatically also set the active class on all contained links. That simplifies things.
  2. Applied the JS changes to make sure the "back to site" button continues to work with this patch committed (and manually tested it to verify).
  3. In further manual testing, I discovered that this patch apparently is also breaking in-place editing (irony FTW), maybe only in recent patches, but that's still not ok of course. Fixed that too.

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 gods core committers' attention again, so that this patch may be united with Drupal core.

The last submitted patch, 250: active_link-1979468-250.patch, failed testing.

webchick’s picture

Status: Needs review » Needs work

Sniff. No birthday candles for Wim. :(

longwave’s picture

Failure 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!

webchick’s picture

Status: Needs work » Needs review

Oops. :) Back to needs review to kick testbot again.

Wim Leers’s picture

FileSize
103.22 KB
PASSED: [[SimpleTest]]: [MySQL] 63,008 pass(es). View

Didn'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']]).

nod_’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

nod_’s picture

FileSize
103.66 KB
PASSED: [[SimpleTest]]: [MySQL] 62,984 pass(es). View
2.34 KB

On closer inspection, there were a few drupalSettings.basePath that were not replaced. Updated those.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Getting this in again, before it breaks again (again). Committed/pushed to 8.x, thanks!

Wim Leers’s picture

THANK YOU!

/me rejoices :)

thedavidmeister’s picture

wow, fixed! amazing!

Status: Fixed » Closed (fixed)

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