Add new theme_support type: body-only #872

joyously · 2021-12-04T02:31:04Z

This is to implement the petition for core to have more control over the front end.

Motivation and context

The two main goals are

the required calls like wp_head(), wp_footer(), wp_body_open() are handled by core, not the theme
core provides a standardized way to filter attributes on the HTML

The approach used is to add a new theme_support called 'body-only' with the intent that the theme is written to handle the HTML that goes in the body of the page, and expects core to do the rest (enqueuing scripts and styles is unaffected).
The theme_support code calls for 'automatic-feed-links', 'title-tag', and 'html5'.

The template loader checks for 'body-only' and calls two new functions do_html5_header and do_html5_footer. It's important that the include for the template file remains as-is, so that the scope of variables in the template file is not affected.
To make the <head> section HTML flexible, the attributes are filtered using a new function cp_attributes. Themes would be able to use the same function to get a standardized way to filter HTML attributes. The existing filters are unaffected.
Future enhancements can be made easily by using cp_attributes in core functions that generate HTML. (See #792 (comment))

For themes:
There's no enforcement of using the cp_attributes function, but it would be expected if calling add_theme_support('body-only').
The changes needed for a 'body-only' theme:

in functions.php, call add_theme_support('body-only')
- remove add_theme_support for 'title-tag', 'html5', 'automatic-feed-links'
in header.php, remove all HTML output before and including the <body> tag
- remove output of pingback_url
in footer.php, remove wp_footer(), </body>, and </html>
to modify the viewport meta, use 'cp_attributes' filter

How has this been tested?

PR includes tests for new cp_attributes function.
I made minor changes to a copy of Twenty Fifteen and ran it locally to test the other changes.

Types of changes

New feature

Add 'body-only' to theme_support case statement. Add a check for 'body-only' themes to template-loader. Add output functions for top and bottom of HTML document, using a standard filtering function. Add new standard filtering function 'cp_attributes' to filter and escape attributes. Add test cases for new 'cp_attributes' function.

github-actions · 2021-12-04T02:31:16Z

Hi @joyously! 👋
Thank you for your contribution to ClassicPress! 💖
It looks like this is your first pull request. We appreciate and value your contribution!
Please read our contribution guide to find out how you can be more involved.
Thank you, ClassicPress Core Team.

ClassyBot · 2021-12-04T02:35:43Z

This pull request has been mentioned on ClassicPress Forums. There might be relevant details there:

https://forums.classicpress.net/t/make-core-in-charge-of-front-end-output/3649/5

pattonwebz · 2021-12-18T00:43:19Z

Does the html5 theme_support flag also power something related to comment forms or am I misremembering?

joyously · 2021-12-18T01:40:25Z

Does the html5 theme_support flag also power something related to comment forms or am I misremembering?

Two things actually.

ClassicPress/src/wp-includes/theme.php

Lines 2527 to 2539 in 049fafa

    
           case 'body-only' : 
        
           	add_theme_support( 'automatic-feed-links' ); 
        
           	add_theme_support( 'title-tag' ); 
        
           	add_theme_support( 'html5', array( 
        
           		'comment-list', 
        
           		'comment-form', 
        
           		'gallery', 
        
           		'caption', 
        
           		'search-form', 
        
           		'script', 
        
           		'style', 
        
           		'navigation-widgets', 
        
           	) );

pattonwebz · 2021-12-18T01:50:36Z

Does the html5 theme_support flag also power something related to comment forms or am I misremembering?

Two things actually.

ClassicPress/src/wp-includes/theme.php

Lines 2527 to 2539 in 049fafa

case 'body-only' :

add_theme_support( 'automatic-feed-links' );

add_theme_support( 'title-tag' );

add_theme_support( 'html5', array(

'comment-list',

'comment-form',

'gallery',

'caption',

'search-form',

'script',

'style',

'navigation-widgets',

) );

Gotcha, so a theme which deferred to core to power header, footer would inherently get all the things the html5 flag provided. Nice solution :)

