Print skip-link-focus-fix inline instead of enqueueing as blocking script

[westonruter]

Enqueueing scripts is very costly since the browser will block rendering until the script is loaded. What's more is that it is only applicable in IE11, so it is particularly bad to have this blocking script loaded for 90% of browsers which will never use it. (If IE11 supported conditional comments, those could have been used here, but it does not.) Blocking scripts should be minimized as much as possible, either by adding the async attribute or by outputting the script inline.

While core doesn't yet have built-in support for async, core does currently add an inline script for adding the emoji logic (print_emoji_detection_script()/_print_emoji_detection_script()). A similar approach is proposed here for the skip-link-focus-fix script. Just like for the emoji detection script, the skip-link-focus-fix is small in size and it is only applicable to IE11.

See corresponding pull requests:

• Print skip-link-focus-fix inline instead of enqueueing as blocking script Automattic/_s#1323
• Print skip-link-focus-fix inline instead of enqueueing as deferred script wprig/wprig#139

[joyously]

There might be a great reason to do it, but it goes against the Theme Requirements, which even default themes should follow (lead by example). The reason is for child theme ability to dequeue a script.

[westonruter]

@joyously all a child theme has to do to dequeue is this:

remove_action( 'wp_print_footer_scripts', 'twentynineteen_skip_link_focus_fix' );

[mor10]

I think this is a good case for the Requirements being updated to add an exception.

[Nikschavan]

file_get_contents is not allowed from theme check plugin WordPress/theme-check#55
Which means this is essentially blocked from W.org for themes.

[westonruter]

So swap it out with calls to $wp_filesystem?

global $wp_filesystem;
echo $wp_filesystem->get_contents( get_template_directory() . '/js/skip-link-focus-fix.js' );

[Nikschavan]

Yes. Theme check requires to use WordPress API's wherever possible.

This change will also be required in Automattic/_s#1323 (comment)

[westonruter]

Is there precedent for using WP Filesystem in themes? The $wp_filesystem API is located in wp-admin and is not loaded by default in WP.

[westonruter]

It seems not appropriate to use WP Filesystem here. WordPress uses file_get_contents() internally for example in WP_Theme::get_post_templates(). If the main concern is that allow_url_fopen is enabled and that a URL is passed to file_get_contents(), then it would seem better to define a new function in WP that is specifically for reading files from the filesystem and short-circuits if a URL is provided as the file to read.

[joyously]

This is discussed many times in #themereview and it is mostly to flag questionable usage of the function. We are aware that the function is used internally, so it doesn't make sense to try to use the WP Filesystem like that. But then themes should not need to read files either.

[westonruter]

Why should themes not need to read files?

[Nikschavan]

As per the Theme check plugin - Not using this function is a "Warning" and not "Required" but historically this function is disallowed in the manual reviews by the reviewers for this explaination.

Themes do require to read from files. The most discussed use case if reading JSON files and after this PR and Automattic/_s#1323 there will be another reason for all the themes as the default themes are role models for all the other theme developers.

Apart from allow_url_fopen, another discussed problem with this function is related to incorrect file permissions on some hosts. filesystem access method with access credentials can be added in wp-config.php in such cases, to use correct method for reading/writing to files with WP_Filesystem. I think @Otto42 can chime in on this if this is actually a problem or not.

If a function can be added from the core to handle these cases then it would be a great solution.

[westonruter]

I think that WP_Filesystem is only ever needed when a file needs to be written to. Reading should always be able to be done directly.

[Otto42]

The reason we originally banned file_get_contents was that the function was only ever used by malicious code in order to hide their malicious code. We had several examples of it in the theme directory, and no examples of it being used for good things.

The reason we left it there is because some theme developers started trying to use it to load large chunks of JSON data, like a list of google fonts, for example, then parsing that data with json_decode.. on every page load. This is slow and inefficient when you can simply pre-parse the data into a native PHP array or object and then simply include that PHP directly.

