Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data: Persist user preferences via user meta #19177

Open
wants to merge 10 commits into
base: master
from

Conversation

@aduth
Copy link
Member

aduth commented Dec 16, 2019

Closes #15105

This pull request seeks to implement a custom data persistence storage implementation for the purpose of storing preferences via user metadata. This uses existing REST API functionality for updating metadata via a registered meta key. The storage implementation bootstraps the stored metadata value for use in the initialization of the store. When a preference update occurs, it is saved to the REST API using the users endpoint for the current user.

Implementation Notes:

  • From a data permissions perspective, it isn't clear to me whether we should want this data to be shown to anyone but the current user. The users endpoint does require higher capabilities in order to retrieve details about other users on the site, but even then, the preferences specific to a user seem to be information which isn't necessary to include on the endpoint except for the current user. Searching through the implementation of REST API meta fields handling, there doesn't seem to be very many options for such explicit control over this behavior, except via the prepare_value property of show_in_rest, which still isn't provided with the specific user ID of the meta value being served (in order to distinguish whether it should be served based on matching the current user). I may be over-thinking this, and it may be perfectly fine to assume that if a user has permissions to request details of other users on the site, they should also be permitted to view persisted preferences details of that user.
  • This does not yet handle site-specific settings, which could be implemented in a future pull request. It is not yet clear how we would want to express this from a store that persists state values in a way which could be generalized. I expect if this were needed, it could apply as a post-processing transform on the metadata save, where filtering occurs on the saved metadata to "pick" values which should apply specific to the current site.

Follow-up Tasks:

At the earliest opportunity, this code should be migrated into the core codebase.

The inline script should take the place of this segment of code:

https://github.com/WordPress/wordpress-develop/blob/8a88cfa/src/wp-includes/script-loader.php#L679-L693

As far as I can find, there is no prior art for explicit meta registration in core. Most meta is not referenced without being registered. This meta value must be registered in order for it to be accessible via the REST API. It's unclear to me where this should reside in the core codebase (rest-api.php? something akin to post.php's create_initial_post_types, but for users and their meta?).

Testing Instructions:

  1. Navigate to Posts > Add New
  2. Note that the Welcome modal is not shown, because there is backwards-compatibility in place which assures that existing persistence value is preserved.
  3. Clear localStorage by entering localStorage.clear() in your browser console (or follow these instructions to manage Application Storage in Chrome)
  4. Reload the page
  5. Note that the Welcome modal is shown
  6. Dismiss the Welcome modal
  7. Reload the page
  8. Note that the Welcome modal is not shown, because the preference has persisted from prior to the reload (this time using user meta)
  9. In your browser's "Private Browsing" ("Incognito") mode, log in and navigate to Posts > Add New
  10. Note that the Welcome modal is still not shown
$wp_scripts->registered['wp-data']->extra['after'] = array();
wp_add_inline_script( 'wp-data', $persistence_script );
}
add_action( 'enqueue_block_editor_assets', 'gutenberg_user_settings_data_persistence_inline_script', 20 );

This comment has been minimized.

Copy link
@aduth

aduth Dec 16, 2019

Author Member

Note: The priority here shouldn't be necessary, but the behavior of the function is such that it replaces any inline scripts for the wp-data handle. This conflicts with Gutenberg's own gutenberg_enqueue_block_editor_assets function, which does the same and was intended to serve as a temporary implementation of the preferences migration. Since this was migrated to core as part of Trac#46429, it should be removed from Gutenberg, at which point the priority here would no longer be necessary.

This comment has been minimized.

Copy link
@aduth

aduth Dec 17, 2019

Author Member

Since this was migrated to core as part of Trac#46429, it should be removed from Gutenberg, at which point the priority here would no longer be necessary.

See #19178 . If the other is merged first, this can be rebased to remove the priority parameter.

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Dec 16, 2019

It's unclear to me where this should reside in the core codebase (rest-api.php? something akin to post.php's create_initial_post_types, but for users and their meta?).

That is what I was thinking as well. create_initial_user_meta() perhaps? It'd be in wp-includes/user.php I guess.

I may be over-thinking this, and it may be perfectly fine to assume that if a user has permissions to request details of other users on the site, they should also be permitted to view persisted preferences details of that user.

I think it's fine for them to be able to view those preferences.

