Problem/Motivation
The patch committed in #2404929: Path class on <body> may be language specific, results in CSS bugs contains an error in the logic that adds body classes. As a result, the class 'path-frontpage' is never applied to the body class.
Proposed resolution
Fix the logic in template_preprocess_html
so the body class is applied correctly
Remaining tasks
- Write a patch
- Review
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff.txt | 1.01 KB | Gábor Hojtsy |
#33 | 2411597-33.patch | 2.54 KB | Gábor Hojtsy |
#29 | 2411597-28-after.png | 3.27 KB | idebr |
#29 | 2411597-28-before.png | 2.95 KB | idebr |
#28 | 2411795-28.patch | 2.55 KB | idebr |
Comments
Comment #1
idebr CreditAttribution: idebr commentedAdded an additional assertion in the test where the bug was introduced and fixed the missing body class.
Comment #2
davidhernandezWhy switch that to an empty string?
Comment #3
idebr CreditAttribution: idebr commentedIf we define the root_path as the first part of the path, eg. '/node/1' becomes 'node', the frontpage '/' would become an empty string.
Comment #4
Gábor HojtsyUhm, sorry for breaking that.
Since this is a language test (and anyway) it would be good to check in multiple languages to make sure this will be intact.
Comment #5
davidhernandezWell, we're defining root_path as a variable. For the front page the root isn't empty, it doesn't exist.
Comment #6
davidhernandezThe front page check doesn't strike me as language specific, so would it be better if that lived somewhere else?
Comment #7
idebr CreditAttribution: idebr commentedA added an additional assertion with french as the active language.
Makes sense :). I updated the patch accordingly.
Comment #8
Gábor Hojtsy@davidhernandez: the thing is multiple paths are the front page on a multilingual site, so testing that those return the same class makes sense IMHO.
The second is not French. If you would drupalGet('fr') that would be the French homepage.
Comment #9
davidhernandezOk, that makes sense.
Comment #10
idebr CreditAttribution: idebr commentedI assumed
<front>
would be the french homepage, but this works.Comment #11
Gábor HojtsyOnly found some comment English to fix at this point. Should be good to go. No need for a test only version since only changed comments.
Comment #19
davidhernandezNot sure I understand what's going on here.
$this->drupalGet('<front>');
is causing the test to retrieve user/2 ?Comment #20
davidhernandezLogging out fixes the get, but I don't know why that is necessary.
Comment #23
Gábor Hojtsy@davidhernandez: the testing profile does not have node or views modules so it is logical it sets the front page to /user/X which it does have a module enabled for. The test should work nonetheless with that setup, no?
Comment #24
davidhernandezOh, interesting. I hadn't thought of that. Learning new things.
Looking at the tests output it doesn't appear to render the user page as the front page, like it would with node or any other page. It is getting user/2, like a redirect. It does the same in all the other places in this group of tests where there is a get of the front page. I'm attaching screenshots. The taller image is after adding the logout.
Comment #25
idebr CreditAttribution: idebr commented@davidhernandez Does this patch still need work? If so, could you elaborate on what needs to be changed?
Comment #26
davidhernandez@idebr, the fix itself is fine. The only question I had is whether my adding the logout to the test is the right thing to do. It certainly works.
Comment #27
davidhernandezI just worked through this and I think I understand what is going on. Since node, etc. are not enabled
<front>
defaults to rendering the user page as the front page, but the main user page is the login. As a logged in user, the test gets redirected touser/2
which is the user's own account page. The test fails because it is looking atuser/2
. I assume then adding the logout is appropriate. Does that make sense?Comment #28
idebr CreditAttribution: idebr commentedYes, I was able to reproduce the behavior you described in #27. Since this is not immediately apparent, I updated the logout action with a comment. Can you check if I explained it properly?
Comment #29
idebr CreditAttribution: idebr commentedFor reviewers:
Markup before:
Markup after:
Comment #31
davidhernandezLooks ok to me.
Comment #32
idebr CreditAttribution: idebr commentedThe feedback from @Gábor Hojtsy in #4 has been adressed and @davidhernandez has fixed the tests to make sure the class is available. Setting this to RTBC.
Comment #33
Gábor HojtsyThe new comment was improperly wrapped. I took the liberty to make it a little bit more correct as well while rewrapping. Still RTBC :)
Comment #34
alexpottNormal, non-disruptive, bug fixes are permitted in beta. Committed 7736ba0 and pushed to 8.0.x. Thanks!
Comment #36
Gábor HojtsyThanks all!