Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#46195 closed defect (bug) (fixed)

is_block_editor() returns false in the block editor

Reported by: chouby's profile Chouby Owned by: desrosj's profile desrosj
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.0
Component: Editor Keywords: has-patch has-unit-tests has-dev-note
Focuses: administration Cc:

Description

The current_screen action is supposed to be fired after the necessary elements to identify a screen are set up. So I would expect that $screen->is_block_editor() returns the right value in a function hooked to current_screen. But in fact $screen->is_block_editor() always return false even if are in the block editor.

That's because the property is_block_editor is set much later, after current_screen action is fired.

This small code snippet allows to reproduce the issue:

add_action( 'current_screen', function( $screen ) {
  error_log( var_export( $screen->is_block_editor(), true ) );
} );

Attachments (7)

46195.diff (2.1 KB) - added by desrosj 6 years ago.
46195.2.diff (2.3 KB) - added by Chouby 6 years ago.
46195.3.diff (2.6 KB) - added by desrosj 5 years ago.
46195.4.diff (3.0 KB) - added by desrosj 5 years ago.
46195.5.diff (2.4 KB) - added by desrosj 5 years ago.
46195.6.diff (3.0 KB) - added by desrosj 5 years ago.
46195.7.diff (16.3 KB) - added by desrosj 5 years ago.

Download all attachments as: .zip

Change History (34)

#1 @desrosj
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.1.1

Thanks for this, @Chouby! I am going to milestone this for 5.1.1 as this would be too late in the 5.1 timeline with RC set for tomorrow.

I have a patch incoming that will fix the issue, but it needs to be well tested for any adverse effects.

@desrosj
6 years ago

#2 @desrosj
6 years ago

  • Focuses administration added
  • Keywords has-patch needs-testing added; needs-patch removed

46195.diff moves the logic that sets the is_block_editor property into the WP_Screen::get() method so it is properly set when the current_screen action is triggered.

I tested with the Classic Editor plugin active, and all the different combinations of plugin/user settings and user flows and each scenario worked correctly for me.

@Chouby
6 years ago

#3 @Chouby
6 years ago

I tested the patch and, for me, it doesn't work for new posts. Because the post ID is created later in post-new.php. 46195.2.diff aims to fix this.

However this is not fully backward compatible as use_block_editor_for_post_type won't honor the filter use_block_editor_for_post for new posts.

This ticket was mentioned in Slack in #core-editor by lukecarbis. View the logs.


5 years ago

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


5 years ago

@desrosj
5 years ago

#6 @desrosj
5 years ago

Thanks for spotting that, @Chouby. I think that 46195.3.diff would solve the issue with new posts in a backward compatible way. If, after a new post is created, use_bloc_editor_for_post() returns true, the is_block_editor property can be updated. This is similar to what is happening now, just moved down and only when creating a new post.

@desrosj
5 years ago

#7 @desrosj
5 years ago

After giving it a bit more thought, I think including the changes in 46195.2.diff makes sense. This would catch:

  • A situation where someone loads wp-admin/edit-form-blocks.php on their own.
  • Establish the default value for the post type in the current screen early.
  • The block editor can still be switched with use_block_editor_for_post for new posts after the post is created.

The downfall of both these approaches, though, is that the support indicated when current_screen is fired and later on in the process that value could be different (which is actually an issue in the current state). This can be avoided by using the blanket use_block_editor_for_post_type filter whenever possible (not filtering dependent on data specific to an existing post).

Last edited 5 years ago by desrosj (previous) (diff)

#8 @desrosj
5 years ago

  • Owner set to desrosj
  • Status changed from new to assigned

@desrosj
5 years ago

#9 @desrosj
5 years ago

I worked through this with @earnjam today to make sure there were no scenarios being missed, and that we are approaching this correctly.

Here is how the code works in 46195.5.diff:

  • If a post is being edited, WP_Screen::get() will run the post through use_block_editor_for_post() and set is_block_editor to the function's return value.
  • If a new post is being edited, the value returned by use_block_editor_for_post_type for the post type will be set to is_block_editor.
  • When edit-form-advanced.php and edit-form-blocks.php are included, is_block_editor is updated to ensure it is the correct value.

Looking at the Classic Editor and Gutenberg Ramp plugins, they both seem to rely on the use_block_editor_for_post filter, which, as @Chouby noted above, is only run in use_block_editor_for_post() (not in use_block_editor_for_post()). This means that when a WP_Screen instance is created when creating a new post, is_block_editor could incorrectly indicate whether the page is using the block editor.

The steps noted in the third bullet point above ensure that WP_Screen has the correct value, but between the current_screen action and the corresponding edit form file is included, the value could be incorrect. Due to the current order code is executed for an "add new post" request, I am not sure if there is a better way.

The third bullet point would also catch any scenarios where the edit advanced/block form files are being included manually.

@azaozz I'd love your thoughts on this approach as it relates to the Classic Editor.

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


5 years ago

This ticket was mentioned in Slack in #core-editor by pbiron. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-editor by youknowriad. View the logs.


5 years ago

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


5 years ago

#14 @aduth
5 years ago