This does not yet handle site-specific settings

As in settings specific to a single site in a multisite?

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 18, 2019

This does not yet handle site-specific settings

As in settings specific to a single site in a multisite?

Yes, there was a bit of discussion in #15105 about how certain preferences could make sense to be associated for a user in the context of a specific site: Frequently used blocks, "enabled" blocks; especially considering that two sites in a network may have different blocks installed.

Right now we don't have any way to express this. It sort of "just worked" previously because the storage would be unique per domain. I actually expect it could be an issue already for subdirectory-based multi-sites (where I assume the browser storage could still be shared across sites).

@spacedmonkey

This comment has been minimized.

Copy link
Contributor

spacedmonkey commented Dec 19, 2019

The key for these settings should not be wp_data_persistence, as this would make the data global for all sites on a multisite. WordPress treats every site differently, so you should be allowed to have different testings per site on the multisite. Instead of using wp, use $wpdb->prefix, which will automatically prefix for you with site prefixed id. This is how capabilities work for example.

update_user_meta( $user->ID, $wpdb->prefix . 'capabilities', array( $role => true ) );
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 19, 2019

The key for these settings should not be wp_data_persistence, as this would make the data global for all sites on a multisite. WordPress treats every site differently, so you should be allowed to have different testings per site on the multisite. Instead of using wp, use $wpdb->prefix, which will automatically prefix for you with site prefixed id. This is how capabilities work for example.

update_user_meta( $user->ID, $wpdb->prefix . 'capabilities', array( $role => true ) );

I could imagine there to be some use-cases for storing user preferences global to all sites in a multi-site. Per previous discussion, it's acknowledged there would need to be some way to express this distinction which doesn't currently exist. On reflection though, I would agree that, at least based on current use of preferences, storing the value as distinct per site would probably be the safer default for now.

As to the specific implementation, there might be a challenge in making this database prefix available to the browser for use in calling to the REST API. I suppose that could be part of the "bootstrapping" logic for how this inline script is generated (i.e. generate the meta key server-side, and inject into the inline script).

@spacedmonkey

This comment has been minimized.

Copy link
Contributor

spacedmonkey commented Dec 19, 2019

As a maintainer of multisite in core, I would say that per site is a much safer options. There are no other options in WordPress that a global like this. There maybe good reason you want different settings per site, you never know.

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Dec 19, 2019

One possibility to handle the bootstrapping, we can utilize the name feature to only expose the underlying meta key in a consistent way.

global $wpdb;
register_meta( 'user', $wpdb->prefix . 'data_persistence', [
	'show_in_rest' => [
		'name'   => 'data_persistence',
		'type'   => 'object',
		'schema' => [
			'type'                 => 'object',
			'additionalProperties' => true,
		]
	]
] );

That way the preferences for the current site will always be available as meta.data_persistence.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 20, 2019

One possibility to handle the bootstrapping, we can utilize the name feature to only expose the underlying meta key in a consistent way.

Thanks for the tip, @TimothyBJacobs . That seems to work pretty well in cb0bb4b.

One open question here is whether to treat this value as an object, or as a (JSON) string. I noticed in your example that you structured it as an object. As least as it concerns the usage in the client-side data module, it's currently implementing as conforming to the Web Storage API interface, where the value is always stored as a string. So at least as to the practical extent of how this value is used, it's expected as a JSON string. But, since that string represents an object, I could grant an argument that its natural form is that of an object, and we could define its REST schema accordingly. In the end, it's just a matter of doing some JSON parse/stringify dance for the sake of supporting it as an object over REST transmission.

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Dec 20, 2019

Thanks for the tip, @TimothyBJacobs . That seems to work pretty well in cb0bb4b.

Awesome!

One open question here is whether to treat this value as an object, or as a (JSON) string.

I would personally prefer us to send the value as an actual object instead of a JSON string. That way when the data is transferred it is easily readable, and can be validated natively by the REST API.

I also think it is far more common in core to store non scalar data as serialized PHP vs encoded as a JSON string. That being said, I think there are many people who would like to move to storing that info in JSON. Though, even if we were to move to start storing those non-scalars as JSON, we wouldn't need to send the values as JSON strings.

So I think my preference would be to send an object, but I'm not strongly opposed if you think it'd be best to conform literally to the Storage API.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 20, 2019