I'm not against removing the rule from theme-check, but I am generally against the notion of themes needing to read data from files in order to do something with that data, when there's simpler ways like simply including the data in the code directly. There's no reason other than organization or readability that tiny pieces of code like this can't be minified and added inline to the function, rather than being a separate file that requires additional file reads.

Also, simply reading and outputting the file leaves the comments intact, bloating the html page output and so forth. Might as well pre-minify it and just put it right in there with wp_add_inline_script. It's tiny enough to justify that.

[justintadlock]

To be clear, outputting JS code via wp_head is not against the TRT requirements. We do have a requirement that script files use wp_enqueue_script() so that it's possible to dequeue. Since this is output JS code, it technically falls outside of the requirement.

With that said, the file_get_contents() call is the issue. While not against the TRT requirements, it's not allowed because of Theme Check. I've tried more than once, and failed, to get this check lowered to something that would make it through the submission process. I'd love to see some more push to change this.

Also, file() will work in the case that the more appropriate file_get_contents() isn't used.

[joyously]

To be clear, outputting JS code via wp_head is not against the TRT requirements. We do have a requirement that script files use wp_enqueue_script() so that it's possible to dequeue. Since this is output JS code, it technically falls outside of the requirement.

This seems to contradict itself. Did you mistype something?

[justintadlock]

This seems to contradict itself. Did you mistype something?

No. I meant exactly what I wrote. The intention of the TRT requirement is for theme authors to not output something like this without using wp_enqueue_script():

add_action( 'wp_head', function() { ?>
	<script type="text/javascript" src="example.js"></script>
<?php } );

However, it's perfectly acceptable in terms of the guidelines to do this:

add_action( 'wp_head', function() { ?>
	<script>
		// Output some JS code.
	</script>
<?php } );

[westonruter]

Also, file() will work in the case that the more appropriate file_get_contents() isn't used.

So in the mean time we could do:

echo implode( '', file( get_template_directory() . '/js/skip-link-focus-fix.js' ) );

This would just be a workaround for Theme Check because, like file_get_contents(), the file() function is also allow_url_fopen-aware:

A URL can be used as a filename with this function if the fopen wrappers have been enabled. See fopen() for more details on how to specify the filename. See the Supported Protocols and Wrappers for links to information about what abilities the various wrappers have, notes on their usage, and information on any predefined variables they may provide.

[joyously]

Those two examples look too similar, but I don't come to the same conclusion as Weston. It seems like echoing a file would be more like the TRT is trying to prevent than the one they allow.
If you are outputting the scripts tags and JS, why not just do include_once instead of reading the file?

[westonruter]

@joyously using include_once would be undesirable because PHP would actually try to run the JS file as PHP.

[Otto42]

Include is perfectly allowable here, actually. Without a starting <?php tag, then it would be treated as output like HTML.

However, I would suggest that a much more sane way to deal with this issue is to simply compress the script down into a tiny minified string, and directly include that string properly using wp_add_inline_script, attached to some other script handle in the theme which makes the most sense.

Alternatively, I would suggest that there is actually no problem doing code like this:

add_action( 'wp_head', function() { ?>
	<script type="text/javascript" src="example.js" async></script>
<?php } );

With the main reason that being acceptable is because of the use of the async parameter there. WordPress doesn't support the async, and we've allowed things like HTML5 shivs and such before. Twenty Eleven up to Twenty Fourteen all still have a straight up <script> tag for the html5 shiv in the header.php. Not enqueued at all.

For special cases like these, where special handling for things like async is needed, then it's perfectly acceptable to not use the enqueue system.

[dingo-d]

This looks ok, except for the anonymous function as the callback. Adding it like this means that nobody would be able to unhook it, no? At least not in an easy way.

[Otto42]

Well, yes, okay, use a named function. I was just coping justin's code from above.

[westonruter]

I would suggest that a much more sane way to deal with this issue is to simply compress the script down into a tiny minified string

