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

Refactor with User_Setting class #1029

Closed
aaemnnosttv opened this issue Jan 13, 2020 · 5 comments
Closed

Refactor with User_Setting class #1029

aaemnnosttv opened this issue Jan 13, 2020 · 5 comments
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Milestone

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jan 13, 2020

Feature Description

In #859/#995 we introduced a new Setting class to model a single option/setting. This has the benefit of centralizing logic and behavior that belongs to a specific setting in a single class which extends the base.

Site Kit also has many user options which WordPress implements as user meta. User options are slightly different than options or plain meta as the key is optionally prefixed with the blog prefix from wpdb depending on the site. This is because the users and usermeta tables are network-wide tables in multisite, and so prefixes are used to identify site-specific entries.

Classes which could be refactored as sub-classes of User_Setting are:

  • Verification
  • Verification_File
  • Verification_Meta
  • Tracking

Similar to what was done in the issue above, we should also add a User_Options_Interface that both existing User_Options and Encrypted_User_Options should implement.

An individual User_Setting would then only require a User_Options_Interface as a constructor dependency. Each sub-class would keep its key defined with its constant.

The User_Setting class would have a register method which calls register_meta internally and registers the option dynamically using methods to populate the arguments array similar to Setting. The registered meta key must be with/without blog prefix according to whether or not the plugin is in network mode. The user options interface could require a get_meta_key( $option ) method to get the underlying meta key as its implementations would have access to Context:is_network_mode() which is needed to conditionally prefix the option name.

User_Setting should also have a method for getting the respective meta key for its option. It would also be useful if there was a static method which could be used to apply the blog prefix to an arbitrary string for use in other places in the codebase, such as uninstall.php.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • There should be a User_Options_Interface interface exposing the following methods:
    • get( $option )
    • set( $option )
    • delete( $option )
    • get_meta_key( $option )
    • get_user_id()
    • switch_user( $user_id )
  • User_Options and Encrypted_User_Options should implement User_Options_Interface.
  • There should be an abstract User_Setting class that receives a User_Options_Interface as sole constructor parameter and exposes the following methods:
    • register()
      • should call register_meta(), using User_Options_Interface::get_meta_key( static::OPTION ) and protected methods get_type() and get_sanitize_callback() (the value for single should always be true)
      • should also filter in a default user option value via protected method get_default() --> let's see if this is possible based on the filters WordPress core provides
    • has()
    • get()
    • set( $value )
    • delete()
  • Verification, Verification_File, Verification_Meta, and Tracking should extend User_Setting.

Implementation Brief

  • Add a Core\Storage\User_Options_Interface with methods as defined in AC
    • Should not extend Options_Interface
  • Add an abstract Core\Storage\User_Setting class, which implements the above interface
    • Implement default handling in get() as WP does not offer the same kind of filters as for options
      • Potentially add a parse_defaults method as we have used before
    • Only filters for meta are just for short-circuiting cache and DB
  • Refactor Verification, Verification_File, Verification_Meta, and Tracking as instances of User_Setting
    • Remove Transients as a constructor dependency of Verification_Meta
      • Move Verification_Meta::get_all() to Site_Verification

Changelog entry

  • Register all of the plugin's user options in WordPress via register_meta().
@felixarntz
Copy link
Member

A couple of comments on the description:

  • User_Option should be User_Setting, consistent with Setting.
  • I don't like the idea of exposing Context in User_Options_Interface. User_Options_Interface could match Options_Interface, plus get_user_id() and switch_user(). Since we need Context for the transformation of user option to user meta key only, we should IMO only expose a method like this on User_Options_Interface, e.g. get_meta_key( $option ).

@felixarntz felixarntz added the P1 Medium priority label Jan 13, 2020
@aaemnnosttv aaemnnosttv changed the title Refactor with User_Option class Refactor with User_Setting class Jan 13, 2020
@aaemnnosttv
Copy link
Collaborator Author

Those all sound good to me @felixarntz, I've updated the title and description accordingly.

@felixarntz felixarntz removed their assignment Jan 13, 2020
@aaemnnosttv aaemnnosttv self-assigned this Jan 13, 2020
@felixarntz
Copy link
Member

IB ✅

@aaemnnosttv
Copy link
Collaborator Author

@felixarntz This one has a partial dependency on the changes to the Tracking class in #1055.

@aaemnnosttv aaemnnosttv added the QA: Eng Requires specialized QA by an engineer label Jan 30, 2020
@ThierryA ThierryA added this to the Sprint 15 milestone Feb 3, 2020
@ThierryA ThierryA modified the milestones: Sprint 15, Sprint 16 Feb 3, 2020
@aaemnnosttv aaemnnosttv mentioned this issue Feb 4, 2020
6 tasks
@eclarke1 eclarke1 modified the milestones: Sprint 16, Sprint 17 Feb 12, 2020
@felixarntz felixarntz assigned aaemnnosttv and unassigned felixarntz Feb 14, 2020
@felixarntz felixarntz assigned aaemnnosttv and unassigned felixarntz Feb 19, 2020
@felixarntz felixarntz removed their assignment Feb 19, 2020
@tofumatt tofumatt self-assigned this Feb 26, 2020
@tofumatt
Copy link
Collaborator

Code here looks solid; settings continue to work for me, so I think this one is QA ✅

@tofumatt tofumatt removed their assignment Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants