Site-kit-wp: Register module settings options

[aaemnnosttv]

Feature Description

Module settings are currently saved as an array. This can lead to inconsistent return values when using Option::get() since there is no ability to pass the $default to the internal get_option calls. A better approach would be to register the option's default using register_setting so that it is applied globally.

Let's register all settings (options) used by modules so that defaults and other metadata are consistent globally.

See https://developer.wordpress.org/reference/functions/register_setting/
See https://github.com/google/site-kit-wp/pull/824#issuecomment-553674695

---

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

Acceptance criteria

• Every Site Kit module should register its settings using WordPress's register_setting() function.
• These settings should be registered regardless of whether the module is active or not (we'll likely need to make some updates to the Module base class to support that).
• The registration should include the data type and the default value at a minimum.
• Generic option filters for a module's settings should also be added at the same time, so that they run regardless of whether a module is active.

Implementation Brief

• Introduce new abstract Core\Storage\Setting class, representing a single option setting
  
  
  
  • __construct( Options $options )
  
  
  • Add has/get/set/delete methods to call Options internally for its option
  
  
  • abstract public register() : void
  
  
• Introduce a new Module_With_Setting interface
  
  
  
  • public function get_settings(): Core\Storage\Setting
  
  
  • Add to all current modules that use a Setting
    
    _Note PageSpeed Insights has an option but isn't used. Remove?_
  
  
• Create a new Modules\{module}\Settings class for each module which extends Setting
  
  
  
  • register should be implemented which register's the setting and adds option_ filters (if any) that were previously in the module's register method
  
  
  • The base class uses the singular form because it represents a single option, but the module instances use the plural form since they are serialized options, storing many key-value pairs
  
  
• Update Modules::register() to register module settings before calling the active modules' register methods (this ensures defaults and filters are ready for use in any other module when registered)
  
  
  
  • Call Module::get_settings()->register() on all modules that implement Module_With_Setting
  
  
• Update existing option calls for module options to use setting instead
  php $this->options->get( self::OPTION ) // becomes: $this->settings->get()

Other possible refactoring (future)

• Update Credentials to extend Setting since it is essentially the same thing to reduce duplication
  
  
  
  • If this pattern is one that we like, we could do something similar for user options as well, such as Profile and Verification
  
  

Changelog entry

• Ensure that all module settings are properly registered in WordPress for consistent behavior.

[felixarntz]

@aaemnnosttv I have a couple of suggestions:

__construct( string $name, string $group, $args = [], Options $options = null )

I wonder if we should instead create Setting as an interface or abstract base class and have sub-classes per actual setting (e.g. Google\Site_Kit\Modules\AdSense\Settings for googlesitekit_adsense_settings --> this would be the first time we introduce extra module-specific PHP classes, I think those would make sense in a namespace for that module). This would be closer to our current implementation of e.g. Credentials, the constructor and method signatures would pretty much match as they are. The other benefit of that would be that each setting class would manage itself, we wouldn't need to pass all the logic in one centralized class via the constructor, which would also mean that that information only gets processed right before actually registering the setting.

Add a new abstract public Module::register_active() method to the base Module class for registering hooks only if the module is active

I think this is a bit confusing, and it also means that we'd change what the current register() methods are used for. Especially since we'd only use the "non-active" one for registering the setting, I suggest to keep register() logic untouched and simply rely on whether a Module is a Module_With_Setting to call its Module::get_setting()->register(). We shouldn't need to introduce a general differentiation between general registration and active registration.

E.g. option_ filters would stay in register() while most others would move to register_active()

Based on my above comment, I think these filters could simply move to the respective setting class's register() method.

[aaemnnosttv]

IB updated per your feedback and our conversation @felixarntz 👍

[felixarntz]

IB looks great ✅

@aaemnnosttv Just adding to our conversation, I think the actual setting name can be in an OPTION constant per Setting sub-class, and it could either be assigned as an internal property so that the parent methods can access it - or the abstract class should define a "base const", and then use static::OPTION.

Let's include the Credentials class refactoring in this PR, that should be a quick change for more consistency. Note that we might need to override the has() method here to keep the current functionality (which is okay).

[cole10up]

Ran a series of regression around setting defaults after resetting site kit. No issues on my end.

Passed QA ✅

[felixarntz]

@aaemnnosttv I just noticed that several classes extending Setting implemented get_default() as public. Can you open a follow-up PR to make all of these protected like in the base class?