Done in 9d6515e. I left it in a function that runs on the wp_print_footer_scripts action so it can be unhooked if desired. Using wp_add_inline_script() wouldn't be feasible in WP<5.0 since there would be no script to attach it to.

[westonruter]

If you are outputting the scripts tags and JS, why not just do include_once instead of reading the file?

I just found out that this is what Twenty Seventeen is doing for loading the SVG icons:

https://github.com/WordPress/wordpress-develop/blob/5d92e0c0a950399c53294a41000305ddcf47efb5/src/wp-content/themes/twentyseventeen/inc/icon-functions.php#L14-L20

[jrfnl]

Silly question maybe: I can understand why having the JS in a separate file would make maintenance easier, so why was file_get_contents() used in the first place ? Why not just use include ?
If you include a non-PHP file, PHP will just echo it out to the screen, which from all I can see is the intention anyway....

[joyously]

@jrfnl That's what I said 2 days ago, and what Otto said, and what Weston finally agreed when he found an example.

[westonruter]

what Weston finally agreed when he found an example.

I don't actually agree that include should be used. It is better to use file_get_contents() because it doesn't try to evaluate the file as PHP.

[westonruter]

The _print_emoji_detection_script() function in core uses yet another means of inlining JS from a file via the readfile() function, but then also uses the include statement. 🤷‍♂️

In any case, the change in 9d6515e includes a minified copy of the JS in the page for additional reduction on page weight. I think it is very unlikely that this JS file will ever be modified before it gets removed entirely due to IE11 usage dropping off, so I'm happy with the minified version being used.

[jrfnl]

I don't actually agree that include should be used. It is better to use file_get_contents() because it doesn't try to evaluate the file as PHP.

Not true. PHP will not evaluate the file as PHP unless it encounters PHP tags in the file.

From the PHP documentation:

When a file is included, parsing drops out of PHP mode and into HTML mode at the beginning of the target file, and resumes again at the end. For this reason, any code inside the target file which should be executed as PHP code must be enclosed within valid PHP start and end tags.

Ref: http://php.net/manual/en/function.include.php

[westonruter]

Not true. PHP will not evaluate the file as PHP unless it encounters PHP tags in the file.

Right, that's what I mean. There is a possibility for the file to be evaluated if there is <?php in it. If doing file_get_contents() (or readfile()) then there is no possibility of it.

[jrfnl]

There is a possibility for the file to be evaluated if there is <?php in it. If doing file_get_contents() (or readfile()) then there is no possibility of it.

Except that you know the file you are using this on doesn't contain PHP tags, so that doesn't really apply.

[westonruter]

Except that you know the file you are using this on doesn't contain PHP tags, so that doesn't really apply.

Hopefully, yes. However I'm thinking of a scenario where PHP is required to go through strict code review prior to being deployed, but JS lacks the same strictness. If someone were to modify the JS file to add something like /*<?php do_evil(); ?>*/ then that would be bad.

[jrfnl]

If someone were to modify the JS file to add something like /*<?php do_evil(); ?>*/ then that would be bad.

It would be, but just echo-ing out the return from file_get_contents() (as was the original proposal) doesn't actually protect you against that either.

[westonruter]

It would be, but just echo-ing out the return from file_get_contents() (as was the original proposal) doesn't actually protect you against that either.

Why? Just echo-ing out the return value won't result in PHP evaluating it. This is what I was aiming to avoid the danger of.

[jrfnl]

@westonruter That still wouldn't protect against injected JS hacks which can be just as evil.

[westonruter]

Running injected JS is evil for sure, but running injected PHP is much more evil, as it can result in malicious code querying the database, modifying the DB, installing a shell, etc.—in addition to doing all that injected JS can do.

[jrfnl]

Well, if the hacker got access to the file system enough to inject the /*<?php do_evil(); ?>*/ code, they can just add a PHP file to do all that anyway.... ;-)
(and read the wp-config.php file for the passwords etc)

[westonruter]

@kjellr This PR is actually more about performance than code quality.