Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom fields to be hidden when enableCustomFields is null #33912

Closed
wants to merge 1 commit into from

Conversation

mreishus
Copy link
Contributor

@mreishus mreishus commented Aug 5, 2021

Allow custom fields to be hidden when enableCustomFields is null

Scope of Problem

This is a coordination problem between WordPress and Gutenberg and requires changes on both sides. I have patches for both.

Problem

When using a plugin to disable Custom Fields in the post editor, WordPress attempts to change $editor_settings to tell Gutenberg not to render the Custom Fields settings toggle in a preferences modal. However, Gutenberg is listening for a value that is impossible to send in PHP, and the settings toggle remains visible even when the feature itself is disabled.

This change tells Gutenberg to listen for an additional value to turn off the setting (null). It must be applied in conjunction with a WordPress change to send that value (Currently, it uses unset() to not send a value, which mean Gutenberg's state defaults apply).

Alternative Approach

Here is a different approach for the same problem: #33931 This changes the default value to undefined, which allows WP.org's current unset() to work without any changes needed on that side.

Steps to Reproduce

  1. Have a WordPress.org installation.

  2. Create wp-content/plugins/disable-custom-fields.php

<?php
/**
 * Plugin Name: Disable Custom Fields
 * Plugin URI: http://www.mywebsite.com/disable-custom-fields
 * Description: Disable Custom Fields
 * Version: 1.0
 * Author: Your Name
 * Author URI: http://www.mywebsite.com
 */

add_action( 'add_meta_boxes', 'plugin_disable_custom_fields', 1 );

function plugin_disable_custom_fields() {
    remove_post_type_support( get_post_type(), 'custom-fields' );
}


add_action( 'do_meta_boxes', 'plugin_meta_boxes', 10, 2 );

function plugin_meta_boxes( $page, $context ) {
    remove_meta_box( 'postcustom', get_post_type(), 'normal' );
}
  1. Visit /wp-admin and activate this plugin.
  2. Visit post editor on WordPress.org site. With this plugin enabled, this if condition will always trigger, and the unset( $editor_settings['enableCustomFields'] ); always runs.
  3. Click the three dots menu in the top right
  4. Click "preferences" at the bottom
    2021-08-05_10-07
  5. A modal appears. Click Panels on the left
  6. Turn on "Custom Fields"
    2021-08-05_10-08
    2021-08-05_10-08_1
  7. See message asking to enable and reload, click the button
  8. Page reloads
  9. Re-visit the custom fields setting, see that it is turned off.

2021-08-02_13-33_1

Expected to see: The entire "Custom Settings" option does not appear, because $editor_settings['enableCustomFields'] is unset by this core code.

Actually see: A toggle that does nothing after turning it on, because a plugin is disabling the option from working.

The "Custom Fields" settings is designed to be hidden by an editor setting set in WP.org core PHP code, however it cannot be hidden, due to these factors:

  • gutenberg JS strictly checks for an undefined value: Code Link 1, Code Link 2
  • If PHP sends a null or false value, those don't match a === check against undefined
  • If PHP does not send a value, gutenberg's default settings apply a false to the value, which is also !== undefined. Code Link 3.

Therefore, to hide the "Custom Fields" setting, it wants an undefined to be sent in getEditorSettings().enableCustomFields, but it is impossible to send one.

Description

How has this been tested?

  • Used a wordpress.org installation
  • Added the simple plugin described in my steps above
  • cp wp-includes/js/dist/edit-post.js wp-includes/js/dist/edit-post.min.js
  • Edited wp-includes/js/dist/edit-post.min.js
  • Also applied changes to WP.org core code. This is a coordination problem between WordPress and Gutenberg and requires changes on both sides to work.
  • Don't laugh, I don't usually work on Gutenberg and don't have a dev environment set up for it 😄

Screenshots

Types of changes

  • Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@getsource
Copy link
Member

getsource commented Aug 6, 2021

Thanks so much @mreishus for the outstanding bug report, PR, and description of the issue!

I tested this PR (along with making the change in core, and the provided plugin), and it works as described.

I looked into this a little bit, and I’m wondering if instead of changing what core passes, if it would be possible to change the default to undefined as was recommended by @noisysocks here: #16338 (review). What do you think?

@youknowriad do you remember any context on why the default for enableCustomFields was set to false in c29b729? It's possible I'm missing the description somewhere, so apologies if so!

On a related note, I noticed that if the last item in the “Additional” section is removed, it leaves the header with no options, so that might some something to look into as well (maybe in a separate PR?):

Screen Shot 2021-08-06 at 22 06 17

@getsource getsource added [Feature] Meta Boxes A draggable box shown on the post editing screen [Type] Bug An existing feature does not function as intended labels Aug 6, 2021
@mreishus
Copy link
Contributor Author

mreishus commented Aug 6, 2021

I looked into this a little bit, and I’m wondering if instead of changing what core passes, if it would be possible to change the default to undefined as was recommended by @noisysocks here: #16338 (review). What do you think?

  • ✔️ In my testing, this fixes the issue as well.
  • ✔️ Additionally, it does not require any changes on the WP.org side.
  • ✔️ It sticks to the true/false/undefined triad previously established.
  • ❔ I was worried about changing the behavior of anything that relies on the default behavior. Since WP.org out of the box sets enableCustomFields, maybe this not an issue. It would have to be something that uses gutenberg outside of the context of WP.org, which admittedly I don't know much about. And it might be that changing the default behavior is the right thing, perhaps those other contexts are having the same problem as well.

I created a PR using that approach here: #33931

If we go with that method, let me know so I can cancel my WP.org changes. Changing the default to undefined won't help if WP.org starts sending a null. :)

@mreishus mreishus force-pushed the fix/hide-custom-fields branch from 152d7cf to 890b984 Compare August 9, 2021 14:44
@youknowriad
Copy link
Contributor

@youknowriad do you remember any context on why the default for enableCustomFields was set to false in c29b729? It's possible I'm missing the description somewhere, so apologies if so!

I don't remember much tbh, I think that PR was just a refactoring PR separating the "editor" package from the "block-editor" one. That said, packages like "block-editor" and "editor" can be seen as WP independent meaning the default value in the JS store could be different from the one that is provided by WP's backend and that's fine.

@getsource
Copy link
Member

getsource commented Aug 16, 2021

That said, packages like "block-editor" and "editor" can be seen as WP independent meaning the default value in the JS store could be different from the one that is provided by WP's backend and that's fine.

@youknowriad Thanks for the context re: the PR!

I'm sorry, I don't think I completely understand what you mean on the suggestion. Do you mean you think it's fine to move to undefined as default, or it'd be a good idea to consider a different way of storing the state (like using something other than undefined)?

@youknowriad
Copy link
Contributor

I was just saying that it's fine if the default value for the block-editor package (default.js file) is different from the default that is sent from the backend when calling wp.editPost.initializeEditor. the latter will have more priority in WP.

For the fix about this particular issue, I honestly don't know what's best. Looking at the check, I do wonder why null false or undefined have a different meaning here. It can be the same thing no?

@noisysocks
Copy link
Member

For the fix about this particular issue, I honestly don't know what's best. Looking at the check, I do wonder why null false or undefined have a different meaning here. It can be the same thing no?

  • true means that the user has opted to show the Custom Fields panel at the bottom of the editor.
  • false means that the user has opted to hide the Custom Fields panel at the bottom of the editor.
  • undefined means that the current theme (or whatever) doesn't support Custom Fields and so the user shouldn't have a choice in the matter.

#33931 looks like the right fix to me.

We could perhaps make this clearer by replacing true, false and undefined with 'on', 'off' and 'unsupported'. That requires a core change though. Probably easier to just add an explanatory comment wherever possible.

@mreishus mreishus force-pushed the fix/hide-custom-fields branch from 890b984 to 1d0dabb Compare August 19, 2021 19:12
@mreishus
Copy link
Contributor Author

Closing this one since we went with the alternate approach: #33931

@mreishus mreishus closed this Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Meta Boxes A draggable box shown on the post editing screen [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EDITOR_SETTINGS_DEFAULTS makes it impossible to hide the custom fields toggle
4 participants