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

CommentFileSizeAuthor
#33 interdiff.txt1.01 KBGábor Hojtsy
#33 2411597-33.patch2.54 KBGábor Hojtsy
#29 2411597-28-after.png3.27 KBidebr
#29 2411597-28-before.png2.95 KBidebr
#28 2411795-28.patch2.55 KBidebr
#28 interdiff-28-20.txt848 bytesidebr
#28 2411795-28.fail_.patch2.08 KBidebr
#24 Screen Shot 2015-01-22 at 9.24.20 AM.png90.25 KBdavidhernandez
#24 Screen Shot 2015-01-22 at 9.22.38 AM.png31.59 KBdavidhernandez
#24 Screen Shot 2015-01-22 at 9.22.13 AM.png48.08 KBdavidhernandez
#20 interdiff-2411597.txt1.63 KBdavidhernandez
#20 class_path_frontpage-2411597-20.patch2.35 KBdavidhernandez
#11 interdiff.txt1.98 KBGábor Hojtsy
#11 2411597-11.patch2.33 KBGábor Hojtsy
#10 2411597-9.patch1.93 KBidebr
#10 interdiff-9-7.txt811 bytesidebr
#10 2411597-9.fail_.patch1.46 KBidebr
#7 2411597-6.patch1.93 KBidebr
#7 interdiff-6-1.txt2.6 KBidebr
#7 2411597-6.fail_.patch1.46 KBidebr
#1 2411597-1.patch1.86 KBidebr
#1 interdiff-13-10.txt2.62 KBidebr
#1 2411597-1.fail_.patch890 bytesidebr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

idebr’s picture

idebr CreditAttribution: idebr commented
Status: Active » Needs review
FileSize
2411597-1.fail_.patch890 bytes
interdiff-13-10.txt2.62 KB
2411597-1.patch1.86 KB

Added an additional assertion in the test where the bug was introduced and fixed the missing body class.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

Why switch that to an empty string?

idebr’s picture

idebr CreditAttribution: idebr commented

If we define the root_path as the first part of the path, eg. '/node/1' becomes 'node', the frontpage '/' would become an empty string.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: class 'path-frontpage' missing from <body> » Class 'path-frontpage' missing from <body>
Status: Needs review » Needs work
Issue tags: +D8MI, +language-base, +sprint

Uhm, sorry for breaking that.

+++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
@@ -265,6 +265,11 @@ function testLanguageBodyClass() {
+    // Check if the class on the frontpage is applied correctly.
+    $this->drupalGet('<front>');
+    $class = $this->xpath('//body[contains(@class, :class)]', array(':class' => 'path-frontpage'));
+    $this->assertTrue(isset($class[0]), 'path-frontpage class found on the body tag');

Since this is a language test (and anyway) it would be good to check in multiple languages to make sure this will be intact.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

Well, we're defining root_path as a variable. For the front page the root isn't empty, it doesn't exist.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

The front page check doesn't strike me as language specific, so would it be better if that lived somewhere else?

idebr’s picture

idebr CreditAttribution: idebr commented
Status: Needs work » Needs review
FileSize
2411597-6.fail_.patch1.46 KB
interdiff-6-1.txt2.6 KB
2411597-6.patch1.93 KB

Since this is a language test (and anyway) it would be good to check in multiple languages to make sure this will be intact.

A added an additional assertion with french as the active language.

Well, we're defining root_path as a variable. For the front page the root isn't empty, it doesn't exist.

Makes sense :). I updated the patch accordingly.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

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

+++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
@@ -261,10 +261,21 @@ function testLanguageBodyClass() {
+    // Check if the class on the frontpage is applied correctly.
+    $this->drupalGet('<front>');
...
+    // Check if the frontpage class is applied correctly on french language.
+    $this->drupalGet('<front>');

The second is not French. If you would drupalGet('fr') that would be the French homepage.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

multiple paths are the front page on a multilingual site, so testing that those return the same class makes sense

Ok, that makes sense.

idebr’s picture

idebr CreditAttribution: idebr commented
Status: Needs work » Needs review
FileSize
2411597-9.fail_.patch1.46 KB
interdiff-9-7.txt811 bytes
2411597-9.patch1.93 KB

The second is not French. If you would drupalGet('fr') that would be the French homepage.

I assumed <front> would be the french homepage, but this works.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community
FileSize
2411597-11.patch2.33 KB
interdiff.txt1.98 KB

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

The last submitted patch, 1: 2411597-1.fail_.patch, failed testing.

The last submitted patch, 1: 2411597-1.patch, failed testing.

The last submitted patch, 7: 2411597-6.fail_.patch, failed testing.

The last submitted patch, 10: 2411597-9.fail_.patch, failed testing.

The last submitted patch, 7: 2411597-6.patch, failed testing.

The last submitted patch, 10: 2411597-9.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2411597-11.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

Not sure I understand what's going on here. $this->drupalGet('<front>'); is causing the test to retrieve user/2 ?

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Status: Needs work » Needs review
FileSize
class_path_frontpage-2411597-20.patch2.35 KB
interdiff-2411597.txt1.63 KB

Logging out fixes the get, but I don't know why that is necessary.

Status: Needs review » Needs work

The last submitted patch, 20: class_path_frontpage-2411597-20.patch, failed testing.

Gábor Hojtsy queued 20: class_path_frontpage-2411597-20.patch for re-testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

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

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
FileSize
Screen Shot 2015-01-22 at 9.22.13 AM.png48.08 KB
Screen Shot 2015-01-22 at 9.22.38 AM.png31.59 KB
Screen Shot 2015-01-22 at 9.24.20 AM.png90.25 KB

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

idebr’s picture

idebr CreditAttribution: idebr commented
Status: Needs work » Needs review

@davidhernandez Does this patch still need work? If so, could you elaborate on what needs to be changed?

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

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

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

I 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 to user/2 which is the user's own account page. The test fails because it is looking at user/2. I assume then adding the logout is appropriate. Does that make sense?

idebr’s picture

idebr CreditAttribution: idebr commented
FileSize
2411795-28.fail_.patch2.08 KB
interdiff-28-20.txt848 bytes
2411795-28.patch2.55 KB

Yes, 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?

// Without the node module enabled, the user login page becomes the
// frontpage. This page redirects authenticated users to their profile page,
// so check with an anonymous user instead.

idebr’s picture

idebr CreditAttribution: idebr commented
FileSize
2411597-28-before.png2.95 KB
2411597-28-after.png3.27 KB

For reviewers:

Markup before:

Markup after:

The last submitted patch, 28: 2411795-28.fail_.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

Looks ok to me.

idebr’s picture

idebr CreditAttribution: idebr commented
Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2411597-33.patch2.54 KB
interdiff.txt1.01 KB

The new comment was improperly wrapped. I took the liberty to make it a little bit more correct as well while rewrapping. Still RTBC :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Normal, non-disruptive, bug fixes are permitted in beta. Committed 7736ba0 and pushed to 8.0.x. Thanks!

  • alexpott committed 7736ba0 on 8.0.x 
    Issue #2411597 by idebr, davidhernandez, Gábor Hojtsy: Class 'path-...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Thanks all!

Status: Fixed » Closed (fixed)

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