WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 14 months ago

#32202 new enhancement

Add support for options page locking (settings API concurrency)

Reported by: westonruter Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.6
Component: Options, Meta APIs Keywords: has-patch dev-feedback
Focuses: administration Cc:

Description

If two users access an options page at the same time, the user who saves last will have their settings stick, while the user who saves first will have their settings lost. This is because options pages lack any access concurrency checking.

In #31436 we propose to handle conflicts in concurrent Customizer sessions by adding locking to individual controls so that users can't override each others' values. The same functionality is needed for options pages generated generated via the Settings API. At the most basic level, locking could be implemented to block a user from accessing an options page if someone else is already there, presenting the user with the same takeover screen as shown for post locking.

A more sophisticated approach would be to allow multiple users to edit an options page at once, either by syncing values across the sessions via Heartbeat, or by disabling a field entirely for other users when one user starts editing it. In this way, the disabled field would not be included when the other users submit the form. This may not work in all cases, however.

Another approach, less sophisticated and not using Heartbeat at all, would be to introduce a persistent counter for saves to a given admin page. This would serve as a revision number, and the revision could be included with each settings form. If the submitted form has a setting number that is not the same as what is currently stored, then this would indicate there is a potential conflict and the user could be presented with a screen to resolve any differences.

Attachments (1)

edit_lock.patch (11.7 KB) - added by daxelrod 3 years ago.
Generic edit locking functions for wp-admin

Download all attachments as: .zip

Change History (9)

#1 @MikeNGarrett
3 years ago

We talked through this proposal this morning. The ideas of merging settings changes seems like a much larger step forward than making the locking mechanism more generic.

I think the best initial approach would be to make the locking mechanism available to all developers where someone could specify a unique string to "lock" and a checking mechanism.

As an example of this functionality, we could implement this for all options pages (with exceptions for edit.php, edit-tags.php, etc ).

To me, that seems like enough of a jump for a future release. In the future we can look at handling different sections of a page or merging different values from 2 different people editing the page.

@daxelrod
3 years ago

Generic edit locking functions for wp-admin

#2 @daxelrod
3 years ago

Attached a generic edit locking concept for core. Roughly extends the API for post_locks and could serve as a base for settings page or customizer locking. The edit locks leverage *meta tables for comments, posts, and users; sitemeta table (type = 'site') for multisite; and options for everything else. The $type and $id parameters are purposefully not limited as to allow developers edit lock capabilities on their own settings pages.

Basic API:

wp_get_edit_lock( $type, $id ); // Retrieves the edit lock if it exists
wp_check_edit_lock( $type, $id ); // Checks if the object is being edited by another user
wp_set_edit_lock( $type, $id, $time, $user_id ); // Mark an object as being edited.

Introduced some helper functions for consistent handling of locks:

wp_read_edit_lock( $lock ); // Splits apart edit lock into time and user_id
wp_create_edit_lock( $time, $user_id ); // Packs time and user_id into an edit lock string

Introduced 2 new filters:

add_filter('allow_edit_lock', function ( $allow, $type, $id ) {}, 10, 3); // Control over if edit locks are allowed
add_filter('wp_edit_' . $type . '_lock_window', function ( $time_window ) {}); // Control over duration of edit lock for a particular type.

It has full compatibility with existing post_locks and those existing functions now proxy the new edit_lock API.

If this is desirable for core, the next step would be to make a generic version of the locking UI (This XXX is currently locked by YYY) so visual feedback for edit locks can be applied in other contexts (i.e. settings pages, customizer, etc.). We have set up a small demo where taxonomy terms can be edit locked as a proof of concept if anyone is interested. Any input appreciated.

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


3 years ago

#4 @MikeNGarrett
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

It would be awesome to get someone to test this out.

Attaching appropriate tags.

#5 @azaozz
2 years ago

There are several things that need to be considered:

  • The removal of locks (AJAX on unload) doesn't work well in all browsers and in all circumstances. Sometimes the AJAX request doesn't fire, sometimes the browser aborts it. We need to look at improving it/making it more reliable.
  • To be able to do "take over", we may need to save the current user's changes somehow. Not all settings screens will need this but there are several where we wouldn't want to discard unsaved changes/loose the current user's data.

One way this may work is by storing all form fields in session storage and restore on page reload. Another option is to show an "attempted take over" dialog with a cancel button instead of locking the screen.

edit_lock.patch is a good start, but we will need to implement the above too. Also, not sure we need variable lock timeouts. That complicates things without adding any benefits. If we manage to fix lock removal, a 5 min lock timeout would be good.

#6 @daxelrod
2 years ago

  • Keywords dev-feedback added; needs-testing removed

The removal of locks (AJAX on unload) doesn't work well in all browsers and in all circumstances. Sometimes the AJAX request doesn't fire, sometimes the browser aborts it. We need to look at improving it/making it more reliable.

It seems quite reliable in modern browsers, and in extreme cases such as force quit, in my opinion the lock timeout provides an adequate fallback.

To be able to do "take over", we may need to save the current user's changes somehow. Not all settings screens will need this but there are several where we wouldn't want to discard unsaved changes/loose the current user's data.

I agree the edit lock API should provide functions for this. Local storage seems like a logical choice to implement, but what/how data is saved and how the user would go about restoring it are UX questions.

One way this may work is by storing all form fields in session storage and restore on page reload. Another option is to show an "attempted take over" dialog with a cancel button instead of locking the screen.

I'm not sure I would want to go down this route of enabling ping-ponging back and forth.

#7 @danielbachhuber
2 years ago

Created #34173 for a more specific conversation about locking UX on term edit pages.

#8 @johnbillion
14 months ago

I'd like to look into this for 4.8.

Note: See TracTickets for help on using tickets.