WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 5 months ago

#42658 closed defect (bug) (fixed)

Customize: Heartbeat doesn't refresh changeset lock when branching is enabled

Reported by: dlh Owned by: dlh
Milestone: 4.9.3 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Heartbeat requests from the Customizer will create a WP_Customize_Manager instance without a changeset_uuid. However, when changeset branching is active and when the manager is instantiated without a UUID, WP_Customize_Manager::establish_loaded_changeset() will generate a new UUID.

Without the UUID from the client, WP_Customize_Manager::check_changeset_lock_with_heartbeat() won't have a post ID whose lock should be refreshed.

The attached patch would attempt to address this by including the current UUID with the heartbeat request. The patch then has check_changeset_lock_with_heartbeat() use the heartbeat UUID to find the changeset post ID before falling back to WP_Customize_Manager::changeset_post_id().

Attachments (3)

42658.diff (1.9 KB) - added by dlh 7 months ago.
42658.diff.png (428.7 KB) - added by westonruter 5 months ago.
git diff -w --color-words
42658.2.diff (2.0 KB) - added by dlh 5 months ago.

Download all attachments as: .zip

Change History (17)

@dlh
7 months ago

#1 @westonruter
7 months ago

  • Milestone changed from Awaiting Review to 4.9.1

Good catch!

#2 @johnbillion
7 months ago

  • Milestone changed from 4.9.1 to 4.9.2

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 months ago

#4 @westonruter
5 months ago

  • Owner set to westonruter
  • Status changed from new to reviewing

#5 @dd32
5 months ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 months ago

@westonruter
5 months ago

git diff -w --color-words

#7 @westonruter
5 months ago

Just wanted to share 42658.diff.png as a demonstration of how useful git diff -w --color-words is when looking at a patch like 42658.diff to see that it's not as large as it would at first seem.

#8 @westonruter
5 months ago

  • Keywords needs-patch added; has-patch removed
  • Owner changed from westonruter to dlh

@dlh one thing that I think needs to be improved with 42658.diff: it's checking if current_user_can( 'customize' ) but it's not checking whether the current user can actually edit the specific $changeset_post_id. In other words, I think it needs to include a condition like: current_user_can( get_post_type_object( 'customize_changeset' )->cap->edit_post, $changeset_post_id ).

It's too bad that merely data.changeset_uuid = api.settings.changeset.uuid doesn't cause the Customizer to initialize with the UUID supplied there. The reason is that _wp_customize_include() is looking for a customize_changeset_uuid or changeset_uuid input var, but Heartbeat sends it as data[changeset_uuid]. The only way to get around that would be to opt to inject the customize_changeset_uuid param by some other means, such as by jQuery.ajaxPrefilter, which we are currently doing to add the customize_preview_nonce: https://github.com/WordPress/wordpress-develop/blob/026cd70/src/wp-admin/js/customize-controls.js#L7784-L7792

I don't know if that's better than just improving the cap checking for locking as it exists in core today.

#9 @westonruter
5 months ago

  • Priority changed from normal to high

@dlh This needs to be included in 4.9.3 now because, if I'm not mistaken, the change in #42975 will mean the lock will get lost when a user is sitting in the Customizer but doesn't make any changes. If they make a change, the autosave will cause the lock to be regained, but heartbeat won't be maintaining their lock otherwise. Right?

This ticket was mentioned in Slack in #core-customize by dlh. View the logs.


5 months ago

@dlh
5 months ago

#11 @dlh
5 months ago

  • Keywords has-patch added; needs-patch removed

42658.2.diff makes the current_user_can() check more specific as suggested in comment:8.

The only way to get around that would be to opt to inject the customize_changeset_uuid param by some other means...I don't know if that's better than just improving the cap checking for locking as it exists in core today.

I'm not sure I entirely follow. I agree that it would be nice if the Customizer automatically initialized with the UUID from the heartbeat request. But, as far as I can tell, that wouldn't cause the Customizer to start running any new checks for current_user_can( get_post_type_object( 'customize_changeset' )->cap->edit_post, $changeset_post_id ). So, adding that check in check_changeset_lock_with_heartbeat() seems useful either way.

the change in #42975 will mean the lock will get lost when a user is sitting in the Customizer but doesn't make any changes. If they make a change, the autosave will cause the lock to be regained, but heartbeat won't be maintaining their lock otherwise. Right?

It's true that \WP_Customize_Manager::establish_loaded_changeset() will no longer call set_changeset_lock() during heartbeat requests because $pagenow is admin-ajax.php: https://github.com/WordPress/wordpress-develop/blob/026cd70e25b90f04c27a21eaf634048945848398/src/wp-includes/class-wp-customize-manager.php#L643.

However, for users in "linear" changeset mode, establish_loaded_changeset() should still be able to find the current changeset post ID, and \WP_Customize_Manager::check_changeset_lock_with_heartbeat() can use that post ID to proceed with its logic. Users with changeset branching enabled are still in the same boat they're in now. So I'm not sure #42975 changes the situation in this ticket.

Of course, let me know if I've misunderstood what you meant on either point.

#12 @westonruter
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 42612:

Customize: Ensure heartbeat keeps changeset locked when in branching mode.

Props dlh.
See #42024.
Fixes #42658.

#13 @westonruter
5 months ago

  • Keywords commit fixed-major added
  • Priority changed from high to normal
  • Resolution fixed deleted
  • Status changed from closed to reopened

#14 @SergeyBiryukov
5 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42623:

Customize: Ensure heartbeat keeps changeset locked when in branching mode.

Props dlh.
See #42024.
Merges [42612] to the 4.9 branch.
Fixes #42658.

Note: See TracTickets for help on using tickets.