-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update: Footnotes support for custom post types to be support based. #58573
Update: Footnotes support for custom post types to be support based. #58573
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/blocks.php |
6a1a7b9
to
9527c63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a much nicer approach.
To ensure it doesn't hit the order of operations issue in #58341 once it's merged in to WordPress-Develop, are you able to hack the relevant file in WordPress and test it without Gutenberg active?
// Registers the footnotes meta field for post types that support it. | ||
add_action( 'init', '_gutenberg_register_footnotes_meta_field', 100 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note explaining this runs late to allow for plugins registering post types would be helpful here.
@spacedmonkey @TimothyBJacobs does init, 100
run in the REST API or does rest_api_init
exit beforehand on init, 10
?
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNCore Committers: Use this line as a base for the props when committing in SVN:
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having sat on this for a while...
I think it might be better to require developers opt-in (either via supports => footnotes
or add_post_type_support()
.
Making it opt-out will require extenders not wishing to support footnotes register their CPTs with the following code. Due to WP's commitment to back compat, we'll be stuck with it.
register_post_type( 'my_cpt', $args );
remove_post_type_support( 'my_cpt', 'footnotes' );
Making it opt-in will require extenders wanting to support footnotes make a single code change to registering their post types. Versions of WP that don't support footnotes will simply ignore the feature.
I think I suggested making it opt-out initially, sorry about that.
Co-authored-by: Peter Wilson <[email protected]>
Tbh, I think as footnotes as just content and it should remain opt-out. The footnotes meta is just an implementation detail, we might as well have stored footnotes in the post content field. Footnotes should imo be thought of as any other block, which you can disable (globally or per post type) with the current block APIs and filters. For really advanced cases where you really don't want the meta to be registered, you can deregister meta and then footnotes will also get disabled without error, but I don't think it should be the way to do it, nor do I think we should move this into post type supports. Btw, we might even have an option in the future where footnotes just get saved to the post content if there's no meta field, which is useful for example in standalone GB instances. |
It is a good point @ellatrix, I guess it makes sense to not treat footnotes differently from any other block. So the way to disable footnotes in a post type would be:
The same code can be used to disable any other block in a specific post type. There was a bug where the disabling of the format was not working as expected that I'm fixing at #58855. Following your suggestion I'm going to close this PR. |
Making them opt-out via the block APIs works for me as blocks are already opt-out. |
This all makes sense to me too, I've flipped my opinion on the matter. 👍 |
Update to #57353.
Fixes: #58341
Alternative to #58343.
Follows @mcsf suggestion in #57353 (comment) and makes footnotes have their own supports key.
By default, every post that meets footnotes requirements has support for footnotes.
Developers can now remove footnotes support with:
cc: @mcsf, @peterwilsoncc, @ellatrix
Testing
Added the following post type:
I have confirmed that the footnotes are functioning correctly on the specified post type.
Furthermore, I have removed the "custom-fields" support from the post type and confirmed that the footnotes are no longer available on the post type.
In addition, I have verified that
remove_post_type_support( 'page', 'footnotes' );
successfully removes footnotes support from pages.