Issue #4678 - Improve tests for 'classic-editor'

[bobbingwide]

Description

When invoking the editor with the classic-editor parameter
we need to avoid enqueueing JavaScript that can break the Visual editor.

Changing the code so that enqueue_block_assets is not invoked for the classic-editor means that plugins don't need to concern themselves about this issue.

How Has This Been Tested?

Manually. See #4678.

Screenshots (jpeg or gifs if applicable):

Types of changes

Bug fix for #4678

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code has proper inline documentation.

[aduth]

Should we consider

[aduth]

PHPCS fails here because a space is needed inside the opening parenthesis:

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#space-usage

[aduth]

Can you explain why this change is necessary? This part of your original comment is not clear to me:

When invoking the classic-editor no parameter value is passed so the isset() test returns false.

Based on this comment, I would expect that an unassigned value is still an empty string, and thus should return true for isset:

http://php.net/manual/en/reserved.variables.get.php#98419

[aduth]

Further, if the logic is complex enough that it warrants a back-and-forth, we may want to extract this out to a common utility method to share between here and the other use.

[bobbingwide]

From PHP manual for isset http://php.net/manual/en/function.isset.php

isset — Determine if a variable is set and is not NULL

[bobbingwide]

I didn't think it was necessary to create a separate method to test for the classic-editor just to solve this problem. However, I've now found other uses of isset() against different locations ie. $_REQUEST as well as $_GET so I imagine there are other problems lurking. I'll investigate further.
Meanwhile, can this change be merged?

[aduth]

From PHP manual for isset http://php.net/manual/en/function.isset.php

isset — Determine if a variable is set and is not NULL

To my original question, when would $_GET['classic-editor'] ever be null?

[bobbingwide]

when you're not using the classic-editor plugin the URL is simply

https://qw/src/wp-admin/post-new.php?classic-editor
or
https://qw/src/wp-admin/post.php?post=629&action=edit&classic-editor

and that causes the value to be null.

I only installed the classic-editor plugin today.
This was the first time I saw a value being passed.

[aduth]

and that causes the value to be null.

But as was explained in the previous link I shared, this is not true.

When the URL is ?classic-editor, the value is still an empty string:

[bobbingwide]

I got in a muddle and there appear to be many more commits but the result is that only lib/client-assets is changed.

[bobbingwide]

And now the code matches the version in aduth's diff. ie. it uses isset()

[aduth]

One other potential issue is that, since this filter is run both for front-end and back-end script enqueues, an unsuspecting front-end visitor could cause blocks requiring scripts to break merely by including ?classic-editor on a page of the site.

Maybe we ought to add this to the condition?

if ( 'admin_enqueue_scripts' === current_filter() && isset( $_GET['classic-editor'] ) ) {

[bobbingwide]

That unsuspecting visitor could be WordPress itself.
See #4409 (comment)

Rather than doing the test inside a multi-use filter function I would consider making the change here

add_action( 'wp_enqueue_scripts', 'gutenberg_common_scripts_and_styles' );
add_action( 'admin_enqueue_scripts', 'gutenberg_common_scripts_and_styles' );

with a new action hook for admin_enqueue_scripts that would make the test
before calling gutenberg_common_scripts_and_styles.

What would this new function be called?

And I'd perform the isset() first since it's slightly quicker than current_filter()
... and which would be unnecessary if using the above approach.

[aduth]

with a new action hook for admin_enqueue_scripts that would make the test
before calling gutenberg_common_scripts_and_styles.

Sounds good.

What would this new function be called?

Maybe gutenberg_admin_scripts_and_styles ?

[aduth]

In my testing, the concern at #4678 (comment) proves to be true for all blocks which don't assign their scripts via the register_block_type API and instead hook directly into enqueue_block_assets, which was the convention prior to the introduction of the editor_script property in #4039

[bobbingwide]

I haven’t tried enqueing scripts using the register_block_type API so don’t know what happens in this instance. Will need to check.
Any block that enqueues editor block assets when it’s not the editor being invoked could break the rest of the application. So, yes, this fix would break blocks that only respond to enqueue_block_assets to enqueue both front end and editor assets.
That’s the whole point of this change. Did you see my summary table of hooks invoked?
Https://github.com/bobbingwide/oik-block/issues/8