Conversation
|
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 Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Hug0-Drelon
left a comment
There was a problem hiding this comment.
Should we take the opportunity to improve code coverage for https://github.com/WordPress/wordpress-develop/pull/13246/changes#diff-7c58dcea3675e604ab0caa9263522686924e3dd881d5a36b94865dacfbca129bR309-R323?
i.e. testing with $post_data['visibility'] set to 'public'|'password'|'private'?
Otherwise, thanks for the proposed tests!
|
Good call, done in 5540b57. Three characterisation tests for the switch, plus a fourth that turned out to matter more than I expected.
The fourth oneWhile writing those I noticed the change alters behaviour for one input nobody has mentioned on the ticket. Inferring the visibility from the password happens after the caller's own value is read, so the two can now contradict each other and the inferred one wins. On trunk,
If that precedence is wrong, the fix is to infer only when the caller sent no VerificationFour new tests, negative control run rather than assumed. Against trunk's Whole file: 56 tests, 125 assertions, 0 failures. The 6 warnings are the pre-existing |
I think this needs a core committer opinion. |
|
@Hug0-Drelon Fair, and while writing up the question for a committer I found a second case that answers it, so I have taken the narrow route in 4ecdf88. The precedence question does not need a ruling
Inferring unconditionally rewrote that to The change// Infer the password visibility when the caller did not send one of its own.
if ( ! isset( $post_data['visibility'] ) && ! empty( $post_data['post_password'] ) ) {
$post_data['visibility'] = 'password';
}The inference now only fills a gap. Every caller that sends a visibility behaves exactly as it does on trunk, so the only behaviour this PR changes is the reported one: a caller that sends no visibility at all, which is Quick Edit in every case except Private. Tests
Verified in three directions
The middle row is the ticket's bug and nothing else. The bottom row is why the guard is there. Whole file: 56 tests, 125 assertions, 0 failures. The 6 warnings are the pre-existing If a committer would rather have the password win outright, dropping the |
4ecdf88 to
42e4b1a
Compare
At this day, the bug is like that (I'm a begginer). wordpress-develop/src/wp-admin/includes/post.php Lines 313 to 315 in 0c3e42c |
|
Welcome @GwendolinePicquet! Exactly: lines 310 and 315 use The bug before this PR was that Quick Edit updates the password but never sends a By inferring |
Quick Edit posts `sticky` but never posts `visibility`, so `edit_post()` never reached the `case 'password'` branch that drops the sticky flag. A post could end up both sticky and password protected, a combination the block editor itself forbids. Set `visibility` to `password` whenever a post password is present, so the existing branch runs whatever the caller sent. Original patch by Hug0-Drelon in PR WordPress#11180. See #64810.
…value. Reproduces the Quick Edit payload: `sticky` is posted, `visibility` is not. Fails on trunk, where the post keeps both the sticky flag and the new password, and passes with the preceding change. See #64810.
Adds the `public`, `password` and `private` cases of the visibility switch, which had no coverage before, and pins the precedence between an inferred visibility and a contradicting one sent by the caller. That last one is a behaviour change worth naming: on trunk, sending `visibility` as `public` alongside a non-empty `post_password` cleared the password. Inferring the visibility from the password applies last, so the password now wins instead. The editors never send that pair, since choosing Public clears the password field, but `edit_post()` is also reachable from bulk edit and Quick Edit. See #64810.
The inference is meant to fill a gap, not to outrank the caller. Reading it after the caller's own value let the two contradict each other, with the inferred one applied last: - `visibility => 'public'` sent with a non-empty password kept the password, where trunk dropped it. - `visibility => 'private'` sent with a password left the post public and password protected instead of private. Quick Edit reaches this pair through `wp_ajax_inline_save()`, which sets `visibility` to `private` whenever "Private" is ticked. Guarding the inference with `! isset( $post_data['visibility'] )` confines the behaviour change to the reported case, a caller that sends no visibility at all. The `private` characterisation test now sends a password alongside, and the precedence test is inverted: an explicit visibility survives. See #64810.
e280c39 to
ccd71dc
Compare


Trac ticket: https://core.trac.wordpress.org/ticket/64810
Opened at @audrasjb's request in comment:15: the change from #11180 plus the unit test that was only sitting in a comment there, so the ticket can lose
needs-unit-testsand be committed as one changeset.The change
@Hug0-Drelon's patch from #11180, committed under their authorship, plus one guard added in review (see below).Quick Edit posts
stickybut never postsvisibility.edit_post()only unsetsstickyinsidecase 'password'of thevisibilityswitch, so that branch was never reached and a post could end up both sticky and password protected, a combination the block editor itself forbids.Inferring
visibility => 'password'from a non-empty post password makes the existing branch run for a caller that sends no visibility of its own:Why the
! isset()guardInferring unconditionally would have outranked the caller. Two inputs changed behaviour beyond the reported one:
visibility => 'public'sent with a non-empty password kept the password, where trunk drops it.visibility => 'private'sent with a password left the post public and password protected instead of private. This one is reachable from the screen the ticket is about:wp_ajax_inline_save()setsvisibilitytoprivatewhenever "Private" is ticked (ajax-actions.php:2146).With the guard, the only behaviour this PR changes is the reported one.
The tests
Five tests in
tests/phpunit/tests/admin/includesPost.php, which had no coverage of the visibility / sticky / password interaction at all.test_edit_post_unsticks_a_post_when_a_password_is_set_without_visibility()reproduces the Quick Edit payload:stickyis posted,visibilityis not. This is the ticket's bug.public,passwordandprivatebranches reached by an explicit value. Theprivateone sends a password alongside, which makes it the regression test for the Quick Edit Private case above.test_edit_post_keeps_an_explicit_visibility_over_an_inferred_one()pins that the inference never overrides the caller.Verified in three directions locally, PHP 8.3 against a single site:
src/wp-admin/includes/post.phpOK (5 tests, 10 assertions)privatecase and the explicit-publiccaseWhole file: 56 tests, 125 assertions, 0 failures. The 6 warnings are the pre-existing
E_DEPRECATEDexpectation notices PHPUnit 9.6 emits across that file.phpcson both touched files: 0 errors. The single warning insrc/wp-admin/includes/post.phpis at line 901 and predates this branch.Testing
Independently confirmed in Playground by @ArkaPrabhaChowdhury in comment:20 against
7.2-alpha, PHP 8.3.Credit
The fix is
@Hug0-Drelon's work and the props on #11180 already list hugod, wildworks, abcd95, motylanogha. If this lands instead of #11180, that props line should carry over, plus arkaprabhachowdhury for the test report.The open question this PR does not settle
2nd-opinionwas removed from the ticket on 2026-08-24, but the disagreement behind it stands on its own: in comment:13 @wildworks argued that password protected and sticky should be allowed together in all cases, and that #11180 removes something Quick Edit could previously do.That is a disagreement about intended behaviour, not about the code. This PR exists because @audrasjb asked for the change and the test in one place; if the ticket resolves the other way, the same test file is where the inverse expectation would go.
Use of AI Tools
AI assistance: Yes
Tool(s): Antigravity IDE
Model(s): Gemini 2.5 Pro
Used for: Code analysis, conflict resolution, test suite updates, and patch verification. All changes were manually reviewed and tested locally.
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.