Skip to content

Comments: Add a per-action lock for notes - #13028

Open
adamsilverstein wants to merge 1 commit into
WordPress:trunkfrom
adamsilverstein:add/notes-locking
Open

Comments: Add a per-action lock for notes#13028
adamsilverstein wants to merge 1 commit into
WordPress:trunkfrom
adamsilverstein:add/notes-locking

Conversation

@adamsilverstein

Copy link
Copy Markdown
Member

Claude wrote the code and this description; the plan and review are mine:

Backports note locking from Gutenberg WordPress/gutenberg#81546, which implements WordPress/gutenberg#72724.

Trac ticket: to be filed. See the open questions at the bottom first - the filter and meta names are what get locked in.

Upstream status: the Gutenberg PR is open and should land first, so the plugin can defer to core through its function_exists( 'wp_note_action_is_locked' ) guard.

What

Notes shipped in 6.9 with no way to freeze one. Anyone who can edit the post can edit or delete any note on it, and since there is no admin UI for notes, a deleted note - and its whole reply thread - is gone for good. Editorial flows that need an audit trail, agency sign-off, or a discussion frozen after publish have nothing to reach for.

This adds a lock that leaves notes readable but stops them changing. It is expressed per action - create, reply, edit, resolve, delete - so a site can either freeze a post's notes entirely or preserve them selectively, for example by disallowing deletion while review carries on.

How

One predicate. wp_note_action_is_locked( $action, $post, $comment ) reads a _wp_notes_locked post meta and then runs the note_action_is_locked filter over it, passing the action name.

// Freeze notes everywhere.
add_filter( 'note_action_is_locked', '__return_true' );

// Preserve all notes: nobody deletes, everything else carries on.
add_filter( 'note_action_is_locked', function ( $locked, $action ) {
	return 'delete' === $action ? true : $locked;
}, 10, 2 );

// ...unless they are an administrator.
add_filter( 'note_action_is_locked', function ( $locked, $action ) {
	return 'delete' === $action && current_user_can( 'manage_options' ) ? false : $locked;
}, 10, 2 );

Enforcement in the controller's permission checks. WP_REST_Comments_Controller::create_item_permissions_check(), update_item_permissions_check() and delete_item_permissions_check() each consult the lock and return a rest_notes_locked 403. Two new private methods do the work:

  • get_note_request_actions() classifies the request. A DELETE is delete. A create carrying _wp_note_status meta is resolve, one with a parent is reply, otherwise create. On an item route, sending content is edit and changing status is resolve - resending the status a note already has mutates nothing, so it does not count. Anything else on the note (author, date, meta) falls back to edit.
  • check_note_lock_permission() runs the predicate over those actions.

check_edit_permission() is deliberately left alone. It is shared by update, delete and the read path, and it never learns which action is underway, so a per-action lock cannot live there.

Who may lock. Whoever can write the meta: edit_others_posts plus edit_post on the target, enforced by the meta's auth_callback. Editors and up, not the post's author alone - an author locking reviewers out of their own review thread would defeat the point. No new capabilities.

The editor follows along. get_block_editor_settings() publishes a lockedNoteActions array for the post being edited. The Gutenberg side reads it and hides or disables what the server will refuse. That is a courtesy only; every mutation is enforced here.

Two supporting pieces:

  • _wp_post_type_supports_notes() in post.php reads the notes flag out of the editor support arguments. WP_REST_Comments_Controller had this inline as a private method; it now needs a second caller, so it moves out and the private duplicate goes away.
  • wp_register_note_lock_meta() registers _wp_notes_locked on every post type that supports notes, hooked to init at priority 20 so post types registered at the default priority are already in place.

Scope

The gate is REST-scoped. Direct wp_update_comment() / wp_delete_comment() calls, XML-RPC, and wp-admin bulk actions are not covered.

