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

Implement wrapping infrastructure for dealing with WordPress post metadata #3154

Closed
felixarntz opened this issue Apr 14, 2021 · 4 comments
Closed
Labels
Module: Idea Hub Google Idea Hub module related issues P0 High priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Apr 14, 2021

As part of the Idea Hub module functionality, it will be necessary for the first time in Site Kit to deal with post metadata. Therefore, we should add basic infrastructure wrapping the relevant WordPress API (similar to e.g. Options and User_Options classes). See more information on the overall approach.


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

Acceptance criteria

  • There should be a new interface Google\Site_Kit\Core\Storage\Post_Meta_Interface, with the following methods:
    • get( int $post_id, string $key, bool $single = false ) : mixed: Gets the value of the given post meta key.
    • update( int $post_id, string $key, mixed $value, mixed $prev_value = '' ): bool: Updates the value for a post meta key.
    • add( int $post_id, string $key, mixed $value, bool $unique = false ) : bool: Adds a new value for a post meta key.
    • delete( int $post_id, string $key, mixed $value = '' ): bool: Deletes the given post meta key.
  • There should be a new class Google\Site_Kit\Core\Storage\Post_Meta which implements the above interface:
    • get should rely on get_post_meta.
    • update should rely on update_post_meta.
    • add should rely on add_post_meta.
    • delete should rely on delete_post_meta.
  • There should be a new abstract class Google\Site_Kit\Core\Storage\Post_Meta_Setting:
    • Relies on a META_KEY class constant, to be defined in each sub-class.
    • Relies on a Post_Meta_Interface being passed into the constructor.
    • Has a public register() method, which calls register_meta( 'post', ... ). It should rely on protected methods get_type() and get_sanitize_callback() which can be overridden by each sub-class (similar to e.g. in the existing User_Setting class. The single argument should be enforced to true for now.
    • Should have methods get( $post_id ), set( $post_id ) and delete( $post_id ) which wrap the respective Post_Meta methods.
      • get should always use $single as true, and it should return the value of a protected get_default() method if no actual meta value is set.
        • It would be more elegant to solve this at registration level; however, WordPress only started supporting registering a default meta value in version 5.5, but we support back to 4.7, so we can't rely on this.
      • set should use update for now, since for $single meta keys that is always appropriate.
      • In general, the more granular features of Post_Meta::update and Post_Meta::delete, as well as Post_Meta::add overall won't be needed as long as we only deal with $single meta keys. They should still be available in the lower-level infrastructure to allow flexibility.
  • When resetting Site Kit, all post metadata starting in googlesitekit_ should be wiped.
  • Test coverage for the new classes needs to be added.

Implementation Brief

Create a new interface /includes/Core/Storage/Post_Meta_Interface.php with the following methods, params, return types and documentation as specified in AC.

Create a new class implementing the interface /includes/Core/Storage/Post_Meta.php
final class Post_Meta implements Post_Meta_Interface implementing the four methods using the relevant *_post_meta functions as specified in the AC.

Create a new abstract class /includes/Core/Storage/Post_Meta_Setting.php following the ACs, here is a skeleton to start you off

abstract class Post_Meta_Setting {
	const META_KEY = '';

	public function __construct( Post_Meta_Interface $post_meta ) {
		$this->post_meta = $post_meta;
	}

	public function register() {
		register_meta(
			'user',
			$this->post_meta->get( static::META_KEY ),
			array(
				'type'              => $this->get_type(),
				'sanitize_callback' => $this->get_sanitize_callback(),
				'single'            => true,
			)
		);
	}

	protected function get_type() {
		return 'string';
	}

	protected function get_default() {
		return '';
	}

	public function get( $post_id ) {
		$value = $this->post_meta->get( $post_id, static::META_KEY, true );
		return empty( $value ) ? $this->get_default() : $value;
	}

	public function set( $post_id, $value ) {
		return $this->post_meta->update( $post_id, static::META_KEY, $value );
	}   

	public function get_sanitize_callback() {
		return null;
	}

Update Google\Site_Kit\Core\Util\Reset::all() to handle post_meta

public function all() {

$this->delete_post_meta( $scope = 'site' or 'network' ) take a look at either delete_options() or delete_user_options() to help build the query to delete the post_meta.

There is also Reset_Persistent, but it won't require any changes
https://github.com/google/site-kit-wp/blob/17a5a8c3800c8ae68d7c8982b36b9ae83118ea72/includes/Core/Util/Reset_Persistent.php

Test Coverage

  • Add tests to tests/phpunit/integration/Core/Storage/Post_MetaTest.php
  • Update cli tests in tests/phpunit/integration/Core/Util/ResetTest.php
  • Update cli tests in tests/phpunit/integration/Core/Util/ResetPersistentTest.php

Make sure wp google-site-kit reset works for postmeta googlesitekit_foo
Make sure wp google-site-kit reset --persistent works for postmeta

Visual Regression Changes

  • N/A

QA Brief

QA:Eng

  • First do this in a single site, then in a network:
  • Add a post meta with a key googlesitekit_foo and another with a post meta with a key googlesitekitpersistent_foo.
  • Ensure wp google-site-kit reset deletes only googlesitekit_foo
  • Ensure wp google-site-kit reset --persistent deletes both googlesitekit_foo and googlesitekitpersistent_foo

Changelog entry

  • Introduce PHP classes for modeling post metadata in Site Kit.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature QA: Eng Requires specialized QA by an engineer labels Apr 14, 2021
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Apr 14, 2021
@felixarntz felixarntz added the Module: Idea Hub Google Idea Hub module related issues label Apr 15, 2021
@fhollis fhollis added this to the Sprint 48 milestone Apr 21, 2021
@tofumatt tofumatt self-assigned this Apr 21, 2021
@tofumatt
Copy link
Collaborator

IB ✅

@aaemnnosttv
Copy link
Collaborator

@felixarntz just a thought which we could change later – why not make Post_Meta_Interface a Meta_Interface, as all of the post-specific details will be handled by the implementing class? We could always have a Post_Meta_Interface if we wanted, which could extend the Meta_Interface but just thing a more generic interface might make sense here since WP uses generic functions internally as well.

@felixarntz
Copy link
Member Author

@aaemnnosttv I was thinking about that too, but I think for now the Post_Meta_Interface allows us to be a bit more explicit about even parameter names (e.g. $post_id instead of generic $object_id), which arguably makes it more intuitive. I agree we can abstract further later if we need another *_Meta_Interface, at which point having Meta_Interface probably makes more sense.

@Hazlitte Hazlitte self-assigned this May 6, 2021
@Hazlitte
Copy link
Contributor

Hazlitte commented May 6, 2021

QA Eng ✅

@Hazlitte Hazlitte removed their assignment May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Idea Hub Google Idea Hub module related issues P0 High 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

7 participants