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

Proposal: Add label argument to register_meta function #7298

Conversation

SantosGuillamot
Copy link

@SantosGuillamot SantosGuillamot commented Sep 5, 2024

With the introduction of Block Bindings, it is more common to see workflows where users need to see the custom fields that are available or connected. Right now, they are relying on the meta key, however, as reported here, it feels too technical sometimes.

In this pull request I'm exploring the possibility of adding a new label argument to include a human-readable name that can be used across the UI.

Trac ticket: https://core.trac.wordpress.org/ticket/61998.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

github-actions bot commented Sep 5, 2024

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@gziolo
Copy link
Member

gziolo commented Sep 5, 2024

@peterwilsoncc, where would be the best place to get feedback on this one? @SantosGuillamot, we need a Trac ticket to milestone it correctly and bring more visibility to WP core contributors. There is also this question for integration with REST API of whether label is a strong enough indicator to automatically expose it in the endpoint and change the show_in_rest to true if no value is provided.

@SantosGuillamot
Copy link
Author

I've started experimenting on Gutenberg consuming this new label: WordPress/gutenberg#65099. It is a good indicator of how it could improve the UX.

we need a Trac ticket to milestone it correctly and bring more visibility to WP core contributors

I'll create a new ticket tomorrow 👌

There is also this question for integration with REST API of whether label is a strong enough indicator to automatically expose it in the endpoint and change the show_in_rest to true if no value is provided.

That's a good question. At first thought, I would be in favor for that, but I didn't think too much about the implications.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gziolo
Copy link
Member

gziolo commented Sep 5, 2024

Looking at the prototype WordPress/gutenberg#65099 that @SantosGuillamot built to showcase how useful it is, I'm motivated to push it forward to make the user experience much more compelling. Presenting labels for post meta over machine-generated keys seems to be an obvious choice for a number of reasons listed in the PR.

@Mamaduka
Copy link
Member

Mamaduka commented Sep 5, 2024

There is also this question for integration with REST API of whether label is a strong enough indicator to automatically expose it in the endpoint and change the show_in_rest to true if no value is provided.

We should consider that all other register_(object) methods require explicitly setting show_in_rest for them to be exposed via REST API. Having different behavior for post-meta would be unexpected, in my opinion.

@gziolo
Copy link
Member

gziolo commented Sep 5, 2024

We should consider that all other register_(object) methods require explicitly setting show_in_rest for them to be exposed via REST API. Having different behavior for post-meta would be unexpected, in my opinion.

Yes, this PR is actually doing exactly that for all types of meta, which I believe is the correct choice. We are also considering exposing more post object types through Block Bindings API (and UI) but there wasn't enough motivation for now.

@SantosGuillamot SantosGuillamot changed the title Try: Add label argument to register_meta function Proposal: Add label argument to register_meta function Sep 6, 2024
@SantosGuillamot
Copy link
Author

I have created the Trac ticket: https://core.trac.wordpress.org/ticket/61998#ticket.

I'm marking this as "Ready for review" because the code is supposed to be ready.

@SantosGuillamot SantosGuillamot marked this pull request as ready for review September 6, 2024 09:54
Copy link

github-actions bot commented Sep 6, 2024

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props santosguillamot, mamaduka, gziolo, timothyblynjacobs, peterwilsoncc.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mamaduka
Copy link
Member

Mamaduka commented Sep 6, 2024

Let's add unit tests to confirm that title from Schema is exposed via the OPTIONS request.

Here's how I did it for settings - https://github.com/WordPress/wordpress-develop/pull/6495/files#diff-af62c99b4a5814e31ddbe08c227ee5d547f97f560fe61d26f42bf58beadc6a79.

P.S. I think we can land this without associated Gutenberg enhancement.

@SantosGuillamot
Copy link
Author

I have added a unit test for title based on the existing one for default. I wasn't sure if they should be unified or it is okay this way. Just let me know 🙂

P.S. I think we can land this without associated Gutenberg enhancement.

I agree they can be handled separated. Now the Gutenberg PR has its own filter to manage it.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me and all PHP Unit tests are passing ✅

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here; I left one nitpick in the test file.

This is fully functional and does the job perfectly for the use case covered in the description.

There is also this question for integration with REST API of whether label is a strong enough indicator to automatically expose it in the endpoint and change the show_in_rest to true if no value is provided.

This is nice to have and can be discussed seperately.

tests/phpunit/tests/meta/registerMeta.php Show resolved Hide resolved
Co-authored-by: Greg Ziółkowski <[email protected]>
@SantosGuillamot
Copy link
Author

Same here; I left one nitpick in the test file.

I just pushed your suggestion 🙂

There is also this question for integration with REST API of whether label is a strong enough indicator to automatically expose it in the endpoint and change the show_in_rest to true if no value is provided.

I've started a separate pull request to discuss the potential implementation of this: #7302

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a nitpick and a question to the tests.

Otherwise I think this looks good & will make the block bindings interface clearer.

tests/phpunit/tests/rest-api/rest-post-meta-fields.php Outdated Show resolved Hide resolved
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So nice to see it close to ready.

@gziolo
Copy link
Member

gziolo commented Sep 16, 2024

@SantosGuillamot, thank you for addressing the remaining feedback. I committed the changes with https://core.trac.wordpress.org/changeset/59023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants