Posted by sun on January 19, 2012 at 5:32pm
15 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | API change, needs backport to D7 |
Issue Summary
Generic off-shoot from #41595: All pager links have an 'active' CSS class
Problem
- If the current path is
/nodeand a link to the same path but with query string is output via l(), then the link is'active', although it is not the current path.
Comments
#1
This patch takes care of the issue at hand, however the tests will not pass because siimpletest plays with the querystring in such a way that I cannot seem to get the existing testLActiveClass() and testLCustomClass() tests to pass.
Any advice is welcome...
#2
The last submitted patch, 1410574.patch, failed testing.
#3
- Considering the performance sensitivity of this function, I would suggest to only look at the query part if the path part is equal to the current path. That is: move the code introduced by this patch into an if.
- Can the front page have a query part? I guess yes, as even the default front page uses a pager (if I'm correct, I've never used the default front page so far). If so, the query part should also be taken into account for the front page case. Combined with my previous remark, I think that this results in moving the new code into the already existing if.
+ $parsed_path = parse_url(check_plain(url($path, $options)));+ if (isset($parsed_path['query'])) {
+ parse_str(str_replace('&', '&', $parsed_path['query']), $path_query);
+ }
+ else {
+ $path_query = array();
+ }
- Why call url(), check_plain(), parse_url(), array check, array access, str_replace, and parse_str, to get what is directly available in $options['query'], though as an array? So it seems better to compare $options['query'] with parse_str($_SERVER['QUERY_STRING']). Please add a comment that we must use == (same key/values) and not === (same key/values, same order), and that we neither can use http_build_query($options['query']) === $_SERVER['QUERY_STRING'] for the same reason (being: different order in query is still the same resource).
Note: not passing in a query part (to the l() function) in the $options array but in the $path parameter is at your own risk and you're self responsible for encoding and such. So, IMO that is a case that we don't have to cater for here and thus this optimization is possible. Moreover, a (the only?) valid use case for passing in the query part in the $path is external URL's and they never equal the current page...
I think that in the resulting patch, any performance implications won't be on the critical performance path anymore, and thus this patch can get in more easily.
#4
I think that links to fragments in the page should also be excluded from getting the active class. Example: the "Add new comment" link to the reaction form at the end of a page (where comments are enabled). If others agree, we should change the title to something like:
Make l() respect the URL query string and fragment part before adding the 'active' CSS class
Code wise it will become only just a bit more difficult. Just check if there is a fragment entry in the $options array and if so do not add active. I think that the function documentation should be extended with a more elaborate description of what the "active" class on a link conceptually indicates.
#5
No, fragments need to be ignored. That's by design.
#6
This is just a test, I disregarded the patch in #1 for now.
#7
The last submitted patch, drupal-1410574-6.patch, failed testing.
#8
How about this?
#9
This works, but needs a test case for when you're on a page with a query string, which I guess will need a custom module?
#10
The last submitted patch, drupal-1410574-9.patch, failed testing.
#11
Okay, added a full test case fixed above failures.
#12
Glad to see that I was kinda close on the right way to accomplish this. #11 is much cleaner and I just gave it a test drive (while watching the NYC Camp keynotes). Everything looks great.
#13
We might also want to revert the stop-gap fix in pager.inc that was done to theme_pager_link() in the original issue? Just reverting the functional code will let us see whether the new pager tests are passing with the proposed fix here.
Also:
+++ b/core/includes/common.inc@@ -2319,13 +2319,20 @@ function l($text, $path, array $options = array()) {
+ $url_query = array();
+ if (isset($_SERVER['QUERY_STRING'])) {
+ parse_str($_SERVER['QUERY_STRING'], $url_query);
+ }
Isn't this the same as $_GET ?
#14
Even better!
I'm removing the last assertion of UrlTest::testLCustomClass() because it duplicates UrlTest::testLActiveClass() and is also wrong.
#15
The last submitted patch, drupal-1410574-14-combined.patch, failed testing.
#16
#14: drupal-1410574-14-combined.patch queued for re-testing.
#17
The last submitted patch, drupal-1410574-14-combined.patch, failed testing.
#18
Apparently
request()->query->all()doesn't work on the testbot?WFM locally...
#19
UrlTest fails with run-tests.sh, because it does not set up a Request object.
This patch passes with run-tests.sh, when combining it with #1215104: Use the non-interactive installer in WebTestBase::setUp()
#20
Okay, $_GET is still used in plenty of places in core, and Crell says that request() is a temporary function anyway. Might as well stick with what works.
#21
Added a comment and enabled the regression test.
#22
I guess the 'API change' tag makes this not backportable to D7? :(
But now there are no added function calls, so the performance isn't a problem anymore.
#23
I'm... not sure. The stop-gap fix for theme_pager_link() was possible to backport, because it had a focused and controlled effect and consequences. While this technically just fixes the bug in a generic way, the change impacts all links on a site. This can mean a unexpected change in behavior, potentially making existing themes look wonky in case they rely on the current misbehavior for styling.
But aside from that - yay for even passing the pager tests! :) This looks RTBC to me.
#24
Well we can have that conversation with David_Rothstein at the proper time. I know I need this fix for a D7 site with links to different Search API filters all showing as active.
#25
#21: drupal8.l-query-active.21.patch queued for re-testing.
#26
The last submitted patch, drupal8.l-query-active.21.patch, failed testing.
#27
Just rerolled patch from #21.
#28
The last submitted patch, drupal-1410574-27.patch, failed testing.
#29
Fixed the test failures.
This patch blocks #1778990: Merge theme_pager_link*() theme functions into theme_link()
#30
Looks good to me.
#31
+++ b/core/includes/common.incundefined@@ -2298,13 +2298,20 @@ function l($text, $path, array $options = array()) {
+ if ($_GET == $options['query']) {
I don't know if we should still be using $_GET here
#32
Let's leave that conversion of $_GET to whatever-kernel-esquy to another issue and WSCCI folks to figure out, pretty please.
#33
I'm not sure that $_GET is reliable anymore now that we are sometimes firing subrequests. For now, the alternative is fortunately simple:
drupal_container()->get('request');
We can convert it to something purdy and injected later, but at least for now we should be accessing the request.
#34
That's needless duct-taping right now IMHO, but here you go.
#35
The last submitted patch, drupal8.l-query-active.34.patch, failed testing.
#36
So update.php does not use the kernel yet, which means that drupal_container()->get('request') is not available.
Attached patch hacks the service definition into the update.php container in the same way we're hacking other services into the container there already; i.e., this workaround pattern is not introduced here.
Hopefully this comes back green. This patch still blocks #1778990: Merge theme_pager_link*() theme functions into theme_link(), and I need to move forward there ASAP (bound to feature-freeze).
#37
Alright, #36 passed, which means that the injection of the request into the container in update.php works as expected.
Can we move forward here? :)
#38
There are 3 conditions that have to be met to get the active class (path, language, query):
- Only the 3rd is extensively documented.
- The first 2 are joined in 1 if, the 3rd has its own (inner) if
So IMO it would be better to join all conditions in 1 if, with 1 extensive comment before that code that (shortly) explains the conditions to be met. OR: create 3 separate if statements with 1 comment line each.
Do we need to link the @todo's by also referring to this todo in the index.php as well? (or are there other means to take care of the @todo)
#39
I was not really happy about the added inner condition, too, so I've rewritten the conditional logic in l() and also adjusted the comment to explain all three conditions.
There's no new @todo introduced here, thus nothing to update or reference.
#40
Great, awaiting the test results ("test request sent" at the moment of writing), but for me this is RTBC.
#41
The test results are in, it has been RTBC before, so for me good enough to move to RTBC again.
#42
Looks good. Committed/pushed to 8.x.
#43
Tried to backport it to D7. This patch is a mix of:
- #39
- #29 (as $_GET has to be used in D7)
- Undoing the changes from #41595-16: All pager links have an 'active' CSS class (as #39 also already did) that solved the problem for theme_pager_link() only)
The tests from #41595: All pager links have an 'active' CSS class, which are part of the test collection, should show that this patch is also working fine. I am not sure if more tests are needed.
I agree with #41595-55: All pager links have an 'active' CSS class that states that this is not an API change.
#44
The last submitted patch, 1410574-43.patch, failed testing.
#45
- $_GET does contain 'q' in D7, so remove that in a copy of $_GET before comparing. This repaired most of the failing tests.
- The changes in testLActiveClass() from #39 are now also back-ported as in the current situation it tests without taking the query into account.
#46
The last submitted patch, 1410574-45.patch, failed testing.
#47
Works locally and does not seem to be related ...
#48
#45: 1410574-45.patch queued for re-testing.
#49
That doesnt sound right...
#50
re #49: That is how Drupal (until now) works: clean URLs are "rewritten" to index.php?q=..., see this code in bootstrap.inc:
<?phpfunction drupal_environment_initialize() {
...
// When clean URLs are enabled, emulate ?q=foo/bar ...
$_GET['q'] = request_path();
...
}
?>
So clean URLs or not, the q parameter will always be there.
#51
blarg ... read that as "doesnt"
Ignore me