Does the assignment need to account for the replace_editor filter? Prior to this most recent patch, if a plugin applied a filter to replace_editor, then is_block_editor would be false even if use_block_editor_for_post were true.

It seems awfully non-conventional, but I wondered if there might be an option to assign WP_Screen::is_block_editor prior to set_current_screen being called. Pseudo-code: https://gist.github.com/aduth/6459c96ffbb03f458af1a051473da817

When edit-form-advanced.php and edit-form-blocks.php are included, is_block_editor is updated to ensure it is the correct value.

It doesn't appear the patch includes changes to edit-form-blocks.php? I'm trying to follow the explanations provided, but it still seems like it's accounting for a lack of confidence in the accuracy of the WP_Screen::get() implementation.

#15 @lukecarbis
5 years ago

  • Milestone changed from 5.1.1 to 5.1.2

@desrosj
5 years ago

#16 @desrosj
5 years ago

  • Keywords 2nd-opinion added

Ah, you are right. 46195.5.diff did not account for the replace_editor filter.

46195.6.diff adds support for plugins using this filter to replace the block editor.

The same issue exists as described above for this filter, though. Because the replace_editor filter and use_block_editor_for_post() function require the WP_Post instance to be passed, WP_Screen::get() can only make its best guess as to whether the block editor is used based on the default value for the post type being created. So between the current_screen action and roughly the load-{$pagenow} action (which, in this case, would be load-post-new), the value could be incorrect.

These are the only scenarios I can think of where this edge case would be triggered:

  1. A plugin disables the block editor randomly. This is very unlikely and I don't think we should support this.
  1. The block editor is disabled for a user with or without a specific capability or meta value. This would cause the value to be inaccurate during the time frame detailed above. But, this could be corrected by using the use_block_editor_for_post_type filter. This is not specific to a post, so by using this filter, you are still able to achieve an accurate value for is_block_editor 100% of the time.
  1. The filter checks for the presence of a certain string in a post title or content, or a specific term or meta value to be assigned to a post. This would happen only if actions were being performed when the post is created using get_default_post_to_edit(), which is how the post is created in post-new.php. This could happen if any of the filters or actions within wp_insert_post() are used to modify a default post. In most cases, this can also be fixed by adding logic to the use_block_editor_for_post_type filter to detect these scenarios before the post is actually created.

All of these seem like rare edge cases, but they all already exist today. They are just buried among much more common scenarios. The changes in 46195.6.diff would greatly narrow the window where is_block_editor could be incorrect, but these edge cases would still exist.

The patch does not include changes to edit-form-blocks.php because the code already exists in that file to ensure the current screen is correctly flagged as using the block editor.

Also, I get very nervous changing the load order of the admin area. I don't think we can confidently do this in a fully backwards comaptible way.

This ticket was mentioned in Slack in #meta by sergey. View the logs.


5 years ago

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


5 years ago

#19 @desrosj
5 years ago

  • Keywords needs-dev-note added

Marking this as needs-dev-note so the community can be informed of any edge cases and best practices for replacing filtering the block editor.

This ticket was mentioned in Slack in #core-editor by aduth. View the logs.


5 years ago

#21 @johnbillion
5 years ago

This looks like something that could do with test coverage, but at the same time it's not very testable code. Thoughts?

#22 @desrosj
5 years ago

  • Keywords needs-unit-tests added

I agree on both accounts. I'll take a stab at simulating some of the different admin screens and ensuring the correct value as best I can.

@desrosj
5 years ago

#23 @desrosj
5 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

46195.7.diff adds some unit tests as best we can (I think). It also adds filter inline documentation for replace_editor in WP_Screen that I had missed.

#24 @johnbillion
5 years ago

  • Keywords commit added; 2nd-opinion removed

The tests here are pretty comprehensive, let's get this in.

#25 @desrosj
5 years ago

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

In 45224:

Administration: Improve the accuracy of is_block_editor().

Currently, there are a number of scenarios where is_block_editor() (and WP_Screen::is_block_editor) would incorrectly indicate block editor support at different points of the loading process. Most notably, checking is_block_editor when hooking into the current_screen action will always result in false, even when the block editor is being loaded. This is because is_block_editor is not set to true until edit-form-blocks.php is included.

This change adds logic to WP_Screen to ensure the accuracy of is_block_editor on block editor pages earlier in the load process.

While edit screens will now be accurate 100% of the time from current_screen on, there are still a few edge cases where is_block_editor could contain an incorrect value when creating a new post.

Because a WP_Post object is a required parameter for the replace_editor filter and use_block_editor_for_post() function, WP_Screen will fall back to the value returned by use_block_editor_for_post_type() for the post being created. To eliminate these edge cases, the use_block_editor_for_post_type filter can be used to return the appropriate boolean value to indicate support.

Props Chouby, desrosj, aduth, johnbillion.
Fixes #46195.

#26 @desrosj
5 years ago

  • Keywords needs-testing commit removed
  • Milestone changed from 5.1.2 to 5.2

Since there are no current plans for 5.1.2, let's leave this in 5.2. If that changes, we can reopen.

Note: See TracTickets for help on using tickets.