That was a deliberate call rather than an oversight. There is no symmetric seam: wp_update_comment_data can return a WP_Error, but comment deletion has no equivalent short-circuit filter, so enforcing at the data layer would produce a lock that stops edits and not deletions - exactly backwards from the case that motivated the feature. Enforcing there would also block legitimate programmatic work like migrations and CLI cleanup. Notes are created and mutated exclusively through REST by the editor, and they are excluded from the default admin comment queries, so the REST layer is the whole surface a user can reach. Worth revisiting if a note admin UI arrives.

Lock management UI is also out of scope. This is the enforcement layer a toggle can later sit on.

Testing instructions

  1. Create a post and add a couple of notes to it in the editor.
  2. Lock it: wp post meta update <post-id> _wp_notes_locked 1
  3. Reload the editor. The notes still read and navigate; nothing offers to change them.
  4. Confirm reading is untouched: GET /wp/v2/comments?post=<post-id>&type=note&status=all still returns the notes.
  5. Confirm the server is doing the work rather than the editor: with the editor open, lock the post from another window, then delete a note. The request is refused with a rest_notes_locked 403.
  6. Drop in the delete-only filter above and confirm creating, replying, editing and resolving all still work while deleting does not.
  7. Unlock and confirm nothing has changed from trunk.

Run the tests:

npm run test:php -- --filter Tests_REST_Notes_Locking

Open questions

Carried over from the Gutenberg PR, worth settling before the names fossilise:

  1. Should a full lock block resolving, or is "frozen except resolution" the better default for a sign-off flow? It currently blocks; sites can unfreeze it with one filter line.
  2. Who may write _wp_notes_locked - edit_others_posts (as here), manage_options, or plain edit_post?
  3. Should administrators bypass locks by default, or is "express it through the filter" enough?
  4. Filter shape: a boolean note_action_is_locked( $locked, $action, $post, $comment ) (as here) or an array-returning notes_locked_actions( $actions, $post )? The boolean composes better for capability carve-outs.
  5. Naming: _wp_notes_locked / note_action_is_locked / rest_notes_locked / lockedNoteActions.
  6. @Mamaduka has argued upstream that this belongs in a comment-type registrar (#35214). The contract here is deliberately small - one predicate, one filter, no new capabilities - so it can migrate into a registrar later. Ship now, or wait?

Tests

tests/phpunit/tests/rest-api/rest-notes-locking.php - 12 tests, 81 assertions, driving real REST dispatches so the permission checks actually run:

  • the unlocked baseline across every mutation, including the resolution marker note;
  • the meta lock rejecting create, reply, resolve-marker, edit, resolve, trash and delete;
  • administrators bound by the lock;
  • ordinary comments and note reads left alone;
  • the filter locking site-wide, locking one action, and carving out a capability, plus assertions on the arguments it receives;
  • the meta's auth_callback refusing an author and accepting an editor;
  • the meta registering only for post types that support notes;
  • unresolvable targets left to the controller;
  • lockedNoteActions reflecting both the meta and the filter.

@ticket annotations still need adding once the ticket exists.

Notes can currently be edited and deleted by anyone who can edit the post,
with no way to freeze a discussion or preserve an audit trail. There is no
admin UI for notes, so a deleted note and its whole reply thread are gone
for good.

Introduce wp_note_action_is_locked(), which reads a `_wp_notes_locked` post
meta and then runs the `note_action_is_locked` filter over it. The lock is
expressed per action - create, reply, edit, resolve, delete - so a site can
either freeze a post's notes entirely or preserve them selectively, such as
by disallowing deletion while review continues. Reading notes is never gated.

WP_REST_Comments_Controller enforces the lock in its create, update and
delete permission checks, and get_block_editor_settings() advertises the
locked actions as `lockedNoteActions` so the editor can hide affordances the
REST API will refuse.
@github-actions

Copy link
Copy Markdown

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 adamsilverstein.

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

@github-actions

Copy link
Copy Markdown

Trac Ticket Missing

This pull request is missing a link to a Trac ticket. For a contribution to be considered, there must be a corresponding ticket in Trac.

To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. More information about contributing to WordPress on GitHub can be found in the Core Handbook.

@github-actions

Copy link
Copy Markdown

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

  • 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.

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.

1 participant