#46195 closed defect (bug) (fixed)
is_block_editor() returns false in the block editor
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (34)
#2
@
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.
#3
@
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
#6
@
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.
#7
@
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).
#9
@
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 throughuse_block_editor_for_post()
and setis_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 tois_block_editor
. - When
edit-form-advanced.php
andedit-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
@
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.
#16
@
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:
- A plugin disables the block editor randomly. This is very unlikely and I don't think we should support this.
- 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 foris_block_editor
100% of the time.
- 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 inpost-new.php
. This could happen if any of the filters or actions withinwp_insert_post()
are used to modify a default post. In most cases, this can also be fixed by adding logic to theuse_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
@
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
@
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
@
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.
#23
@
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
@
5 years ago
- Keywords commit added; 2nd-opinion removed
The tests here are pretty comprehensive, let's get this in.
#26
@
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.
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.