joyously · 2021-12-18T02:03:49Z

As mentioned on Slack, I was trying to figure a way to make sure that wp_link_pages() was called. I thought of storing a boolean in the theme_support array when the function is called, but I'm not sure of a good way to output it if it isn't called.

KTS915 · 2021-12-20T21:59:23Z

Not sure if this is the right place to ask this question, but it does seem germane. Why does the HTML5 version of the comment form add the novalidate attribute?

joyously · 2021-12-20T22:24:03Z

Why does the HTML5 version of the comment form add the novalidate attribute?

I would guess (just a guess) that the validation is already being done on the server, and so to prevent two different experiences and/or extra scripting needed for legacy to look like HTML5 for client side validation which is unneeded anyway, it's better to put novalidate. There's not much that the HTML5 validation can do for the comment form anyway.

KTS915 · 2021-12-21T03:46:44Z

But wouldn't that be true of the HTML4 version of the comment form? Anyway, on a project where I'm adding other required fields to the comment form, I had to resort to using the HTML4 version so that I could get the form to be validated.

Just to be clear, I am sanitizing server-side, but it would be very annoying for someone to lose their whole comment because the required field wasn't completed, when simple client-side validation would prevent this.

joyously · 2021-12-21T04:22:58Z

I went ahead and looked for the ticket and found
Re-evaluate whether comment form should still get the HTML5 novalidate attribute
which gives a link to why it was put there in the first place. Apparently, some people use JS to remove the attribute. It seems to me that the server side validation would pre-fill the form, so that the comment is not lost, but I've not looked at the code.

KTS915 · 2021-12-21T15:15:40Z

Thanks for finding that, @joyously . I read through that and related tickets, and it all seems a lot of "there be dragons" discussion where they seem unable to pin down what they're afraid of. Very odd.

joyously · 2021-12-21T17:00:38Z

