Notes: Add auth_callback for wp_note_status comment meta #10430

t-hamano · 2025-10-29T09:00:18Z

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

github-actions · 2025-10-29T09:00:34Z

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 wildworks, adamsilverstein, westonruter, mamaduka.

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

github-actions · 2025-10-29T09:15:34Z

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.

Mamaduka · 2025-10-29T11:02:39Z

Do you have any ideas why unit tests didn't catch this failure? Why isn't test_create_empty_note_with_resolution_meta test failing on trunk?

It would be nice to have unit test coverage for _wp_note_status updated and permissions.

Mamaduka · 2025-10-29T12:23:19Z

I was just doing more tests and realized that running only test_create_empty_note_with_resolution_meta properly fails on trunk, but it passes when running the whole test suite.

Unfortunately, I'm not sure what is causing this different behavior. @desrosj, @TimothyBJacobs, any ideas?

npm run test:php -- --filter test_create_empty_note_with_resolution_meta

t-hamano · 2025-10-29T12:28:19Z

I also noticed this and was in the process of investigating the cause. As a result of this, adding new tests and testing them becomes somewhat complicated 😅

t-hamano · 2025-10-29T12:30:19Z

If the problem cannot be resolved, I plan to split the test into two parts, without using a data provider.

Mamaduka · 2025-10-29T12:34:01Z

If the problem cannot be resolved, I plan to split the test into two parts, without using a data provider.