It seems like treating it as an object could open some options for inspecting or transforming that value more easily server-side. I think the main blocker for me was in whether it's premature to consider this, when the main use-case we're targeting (persistence for the client-side data module) explicitly does not expect it to be an object.

I also considered that it's ultimately persisted server-side as a string anyways (in the PHP serialization format, rather than as JSON). As you mention, this is more of an implementation detail, and not really of much consequence one way or the other.

I think I'll change it toward the object schema. It should be pretty trivial to transform it back into the appropriate format client-side.

Edit: See e4f2f1c .

lib/compat.php Outdated Show resolved Hide resolved
lib/compat.php Outdated Show resolved Hide resolved
Copy link
Contributor

spacedmonkey left a comment

Posts a couple of code tweaks.

lib/compat.php Outdated Show resolved Hide resolved
lib/compat.php Outdated Show resolved Hide resolved
'name' => 'data_persistence',
'type' => 'object',
'schema' => array(
'type' => 'object',

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey Jan 8, 2020

Contributor

Should a default value be useful here?

This comment has been minimized.

Copy link
@aduth

aduth Jan 10, 2020

Author Member

Should a default value be useful here?

Would you expect it to have an impact? Or what would you hope should happen by providing a default?

My concern with this might be: We don't want to set a meta value unless one is actually provided with the request.

Trying to follow how this logic flows in the REST API implementation, it doesn't appear it should have an impact one way or the other (at least currently), since it seems like the schema defined here is referenced only after considering that a value was provided.

aduth and others added 5 commits Jan 10, 2020
Co-Authored-By: Jonny Harris <spacedmonkey@users.noreply.github.com>
Co-Authored-By: Jonny Harris <spacedmonkey@users.noreply.github.com>
Co-Authored-By: Jonny Harris <spacedmonkey@users.noreply.github.com>
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 21, 2020

Interestingly, the failing build seems expected considering that the preferences are now more sticky than they were previously, so certain tests which manipulate preferences might have an unexpected bleed-over into other tests. Previously we relied on clearing localStorage before tests to be sufficient, but this won't be enough anymore. I'm not sure what the best option here would be, considering that we don't (currently) provide any APIs for manipulating preferences as a generalized grouping. We could reset the specific preference values, but ideally it's something we can express as "reset all preferences".

Rough ideas:

  • Plugin which defines its own persistence middleware to override the meta value or otherwise disables persistence (do we have any tests which verify persistence behavior and therefore rely on it?)
  • Manually reset meta values to an initial state before tests are run (via REST API? a custom plugin used in tests?)
  • Update tests to either be more tolerant of persisted preferences, or to unset preference changes after test completes (this would be difficult for some preferences like insertUsage which is updated in response to any block insertion)
@spacedmonkey

This comment has been minimized.

Copy link
Contributor

spacedmonkey commented Jan 22, 2020

@aduth Why not just delete / set to empty object the meta on every test? The existing user api should be able to do this.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 22, 2020

@aduth Why not just delete / set to empty object the meta on every test? The existing user api should be able to do this.

Yeah, this is like the second point in my previous comment. I think we'd need/want a test-specific plugin for this (since the tests themselves are run in Node and the browser), but it should be simple enough to do. We have plenty of precedent.

@JJJ

This comment has been minimized.

Copy link
Contributor

JJJ commented Jan 22, 2020

Tangentially related: https://core.trac.wordpress.org/ticket/33542

Instead of user_meta, you will likely want to move to using the user_option() functions.

User Options are used by per-site preferences that persistent across all page loads. Things like admin-color-scheme use it, allowing for per-user per-site option of what color the dashboard area should be.

In addition, WordPress does offer the user_setting() functions, but they are extremely primitive and should probably be improved soon.

They use cookies (ugh) and are only used by the currently logged in user, but their purpose is to provide functional local storage.

I think many Editor/Gutenberg type user-options will inevitably behave like my linked Core ticket, as a nested preference, scoped as such:

  • Install wide
  • Site wide
  • User wide
  • User on a specific site
  • User in a specific device

Ultimately site owners want to have smart defaults and rational overrides. By default, it makes sense for a fresh WordPress installation to show the “Welcome to blocks” pop up, but 5 years from now Time Magazine may want to hide this permanently for all of their experienced authors and editors, and this is what a global Editor setting to act as the default for all users is for.

IMO, solving this for users alone addresses an immediate need (for a problem that shouldn’t exist anyways) which is great, so long as the follow-up work continues up the chain to allow these user-preferences to also have smart defaults, and not simply be hard-coded in place as is.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 22, 2020

Tangentially related: https://core.trac.wordpress.org/ticket/49213
[...]
I think many Editor/Gutenberg type user-options will inevitably behave like my linked Core ticket, as a nested preference, scoped as such:

Did you link the correct ticket? I'm not sure how this one relates.

Instead of user_meta, you will likely want to move to using the user_option() functions.

There was also a similar suggestion to this effect at #15105 (comment) , with follow-on evaluation at #15105 (comment) in discussing REST API availability and how the approach here should be effectively equivalent based on the underlying implementation of get_user_option.

In addition, WordPress does offer the user_setting() functions, but they are extremely primitive and should probably be improved soon.

They use cookies (ugh) and are only used by the currently logged in user, but their purpose is to provide functional local storage.

Is this in reference to the window.getUserSetting JavaScript functions? There was an earlier implementation at #15800 which explored this as well, with conclusions in the original comment and at #15800 (comment) in how the approach was non-viable.

@JJJ

This comment has been minimized.

Copy link
Contributor

JJJ commented Jan 22, 2020

Did you link the correct ticket? I'm not sure how this one relates.

No, I didn’t. Too many tickets open. Fixed my original comment.

how the approach was non-viable.

Oh, totally non-viable. This is a shortcoming of the user settings API, which surely should be fixable in WordPress itself. Cookies for storage was an idea that came before browsers offered any alternative. Now that it’s been several years of having more than cookies, WordPress has some obligation to update its implementation.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 23, 2020

@JJJ I like what you're proposing at Trac#33542. I think for many of the "preferences" used by Gutenberg today, it does make sense that they can be user-specific, but the idea of establishing some hierarchy where the site might want to assign its own defaults is similarly sensible for most. In fact, in thinking about this, I'd probably consider my own Disable NUX plugin as something of a roundabout implementation of that very idea (allowing site-level override of the NUX preference).

I'm trying to think what might be the best way to go about maximizing future-compatibility, while still allowing the more immediate effort to progress. One thing I might worry about with the current implementation is that it treats "preferences" as a single object, vs. individual keys. While this might be convenient as a way to limit what we're providing to the browser and it reflects how the data is actually managed in client-side state, I'm wondering if there might be a better way to go about expressing that for how we store the values server-side and make them available more generally.

Some ideas:

  • A filterable array of user meta keys we consider as "preferences"
  • Including values based on whether they're explicitly registered (register_meta( 'user', ... );), optionally filtering it further to only those with show_in_rest
  • Use a convention for preference meta keys, e.g. prefix "preference_"
    • It prompts the question though: Where conceptually should a line be drawn between an "option" and a "preference", or does it even make sense for there to be one?

A few additional challenges to consider:

  • Is there a performance impact if we're picking many individual meta values? (I'm not entirely clear whether these would be queried from the database en masse or individually)
  • The proposed implementation (and current localStorage-based storage) is structured in a way that the top-level object key corresponds to the @wordpress/data store name to know how to initialize the state. In a more generalized approach to preferences, we'd need to find an alternative way to map preferences to their intended stores
    • Could possibly be embedded in the name of the preference via some convention

For reference, here is a more-or-less complete set of the current active preferences used:

  • core/block-editor (reference)
    • block insert usage
  • core/editor (reference)
    • show pre-publish step
  • core/edit-post (reference)
    • show welcome guide
    • editor mode ("visual", "code")
    • sidebar open/dismissed
    • sidebar individual panel expanded/collapsed
    • fixed vs. contextual block toolbar
    • show inserter help panel
    • pinned plugin buttons
    • hidden block types ("Block Manager" options modal)
    • localStorage-autosave interval
@TimothyBJacobs

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Jan 23, 2020

(I'm not entirely clear whether these would be queried from the database en masse or individually)

They'll be queried at once.

The Gutenberg plugin could use register_rest_field to experiment with a more formal preferences object instead of putting it all in one meta key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.