It does bring up a good point for how the HTML5 support is done, and what is the scope of back-compat that is desired for themes going forward.
If we were to remove all the checks for HTML5 and just do HTML5, it would cut out the older themes (which couldn't be updated due to compatibility with user CSS and child themes). But it would streamline the code a bit and produce more standardized modern markup.

KTS915 · 2021-12-23T15:14:21Z

If we were to remove all the checks for HTML5 and just do HTML5, it would cut out the older themes (which couldn't be updated due to compatibility with user CSS and child themes). But it would streamline the code a bit and produce more standardized modern markup.

This makes sense to me. It's presumably not a breaking change either, because (a) the browsers that don't support HTML5 are browsers CP doesn't support anyway and (b) those browsers will just fall back to type="text" where HTML5 fields are specified.

(As for the validation issue, having read all the threads about the issue, I reverted to using the HTML5 version of the comment form and then used an output buffer to capture the form and remove the novalidate attribute before echoing it to the page.)

joyously · 2021-12-23T15:41:57Z

It's presumably not a breaking change either, because (a) the browsers that don't support HTML5 are browsers CP doesn't support anyway and (b) those browsers will just fall back to type="text" where HTML5 fields are specified.

Assuming "it" is removing the checks for the html5 option, let me just say that back-compat has less to do with the browser support than with older themes written to style the older markup.

joyously · 2022-01-01T21:22:48Z

It occurred to me that there is code to load the wp-includes/theme-compat files for header.php and footer.php, which could be affected by this change. However, those are both deprecated since WP 3.0, so would it make sense to remove them or to check for the theme support?

ClassyBot · 2022-04-11T15:50:12Z

This pull request has been mentioned on ClassicPress Forums. There might be relevant details there:

https://forums.classicpress.net/t/introduction-of-a-credit-function-in-the-theme/3976/11

ClassyBot · 2022-05-19T23:36:58Z

This pull request has been mentioned on ClassicPress Forums. There might be relevant details there:

https://forums.classicpress.net/t/feedback-on-themes/4046/2

viktorix · 2022-07-09T16:26:38Z

We should bring this PR up at the next meeting. This seems reasonable and no one has said it's a bad idea.

ClassyBot · 2022-08-08T19:03:08Z

This pull request has been mentioned on ClassicPress Forums. There might be relevant details there:

https://forums.classicpress.net/t/classicpress-specific-themes/4014/23

…evelop' into body-only

joyously · 2022-10-14T21:19:40Z

Today someone in the WP pluginreview Slack channel was talking about a function to output the attributes, and their snippet was similar to the last part of my function. Someone pointed out that esc_attr is not good enough for the attribute name, and sanitize_key would be better, but it is still not good if the user is involved in the input. It seems there are several core functions that filter attributes without excessive sanitization, though.
I should change esc_attr to sanitize_key for the attribute name, at least. Do you agree?

mattyrob · 2022-10-15T07:48:18Z

We should make that change if that was the WP assertion, sanitize_key() for the attribute name then and esc_attr() for the attribute name.

I wonder if the function name may need to be reconsidered too to more broadly describe what it is doing - it handles more than attributes if it takes the attribute names as inputs too.

joyously · 2022-10-15T13:16:08Z

Since WP doesn't have this function (it was ignored when I proposed it there), so it's not a"WP assertion"; just a comment in Slack.
I have seen a few core functions that filter an attribute array and don't sanitize the key, so I don't know if it needs to happen or not. Core functions cannot determine if the input comes from the user or not, so it seems that whatever code calls the filter should handle that.

As for the name, I'm open to suggestions if you think it is handling more than attributes. Lots of filters on arrays utilize the key and the value, but it's all attributes.
Edit: I forgot to add that sanitize_key makes it lowercase.

mattyrob · 2022-10-15T15:30:51Z

I don't really see a semantic difference between an assertion and a comment, the word assertion means to state something positively to me, so I'm not sure why the need to be adversarial.

If esc_attr() is not going to adequately sanitize the attribute name then alternative escaping is needed. If sanitizize_key() can do the job better then it should be used. Does it make any difference if it makes everything lowercase? Should it be lowercase anyway? This reference indicates they should

As for the function name, I honestly don't really know. I do like the cp_ prefix as it is unique to ClassicPress. WordPress is using [attributes](https://developer.wordpress.org/block-editor/reference-guides/block-api/block-attributes/) in the Block editor for much more. What about cp_html_attributes()? That may not be massively more descriptive but perhaps it differentiates from WP more and makes it clearer it is a function just for HTML attributes.

Whatever we settle on (an it may not even change) the filter names may need amending too.

joyously · 2022-10-15T15:54:03Z

I wasn't being adversarial. I was clarifying that it was not an official WP assertion (no code or document stating it), but simply one contributor's comment in Slack about some code for a plugin.

The question is whether core functions need to sanitize the attribute name at all. There are core functions that filter attribute arrays, and don't modify them before using them. That could be amended with sanitize_key also, but they currently are not.
I don't know if it matters about lowercase; I just thought I would mention it.

I don't see any point in adding another noun to the name. If anything, it should be a verb, but I didn't want it to be a long function name since it should be used a lot in themes and other functions that output HTML. And yes, if the name is changed, I have to change it everywhere in my PR.

mattyrob · 2022-10-15T16:16:31Z

The question is whether core functions need to sanitize the attribute name at all.

I would sound definitely yes. There is a filter in the code, and any nefarious theme or plugin may be able to leverage the filter, so anything passed though it should be sanitised and escaped.

joyously · 2022-10-15T17:27:44Z

This sort of question has come up often at WP: should core escape translated text? They always answer no, even though translated text is filtered. The theme team nudges themes to escape their translations, though.

A quick search shows language_attributes() only has echo get_language_attributes(), which filters the computed attributes. This is standard for core.
The attribute array is filtered in Walker_Category::start_el and the attribute names are not sanitized.
I'm sure I could find more if I look more.
I think the logic is that a nefarious plugin can do anything, so there is some level of acceptance in core functions...

I did see the wp_kses_uri_attributes() function was added for 5.0, which is a bit better than my check for src and href for using esc_url rather than esc_attr.

mattyrob · 2022-10-15T20:43:40Z

This sort of question has come up often at WP: should core escape translated text? They always answer no, even though translated text is filtered. The theme team nudges themes to escape their translations, though.

Do you know the WP rationale for this? I'm presuming it is for backwards compatibility with the available themes they have open to them. If that is the case, perhaps we should have higher aspirations.

A quick search shows language_attributes() only has echo get_language_attributes(), which filters the computed attributes. This is standard for core. The attribute array is filtered in Walker_Category::start_el and the attribute names are not sanitized. I'm sure I could find more if I look more. I think the logic is that a nefarious plugin can do anything, so there is some level of acceptance in core functions...

I can accept that logic but given we are going to allow plugins from anywhere with the Plugin URL header we cannot vet all plugins so security should arguably be stronger in the core.

I did see the wp_kses_uri_attributes() function was added for 5.0, which is a bit better than my check for src and href for using esc_url rather than esc_attr.

It is worth adding that function into CP if it delivers something the project may find useful?

joyously · 2022-10-15T21:08:15Z

Do you know the WP rationale for this? I'm presuming it is for backwards compatibility

No, I think it's mostly because it is not necessarily output at the point of translation. (escape on output, because you need context) Most core translations are seen in the admin, login screen, comments, admin bar. The theme doesn't do any of that and has its own translations for any options(admin) or rare front end stuff like Next, Previous, and screen reader text.

given we are going to allow plugins from anywhere with the Plugin URL header we cannot vet all plugins

In this, we are no different from WP.

It is worth adding that function into CP if it delivers something the project may find useful?

We might ought to investigate what else was added for KSES.

mattyrob · 2022-10-16T08:35:12Z

No, I think it's mostly because it is not necessarily output at the point of translation. (escape on output, because you need context) Most core translations are seen in the admin, login screen, comments, admin bar. The theme doesn't do any of that and has its own translations for any options(admin) or rare front end stuff like Next, Previous, and screen reader text.

Ah, that actually makes some sense based on the WP standards. There is no point doing any escaping in the __() function if the recommendation is that when the echo is called is should be sanitised then.

given we are going to allow plugins from anywhere with the Plugin URL header we cannot vet all plugins

In this, we are no different from WP.

Maybe we can do better. I had not considered until now the risks this poses. Publish a plugin in the main repository that is reviewed and published, but include a header tag to enable updates from your own server. Then add a more recent version on your own server that contains pretty much the same plugin with the addition of a back door for you into any site your plugin is installed on - and with automatic updates you could have a lot of compromised site open to you.

We might ought to investigate what else was added for KSES.

Certainly worth looking at but probably shouldn't hold this PR up - for this we need to satisfy ourselves that we are escaping data immediately before it is output and with the most appropriate and efficient function. And decide what we are calling the function and filters.

joyously · 2022-10-16T12:58:08Z

I had not considered until now the risks this poses.

WP does not allow code updates from plugins in its repository. I have advocated for the same restriction for the CP directory. I refuse to use plugins that are self-updating. Please see discussion surrounding https://classicpress.slack.com/archives/C03N696QLRJ/p1664899139545299 and my comment on the recently merged PR for the Update URI header.

…into body-only

mattyrob · 2022-11-10T17:01:33Z

Discussed at weekly review and agreed for merge.

joyously added 3 commits December 3, 2021 18:25

Merge remote-tracking branch 'upstream/develop' into body-only

ae41122

Adjust for merge conflict

049fafa

joyously mentioned this pull request Dec 21, 2021

Add action within comment form tag #888

Closed

Merge remote-tracking branch 'upstream/develop' into body-only

871ddfe

viktorix mentioned this pull request Jul 9, 2022

Remove microformats from the core #1053

Closed

joyously added 5 commits August 19, 2022 11:50

Merge remote-tracking branch 'upstream/develop' into body-only

2b3ea6d

update @SInCE and coding standard stuff

32fd39d

adjust whitespace for coding standards

e9763e3

adjust whitespace for standards

c647b94

adjust whitespace for standards

d71c068

mattyrob added this to the 1.5.0-rc1 milestone Sep 1, 2022

joyously and others added 8 commits September 1, 2022 13:09

Merge remote-tracking branch 'upstream/develop' into body-only

547f06c

adjust for coding standards, Merge remote-tracking branch 'upstream/d…

d2b7bd8

…evelop' into body-only

adjust for coding standards

36c6017

Merge remote-tracking branch 'upstream/develop' into body-only

5554a7a

adjust whitespace for coding standards

369f047

whitespace changes for coding standards

42ffb4b

Merge branch 'ClassicPress:develop' into body-only

600da3c

Merge branch 'ClassicPress:develop' into body-only

7389c46

joyously added 2 commits October 21, 2022 17:28

use sanitize_key for attribute name

7d16534

Merge branch 'body-only' of https://github.com/joyously/ClassicPress …

411483f

…into body-only

xxsimoxx added the status: needs refresh Merge Conflicts detected by Github. Needs a refresh from develop. label Oct 27, 2022

Merge branch 'develop' into body-only

198b1b0

mattyrob added task: needs 2nd review One member has reviewed this. Need a 2nd review to complete. and removed status: needs refresh Merge Conflicts detected by Github. Needs a refresh from develop. labels Nov 10, 2022

Fix layout and update test assertions

efa787f

mattyrob merged commit 4c74767 into ClassicPress:develop Nov 10, 2022

joyously deleted the body-only branch December 12, 2022 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new theme_support type: body-only #872

Add new theme_support type: body-only #872

joyously commented Dec 4, 2021

github-actions bot commented Dec 4, 2021

ClassyBot commented Dec 4, 2021

pattonwebz commented Dec 18, 2021

joyously commented Dec 18, 2021

pattonwebz commented Dec 18, 2021

joyously commented Dec 18, 2021

KTS915 commented Dec 20, 2021

joyously commented Dec 20, 2021

KTS915 commented Dec 21, 2021

joyously commented Dec 21, 2021

KTS915 commented Dec 21, 2021

joyously commented Dec 21, 2021

KTS915 commented Dec 23, 2021 •

edited

joyously commented Dec 23, 2021

joyously commented Jan 1, 2022

ClassyBot commented Apr 11, 2022

ClassyBot commented May 19, 2022

viktorix commented Jul 9, 2022

ClassyBot commented Aug 8, 2022

joyously commented Oct 14, 2022

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022 •

edited

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022

mattyrob commented Oct 16, 2022

joyously commented Oct 16, 2022

mattyrob commented Nov 10, 2022

Add new theme_support type: body-only #872

Add new theme_support type: body-only #872

Conversation

joyously commented Dec 4, 2021

Motivation and context

How has this been tested?

Types of changes

github-actions bot commented Dec 4, 2021

ClassyBot commented Dec 4, 2021

pattonwebz commented Dec 18, 2021

joyously commented Dec 18, 2021

pattonwebz commented Dec 18, 2021

joyously commented Dec 18, 2021

KTS915 commented Dec 20, 2021

joyously commented Dec 20, 2021

KTS915 commented Dec 21, 2021

joyously commented Dec 21, 2021

KTS915 commented Dec 21, 2021

joyously commented Dec 21, 2021

KTS915 commented Dec 23, 2021 • edited

joyously commented Dec 23, 2021

joyously commented Jan 1, 2022

ClassyBot commented Apr 11, 2022

ClassyBot commented May 19, 2022

viktorix commented Jul 9, 2022

ClassyBot commented Aug 8, 2022

joyously commented Oct 14, 2022

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022 • edited

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022

mattyrob commented Oct 15, 2022

joyously commented Oct 15, 2022

mattyrob commented Oct 16, 2022

joyously commented Oct 16, 2022

mattyrob commented Nov 10, 2022

KTS915 commented Dec 23, 2021 •

edited

joyously commented Oct 15, 2022 •

edited