API page: https://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/t...

I suppose the wrong path to the default location of page.html.twig is given. Actual documentation says:

Most themes use their own copy of page.html.twig. The default is located inside "modules/system/page.html.twig". Look in there for the full list of variables.

But actually it is found under: core/modules/system/templates/page.html.twig

Comments

stefan.korn created an issue. See original summary.

joyceg’s picture

Assigned: Unassigned » joyceg
joyceg’s picture

Uploading the patch.

joyceg’s picture

Status: Active » Needs review
Cottser’s picture

Status: Needs review » Needs work

This is tricky now because we have stable and classy base themes. So the default is either in the stable theme if you have no base theme in your theme's .info.yml, or classy. I would lean towards stable as the "default" page template.

Thanks!

stefan.korn’s picture

Ok, then it should be like this

stefan.korn’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Twig

Thanks!

+++ b/core/includes/theme.inc
@@ -1313,7 +1313,7 @@ function template_preprocess_html(&$variables) {
- * inside "modules/system/page.html.twig". Look in there for the full list of
+ * inside "core/themes/stable/templates/layout/page.html.twig". Look in there for the full list of

This should be re-wrapped to fit within 80 characters per https://www.drupal.org/node/1354#drupal.

@stefan.korn just want to mention since this is assigned it's good to give the person assigned a day or two to work on it before jumping in.

stefan.korn’s picture

Assigned: joyceg » stefan.korn
Status: Needs work » Needs review
FileSize
682 bytes

Reformatting patch, respecting Drupal API documentation standards (general).

jhodgdon’s picture

Status: Needs review » Needs work
+++ b/core/includes/theme.inc
@@ -1314,8 +1314,8 @@ function template_preprocess_html(&$variables) {
+ * inside "core/themes/stable/templates/layout/page.html.twig".

Why are we putting this path in quotes? I think we should take out the quotes.

And anyway... this whole paragraph is kind of crazy. We don't have references like this in any other template_preprocess function that I am aware of, and a lot of themes have a lot of custom templates... Why are we saying this here? We should really just take out this whole paragraph, or just say "See the page.html.twig template for list of variables." and we don't need to be specific about saying which one is the "default". They all have lists of the variables in them.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
651 bytes

@jhodgdon
I have done according to your comment.

@stefan.korn
Apologize After creating patch only, I saw it was assigned to you.

Status: Needs review » Needs work

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

stefan.korn’s picture

@jhodgdon: Regarding quotes, I just took it over as it was. IMHO quotes are ok here, because they outline the path in some way. But of course this is only my personal "feeling" and if it is against standards or otherwise hindering it can and should be changed.

You are surely right that this kind of description with path is not there in every template_preprocess function, but in https://api.drupal.org/api/drupal/core!modules!node!node.module/function... it is the same. And from my point of view this information for the default path is not useless. I came across this when I was checking a problem with page.html.twig in a theme I was using. It seemed there was something wrong in the page.html.twig of this theme and then I was going to look in the default page.html.twig to spot any differences. And I found that the theme I was using had used some older elements (variables) of page.html.twig which got removed from template_preprocess_page somewhere along the way. But anyway I could also go with removing the path, because the search for page.html.twig delivers the necessary informations too.

@Cottser: You stated that the default is now in stable theme. If I go https://api.drupal.org/api/drupal/8/search/page.html.twig then "Default theme implementation to display a single page." is still "core/modules/system/templates/page.html.twig". Just wanted to point this out, because this seems a little inconsistent to me.

@All: I think patch #8 and #11 (while I am not sure why it failed testing) now offer the possibilty to fix it either this or that way, so I am leaving that decision to the experienced folks out there.

stefan.korn’s picture

Assigned: stefan.korn » Unassigned
jhodgdon’s picture

I really think that if we put the reference in here, it should go to system module and not the stable theme.

I also really think that we should not put the long explanation in there at all. The header already says it is the preprocessing for page.html.twig, and for *most* preprocessing functions, that is enough.

But I will leave it to Cottser to decide.

jhodgdon’s picture

Anyway, definitely no quotes!

Cottser’s picture

Since Stable is the default base theme (you get this by just creating a new theme with an .info.yml and not specifying a base theme) I think it would make sense to say it's the default.

However the latest patch looks reasonable to me (I agree with @jhodgdon we probably don't need a full path) other than being a bit terse, it could use the word "a" added in there :)

+++ b/core/includes/theme.inc
@@ -1327,9 +1327,7 @@ function template_preprocess_html(&$variables) {
+ * See the page.html.twig template for list of variables.

Maybe "See the page.html.twig template for a list of variables."

jhodgdon’s picture

how about "the list of variables" not "a"?

Cottser’s picture

Yup sounds good :)

harsha012’s picture

Status: Needs work » Needs review
Issue tags: +#drupalconasia2016, +drupalconasia2016
FileSize
630 bytes

Reload the patch with documentation

harsha012’s picture

Issue tags: -#drupalconasia2016
Cottser’s picture

Status: Needs review » Needs work
+++ b/core/includes/theme.inc
@@ -1313,9 +1313,7 @@ function template_preprocess_html(&$variables) {
+ * See the page.html.twig template for list of variables.

Thanks @harsha012. I think this should say "…for the list of variables." as was discussed in the recent comments :)

aditya_anurag’s picture

Assigned: Unassigned » aditya_anurag
harsha012’s picture

Status: Needs work » Needs review
FileSize
634 bytes

added patch as per the comment

aditya_anurag’s picture

Status: Needs review » Needs work
aditya_anurag’s picture

swarad07’s picture

Status: Needs work » Needs review
aditya_anurag’s picture

swarad07’s picture

The patch looks good, lets wait for tests to finish. Although I don't think there is a reason they will fail. ;-) We can mark it RTBC after that.

Thanks.

Cottser’s picture

Can someone please provide an interdiff? :)

Yes it's a small patch but practice practice practice!

harsha012’s picture

Assigned: aditya_anurag » harsha012
aditya_anurag’s picture

FileSize
980 bytes

@harsha012,

I was working on this.
Added interdiff.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

  • Cottser committed 82296bf on 8.1.x
    Issue #2621422 by stefan.korn, harsha012, aditya_anurag, rakesh.gectcr,...

  • Cottser committed cc369fb on 8.0.x
    Issue #2621422 by stefan.korn, harsha012, aditya_anurag, rakesh.gectcr,...
Cottser’s picture

Assigned: harsha012 » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed and pushed poolside 82296bf to 8.1.x and cc369fb to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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