WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#32118 closed defect (bug) (fixed)

SCRIPT_DEBUG check in print_emoji_detection_script() generating PHP Notices

Reported by: aut0poietic Owned by: craig-ralston
Milestone: 4.2.3 Priority: normal
Severity: normal Version: 4.2
Component: Formatting Keywords: fixed-major dev-feedback needs-patch
Focuses: Cc:

Description

Some simple (or poorly built) WordPress themes generate the following PHP notice in 4.2:

Notice: Use of undefined constant SCRIPT_DEBUG - assumed 'SCRIPT_DEBUG' in /var/www/html/wordpress/docs.oucpm.org/wp-includes/formatting.php on line 4144

In addition to the notice, the emoji script block is printed using the expanded (rather than minified) form.

The Notice is triggered when the active theme does not enqueue any scripts (I know, right?).

Unless added to the config.php, SCRIPT_DEBUG is not defined until a script is enqueued (in wp-includes/script-loader.php).

The the two issues can be resolved by checking to ensure SCRIPT_DEBUG is defined ( as is done in other files that use SCRIPT_DEBUG).

Current:

//wp-includes/formatting.php:4144
if ( SCRIPT_DEBUG ) {
...

Fix:

wp-includes/formatting.php:4144
if ( defined('SCRIPT_DEBUG') && SCRIPT_DEBUG ) {
...

Edge case it is, but googling has turned up a pretty sizable chunk of these.

Attachments (3)

32118.diff (558 bytes) - added by Craig Ralston 3 years ago.
define SCRIPT_DEBUG in wp_initial_constants()
32118-2.diff (551 bytes) - added by Craig Ralston 3 years ago.
32118-3.diff (553 bytes) - added by Craig Ralston 3 years ago.
correcting spacing from 32118-2.diff

Download all attachments as: .zip

Change History (22)

#1 @azaozz
3 years ago

  • Milestone changed from Awaiting Review to 4.2.1

Wondering if we should define SCRIPT_DEBUG the same way we define WP_DEBUG in wp_initial_constants(). Probably not a good idea for a dot release though.

#2 @obenland
3 years ago

  • Keywords good-first-bug needs-patch added

@Craig Ralston
3 years ago

define SCRIPT_DEBUG in wp_initial_constants()

#3 @Craig Ralston
3 years ago

  • Keywords has-patch added; needs-patch removed

#4 @DrewAPicture
3 years ago

  • Owner set to craig-ralston
  • Status changed from new to assigned

#5 @azaozz
3 years ago

  • Keywords has-patch removed

32118.diff is not going to work properly. See how it is defined now in script-loader.php: https://core.trac.wordpress.org/browser/tags/4.2/src/wp-includes/script-loader.php#L51 (defaults to true when running from /src).

The question is: can this possibly conflict with something a plugin would do if we move it to wp_initial_constants()?

#6 @nacin
3 years ago

Let's not deal with early defining SCRIPT_DEBUG for now. I am almost positive we don't do that because of the weirdness of script-loader.php loading.

For 4.2.1, the extra defined() && is good.

Question: what happens when this bug occurs? What happens when the notice is suppressed vs not suppressed? Does the JS break?

#7 @iseulde
3 years ago

  • Keywords needs-patch added

#8 @Craig Ralston
3 years ago

I wasn't able to replicate any JS actually breaking from the notice not being suppressed. @aut0poietic if you experienced different results, please feel free to chime in.

32118.diff didn't include the full move from script-loader.php, my fault. 32118-2.diff just adds the extra defined('SCRIPT_DEBUG') && and stays out of default-constants.php

#9 @iseulde
3 years ago

  • Keywords has-patch added; needs-patch removed

@Craig Ralston Could you update the patch so that it follows the coding standards?
https://make.wordpress.org/core/handbook/coding-standards/php/#space-usage

@Craig Ralston
3 years ago

correcting spacing from 32118-2.diff

#10 @azaozz
3 years ago

What happens when this bug occurs? What happens when the notice is suppressed vs not suppressed? Does the JS break?

There is an unsightly PHP notice at the top of the page when the server is misconfigured.

The JS doesn't break but the non-minified, non-concatenated JS is used (as the undefined constant is treated as string, evaluates to true). This loads two files asynchronously instead of one, making it noticeable slower. Happens when the notice is suppressed and not suppressed.

Last edited 3 years ago by azaozz (previous) (diff)

#11 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 32296:

Check if SCRIPT_DEBUG is defined before using it in print_emoji_detection_script().

props Craig Ralston.
fixes #32118 for trunk.

#12 @SergeyBiryukov
3 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#13 @azaozz
3 years ago

Now this breaks when running from /src :)

Testing with a theme that doesn't enqueue scripts or styles, and with SCRIPT_DEBUG not defined in wp-config.php, there is an "Uncaught SyntaxError: Unexpected string" because the "build" version of the script is outputted: https://core.trac.wordpress.org/browser/tags/4.2/src/wp-includes/formatting.php#L4175.

Perhaps we can keep the above fix for 4.2 but add something better in trunk.

This ticket was mentioned in Slack in #core by azaozz. View the logs.


3 years ago

#15 @DrewAPicture
3 years ago

  • Keywords dev-feedback needs-patch added; good-first-bug has-patch removed

Looks like we're going to need a new patch, see comment:13.

#16 @samuelsidler
3 years ago

  • Milestone changed from 4.2.2 to 4.2.3

#17 @azaozz
3 years ago

In 32482:

Check if running from /src or SCRIPT_DEBUG is defined in print_emoji_detection_script().
See #32118.

#18 @SergeyBiryukov
3 years ago

#32461 was marked as a duplicate.

#19 @azaozz
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 33315:

Check if running from /src or SCRIPT_DEBUG is defined in print_emoji_detection_script().
Fixes #32118 for 4.2.

Note: See TracTickets for help on using tickets.