The test is still passing for me, even after removing the data provider (at least that's the case in the Gutenberg repo). I can only fail it when running the command above.

adamsilverstein · 2025-10-29T13:34:45Z

test_create_empty_note_with_resolution_meta properly fails on trunk, but it passes when running the whole test suite.

Odd, that sounds like some other test is causing a side effect. 👀

adamsilverstein · 2025-10-29T14:09:43Z

When I run the test in isolation, the response error is: Sorry, you are not allowed to edit the _wp_note_status custom field

t-hamano · 2025-10-29T14:23:28Z

When I run the test in isolation, the response error is: Sorry, you are not allowed to edit the _wp_note_status custom field

Yes, for some reason, the comment metadata itself is either not registered or the authentication has failed.

adamsilverstein · 2025-10-29T14:57:35Z

The underlying cause here was the meta was no longer registered after the first test ran - the test controller probably cleans up all meta registration between test runs. Core is adding registering the meta on 'init' and that fires only once per test run. The solution was to add the meta registration to the set_up method, after this change the test fails in isolation and when run with other tests. I added the change to this PR - 27bd29d assuming you are trying to fix this issue, but feel free to remove it if it belongs elsewhere.

Mamaduka · 2025-10-29T15:05:39Z

the test controller probably cleans up all meta registration between test runs.

That's unexpected.

adamsilverstein · 2025-10-29T16:30:13Z

the test controller probably cleans up all meta registration between test runs.

That's unexpected.

Looking into this further, the meta registrations are stored in the $wp_meta_keys global, so the registration must run before the test. Whats unclear to me so far is why the init hook isn't firing for every run, it could have to do with where we are calling add_action( 'init'. I'm looking further.

adamsilverstein · 2025-10-29T17:31:32Z

the test controller probably cleans up all meta registration between test runs.

That's unexpected.

It happens as part of the top level tear down, see

wordpress-develop/tests/phpunit/includes/abstract-testcase.php

Line 457 in 828a3fe

public function unregister_all_meta_keys() {

Therefore, we do need to set this up for each test run, or at least for all tests relying on it being available.

adamsilverstein · 2025-10-29T17:33:19Z

it could have to do with where we are calling add_action( 'init'. I'm looking further.

This wasn't the issue, although I we should probably move that hook to src/wp-includes/default-filters.php like every other core default hook.

and relocate to src/wp-includes/default-filters.php

adamsilverstein · 2025-10-29T17:41:21Z

it could have to do with where we are calling add_action( 'init'. I'm looking further.

This wasn't the issue, although I we should probably move that hook to src/wp-includes/default-filters.php like every other core default hook.

The test suite fires "rest_api_init" on every run, but not init so after the first run, the meta is destroyed and the updates do nothing -

wordpress-develop/tests/phpunit/includes/testcase-rest-controller.php

Line 13 in 828a3fe

do_action( 'rest_api_init', $wp_rest_server );

so moving the hook might fix the issue. After 5501151 tests now pass locally and I see the meta values being set.

adamsilverstein

Fantastic!

…es/js/wp-embed.js` exists

This reverts commit 5501151.

adamsilverstein · 2025-10-29T21:47:00Z

The failures here were actually because REST API fixture generation was failing (not sure when this started, possibly related to commit 8c2ec29). The code in embed.php uses file_get_contents() to read the wp-embed.js file, which doesn't exist in the src directory where tests are run (it's a built asset). It became apparent here because this PR results in a fixture change.

Other oEmbed tests already handle this touch()ing the expected file in their set_up() method to create an empty placeholder file. The REST schema fixture generation test was missing this setup, causing it to fail.

Fixed in 05e386c, I reverted my other changes.

cc: @westonruter fun times

adamsilverstein · 2025-10-29T22:32:48Z

Working in #10434 to avoid too much noise here, I see one other failing test thaat is meta related, once I have those tests green, I'll merge the changes back here

westonruter · 2025-10-29T22:43:49Z

The failures here were actually because REST API fixture generation was failing (not sure when this started, possibly related to commit 8c2ec29). The code in embed.php uses file_get_contents() to read the wp-embed.js file, which doesn't exist in the src directory where tests are run (it's a built asset). It became apparent here because this PR results in a fixture change.

Other oEmbed tests already handle this touch()ing the expected file in their set_up() method to create an empty placeholder file. The REST schema fixture generation test was missing this setup, causing it to fail.

Fixed in 05e386c, I reverted my other changes.

Indeed!

wordpress-develop/tests/phpunit/tests/oembed/controller.php

Lines 38 to 39 in 87cbbb1

    
           // `get_post_embed_html()` assumes `wp-includes/js/wp-embed.js` is present: 
        
           self::touch( ABSPATH . WPINC . '/js/wp-embed.js' );

wordpress-develop/tests/phpunit/tests/oembed/getResponseData.php

Lines 11 to 12 in 87cbbb1

    
           // `get_post_embed_html()` assumes `wp-includes/js/wp-embed.js` is present: 
        
           self::touch( ABSPATH . WPINC . '/js/wp-embed.js' );

wordpress-develop/tests/phpunit/tests/oembed/wpOembed.php

Lines 38 to 39 in 87cbbb1

    
           // `get_post_embed_html()` assumes `wp-includes/js/wp-embed.js` is present: 
        
           self::touch( ABSPATH . WPINC . '/js/wp-embed.js' );

adamsilverstein · 2025-10-29T23:12:47Z

1cba839 switches _wp_note_status meta registration from init to rest_api_init which ensures the it is registered for tests (see

wordpress-develop/tests/phpunit/includes/testcase-rest-controller.php

Line 13 in 828a3fe

do_action( 'rest_api_init', $wp_rest_server );

) - otherwise the test_create_empty_note_with_resolution_meta test only runs once and the meta registration is later deleted (in

wordpress-develop/tests/phpunit/includes/abstract-testcase.php

Line 457 in 828a3fe

public function unregister_all_meta_keys() {

)
a9ffe5c expands test_create_empty_note_with_resolution_meta to check that the meta value is actually set. previously no meta value was set, but the API call was still resulting in a status of 201 so the tests were passing.
2188f3e updates rest-api QUnit test fixtures which is required since we are adding a new meta field. Long story.
31d9ef9 updates another test to include _wp_note_status in the expected results for comment meta.

adamsilverstein · 2025-10-29T23:15:58Z

@westonruter can you please give this another review, see my last comment for an explanation of the additional changes.

adamsilverstein · 2025-10-29T23:20:43Z

@t-hamano Apologies for taking over your ticket, I was trying to help with the failing tests and went down a bit of a rabbit hole. I am done making changes here!

t-hamano · 2025-10-30T04:06:11Z

@adamsilverstein Thank you so much for the detailed investigation!

1cba839 switches _wp_note_status meta registration from init to rest_api_init which ensures the it is registered for tests

My only concern is that with this hook, the timing for registering the comment meta might be a little late. There might be cases where someone wants to access this comment meta before the REST API is initialized.

What are your thoughts on directly executing the callback function within the set_up method? Also, note that +add_action is being moved to default-filters.php.

diff --git a/src/wp-includes/comment.php b/src/wp-includes/comment.php
index acc3379bbe..aa841bfda9 100644
--- a/src/wp-includes/comment.php
+++ b/src/wp-includes/comment.php
@@ -4135,4 +4135,3 @@ function wp_create_initial_comment_meta() {
                )
        );
 }
-add_action( 'rest_api_init', 'wp_create_initial_comment_meta' );
diff --git a/src/wp-includes/default-filters.php b/src/wp-includes/default-filters.php
index 5dc54c3686..f59d25b4fd 100644
--- a/src/wp-includes/default-filters.php
+++ b/src/wp-includes/default-filters.php
@@ -151,6 +151,7 @@ add_filter( 'update_term_metadata_cache', 'wp_check_term_meta_support_prefilter'
 add_action( 'added_comment_meta', 'wp_cache_set_comments_last_changed' );
 add_action( 'updated_comment_meta', 'wp_cache_set_comments_last_changed' );
 add_action( 'deleted_comment_meta', 'wp_cache_set_comments_last_changed' );
+add_action( 'init', 'wp_create_initial_comment_meta' );
 
 // Places to balance tags on input.
 foreach ( array( 'content_save_pre', 'excerpt_save_pre', 'comment_save_pre', 'pre_comment_content' ) as $filter ) {
diff --git a/tests/phpunit/tests/rest-api/rest-comments-controller.php b/tests/phpunit/tests/rest-api/rest-comments-controller.php
index d1e1de9012..7a5109b25d 100644
--- a/tests/phpunit/tests/rest-api/rest-comments-controller.php
+++ b/tests/phpunit/tests/rest-api/rest-comments-controller.php
@@ -170,6 +170,8 @@ class WP_Test_REST_Comments_Controller extends WP_Test_REST_Controller_Testcase
        public function set_up() {
                parent::set_up();
                $this->endpoint = new WP_REST_Comments_Controller();
+               wp_create_initial_comment_meta();
+
                if ( is_multisite() ) {
                        update_site_option( 'site_admins', array( 'superadmin' ) );
                }

adamsilverstein · 2025-10-30T04:21:03Z

Yes that works and aligns with how meta is usually registered so maybe better.

…hook back to init

t-hamano · 2025-10-30T04:34:24Z

I pushed the following changes.

713c73c: In the setup method, I explicitly executed wp_create_initial_comment_meta. At the same time, I moved the hook to default-filters.php.
1c75fb9: I reverted some of the changes because they didn't seem to affect the test results. Are these changes necessary in this pull request?

Mamaduka · 2025-10-30T04:43:35Z

Thanks for debugging this, folks. I think this is good to commit.

Sidenote: The unregister_all_meta_keys makes testing meta values for REST controllers somewhat unpredictable. Do you know if this behavior is documented somewhere?

t-hamano · 2025-10-30T10:12:47Z

May I try to commit this pull request myself? I believe I have the SVN environment set up locally: https://wordpress.slack.com/archives/C18723MQ8/p1761227412356249

I created the following commit message, and I would appreciate it if you could review it.

Editor: Add `auth_callback` to `_wp_note_status` comment meta.

Adds an `auth_callback` to the `_wp_note_status` comment meta so that only users with the `edit_comment` capability can update this meta field via the REST API. 

This is necessary to ensure that users can properly resolve or reopen Notes.

Props wildworks, adamsilverstein, westonruter, mamaduka, desrosj.
Fixes #64153.

Mamaduka · 2025-10-30T10:19:08Z

The commit message looks good to me 👍

t-hamano · 2025-10-30T11:46:29Z

Thank you! I'm going to start preparing for the commit now...

github-actions · 2025-10-30T11:55:15Z

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 61089
GitHub commit: 80156af

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

t-hamano · 2025-10-30T11:56:02Z

The commit appears to have completed successfully 👍

jeffpaul · 2025-10-30T12:57:47Z

Congrats @t-hamano!

adamsilverstein · 2025-10-30T17:56:58Z

Nice work on your first commit to core @t-hamano! 🎉 🎉 🎉

1c75fb9: I reverted some of the changes because they didn't seem to affect the test results. Are these changes necessary in this pull request?

Thats fine; these fix some CI errors I noticed, but they aren't blockers - I can add in a future commit.

Notes: Add auth_callback for wp_note_status comment meta

40103f7

t-hamano marked this pull request as ready for review October 29, 2025 09:00

Mamaduka mentioned this pull request Oct 29, 2025

Notes: Fix PHPUnit tests WordPress/gutenberg#72781

Merged

Mamaduka requested review from Mamaduka and adamsilverstein October 29, 2025 13:25

Ensure note resolution meta is registered before each test

27bd29d

adamsilverstein added 2 commits October 29, 2025 11:35

remove test set up register meta

29c24d7

move wp_create_initial_comment_meta to rest_api_init hook

5501151

and relocate to src/wp-includes/default-filters.php

adamsilverstein approved these changes Oct 29, 2025

View reviewed changes

adamsilverstein added 4 commits October 29, 2025 12:11

Update generated api fixtures

334398f

revert fixture change

4887e1b

get_post_embed_html() used in fixture generation assumes `wp-includ…

05e386c

…es/js/wp-embed.js` exists

Revert "move wp_create_initial_comment_meta to rest_api_init hook"

9643a5e

This reverts commit 5501151.

westonruter approved these changes Oct 29, 2025

View reviewed changes

adamsilverstein added 4 commits October 29, 2025 17:02

Hook meta registration on rest_api_init so it is included for tests

1cba839

Expand test to check that meta is successfully set

a9ffe5c

Fix generation of wp-api fixtures and regnerate with new meta present

2188f3e

iInclude _wp_note_status in expected meta when testing comment meta

31d9ef9

adamsilverstein requested a review from westonruter October 29, 2025 23:15

t-hamano added 2 commits October 30, 2025 13:27

Explicitly register comment meta in unit tests and revert the action …

713c73c

…hook back to init

Revert some changes

1c75fb9

github-actions bot closed this Oct 30, 2025

t-hamano deleted the 64153-note-cap branch October 30, 2025 11:56

Notes: Add auth_callback for wp_note_status comment meta #10430

Notes: Add auth_callback for wp_note_status comment meta #10430

Uh oh!

Conversation

t-hamano commented Oct 29, 2025

Uh oh!

github-actions bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 29, 2025

Test using WordPress Playground

Some things to be aware of

Uh oh!

Mamaduka commented Oct 29, 2025

Uh oh!

Mamaduka commented Oct 29, 2025

Uh oh!

t-hamano commented Oct 29, 2025

Uh oh!

t-hamano commented Oct 29, 2025

Uh oh!

Mamaduka commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025

Uh oh!

t-hamano commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mamaduka commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsilverstein commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsilverstein left a comment

Choose a reason for hiding this comment

Uh oh!

adamsilverstein commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025

Uh oh!

westonruter commented Oct 29, 2025

Uh oh!

adamsilverstein commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsilverstein commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsilverstein commented Oct 29, 2025

Uh oh!

t-hamano commented Oct 30, 2025

Uh oh!

adamsilverstein commented Oct 30, 2025

Uh oh!

t-hamano commented Oct 30, 2025

Uh oh!

Mamaduka commented Oct 30, 2025

Uh oh!

t-hamano commented Oct 30, 2025

Uh oh!

Mamaduka commented Oct 30, 2025

Uh oh!

t-hamano commented Oct 30, 2025

Uh oh!

github-actions bot commented Oct 30, 2025

Uh oh!

t-hamano commented Oct 30, 2025

Uh oh!

jeffpaul commented Oct 30, 2025

github-actions bot commented Oct 29, 2025 •

edited

Loading

adamsilverstein commented Oct 29, 2025 •

edited

Loading

adamsilverstein commented Oct 29, 2025 •

edited

Loading

adamsilverstein commented Oct 29, 2025 •

edited

Loading

adamsilverstein commented Oct 29, 2025 •

edited

Loading

adamsilverstein commented Oct 29, 2025 •

edited

Loading