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

61467 - Filter metadata api for site ( network ) meta. #8161

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/wp-includes/default-filters.php
Original file line number Diff line number Diff line change
Expand Up @@ -778,4 +778,10 @@
add_filter( 'rest_prepare_wp_block', 'insert_hooked_blocks_into_rest_response', 10, 2 );
add_filter( 'rest_prepare_wp_navigation', 'insert_hooked_blocks_into_rest_response', 10, 2 );

// Add filters to metadata API when calling site ( network ) metadata.
add_filter( 'get_site_metadata', 'filter_get_site_metadata', 10, 4 );
add_filter( 'update_site_metadata', 'filter_update_site_metadata', 10, 4 );
add_filter( 'add_site_metadata', 'filter_add_site_metadata', 10, 5 );
add_filter( 'delete_site_metadata', 'filter_delete_site_metadata', 10, 5 );

unset( $filter, $action );
123 changes: 123 additions & 0 deletions src/wp-includes/option.php
Original file line number Diff line number Diff line change
Expand Up @@ -3214,3 +3214,126 @@ function wp_autoload_values_to_autoload() {

return array_intersect( $filtered_values, $autoload_values );
}


/**
* Filters site metadata for multisite installations.
*
* This function is specific to multisite setups and provides a way to retrieve
* network options. It enforces parameters' constraints and suggests using an alternative function.
*
* @param mixed $current The value to return if no changes are made.
* @param int $object_id Site ID.
* @param string $meta_key Metadata key.
* @param bool $single Whether to retrieve a single value. Must be true.
*
* @return mixed The network option value for the specified meta key, or false if the function is used incorrectly.
*/
function filter_get_site_metadata( $current, $object_id, $meta_key, $single ) {
if ( ! is_multisite() ) {
_doing_it_wrong( 'get_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
return false;
}
Comment on lines +3233 to +3236
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On non-multisite, get_network_option() still works and falls through to get_option(). So it would make sense to me to align with that. We could remove this condition since get_network_option() already handles it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think should do that. currently, get_metadata( 'site', 1, 'site_url' ) in a single site, would result false, as it should. It is start returning the option, than that could be considered a breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a breaking change to return another value - the function is expected to return anything, so returning anything is not a breaking change. If you argue that way, even using get_network_option() here at all would be a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a breaking change to return another value - the function is expected to return anything, so returning anything is not a breaking change. If you argue that way, even using get_network_option() here at all would be a breaking change.

I am confused by this feedback. Currently if I called get_metadata( 'site', 1, 'site_url' ), what happens if is try to load data from the sitemeta table. If table doesn't exist, it will result false. If we use get_network_option it will load from option the response of the function call goes from false to option value. That is a different and breaking change for the code.


if ( ! $single ) {
_doing_it_wrong( 'get_metadata', __( 'The $single parameter is not supported in this function.' ), '6.8.0' );
return $current;
Comment on lines +3239 to +3240
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is correct that ! $single does not work when using with network options, I don't think we should allow the function to pass through when $single is false. I think we have two options here:

  • Either return an empty array because it's not valid.
  • Or return an array with just the option value as the sole element.

Here is an example for the 2nd option:

Suggested change
_doing_it_wrong( 'get_metadata', __( 'The $single parameter is not supported in this function.' ), '6.8.0' );
return $current;
$option_value = get_network_option( $object_id, $meta_key, false );
if ( false !== $option_value ) {
return array( $option_value );
}
return array();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, it would a breaking change. If for some reason you were using network options to store multiple values in the same key, the other keys that become in accessible for this change. My hope is that the doing it wrong, will tell developers to stop doing this and we can change this in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would only be a breaking change if someone was actually using this to store multiple values - which I doubt is common. Probably the most common usage of the metadata functions for network options is via WP-CLI.

I'm on board with not changing the return value here for now to first only warn developers about this. But the warning should only appear if there are actually multiple values stored. The problem is not that someone is calling this function with $single = false - the problem is when there are multiple values stored for what should be a single network option.

So we would need to conditionally trigger a warning via another filter, once the value has been retrieved, and only if it's multiple values.

Even more important would be to flag this in the filter callback for add_metadata: If there's already a value stored and you call this function, it should not be allowed to store multiple values.

But here we need basically the same warning, in order to account for data that was already stored before this change.

}

_doing_it_wrong( 'get_metadata', __( 'This function is deprecated. Use get_network_option() instead.' ), '6.8.0' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my overarching comment, I don't think all those warnings and deprecation messages are needed. We can make this work without them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want to push home to developers, that if you are doing this, you are doing this wrong. But I still want the code to work. If developers know what they are doing and don't care, they can always unhook these filters and do use the metadata api as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you that some of these warnings make sense. But using the metadata functions for network options is not necessarily wrong. It may not be recommended, but for the most part it works.

There is only one thing that is wrong: Nobody should be able to store multiple meta values for a single meta key in sitemeta, since it's intended for network options, and those are restricted to one value per key. So I think this is the only aspect where developers would be doing something critically wrong and should be warned about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It IS wrong, as caches ( specially if external object caches ) values for options are not updated / created / delete. It also does not run through the existing filters and actions for network options. I think there are serious issues and should be pushing developers to not do this.


return get_network_option( $object_id, $meta_key );
}

/**
* Filters and updates site metadata in a multisite environment.
*
* This function is only applicable in multisite installations. It has been
* deprecated in favor of using `update_network_option()`.
*
* @param mixed $current The current metadata value. Unused.
* @param int $object_id Site ID for which the metadata is being updated.
* @param string $meta_key Metadata key to update.
* @param mixed $meta_value Metadata value to update.
*
* @return bool|void False if the operation is not applicable or invalid,
* or the result of `update_network_option()` on success.
*/
function filter_update_site_metadata( $current, $object_id, $meta_key, $meta_value ) {
if ( ! is_multisite() ) {
_doing_it_wrong( 'update_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
return false;
}

_doing_it_wrong( 'update_metadata', __( 'This function is deprecated. Use update_network_option() instead.' ), '6.8.0' );

return update_network_option( $object_id, $meta_key, $meta_value );
Comment on lines +3263 to +3270
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, I think this could simply become:

Suggested change
if ( ! is_multisite() ) {
_doing_it_wrong( 'update_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
return false;
}
_doing_it_wrong( 'update_metadata', __( 'This function is deprecated. Use update_network_option() instead.' ), '6.8.0' );
return update_network_option( $object_id, $meta_key, $meta_value );
return update_network_option( $object_id, $meta_key, $meta_value );

}

/**
* Filters the addition of site metadata.
*
* This function is a custom implementation for adding metadata in a multisite environment.
* It ensures that the metadata API handles this case correctly.
* This function is deprecated and advises using `add_network_option()` instead.
* If WordPress is not running in a multisite environment or if the `$unique` parameter is false,
* it will return `$current` or false as appropriate.
*
* @param mixed $current The return value of the original filter function.
* @param int $object_id ID of the object (site ID).
* @param string $meta_key Metadata key.
* @param mixed $meta_value Metadata value. Must be serializable if non-scalar.
* @param bool $unique Whether the metadata key should be unique. If false, `$current` is returned.
*
* @return bool|mixed False on failure, `$current` if the `$unique` parameter is false, or the result of `add_network_option()`.
*/
function filter_add_site_metadata( $current, $object_id, $meta_key, $meta_value, $unique ) {
if ( ! is_multisite() ) {
_doing_it_wrong( 'add_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
return false;
}
Comment on lines +3291 to +3294
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above: We can remove this check and call add_network_option() as it works outside of Multisite too.


if ( ! $unique ) {
_doing_it_wrong( 'get_metadata', __( 'The $unique parameter is not supported in this function.' ), '6.8.0' );
return $current;
}
Comment on lines +3296 to +3299
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make this a bit safer: If $unique is false, this is only a problem if there is already a value for that option. Also, we shouldn't return $current as that would allow the function to proceed with its regular metadata logic, which would not be aligned with the changes this PR otherwise makes.

For example we could do this:

Suggested change
if ( ! $unique ) {
_doing_it_wrong( 'get_metadata', __( 'The $unique parameter is not supported in this function.' ), '6.8.0' );
return $current;
}
if ( ! $unique ) {
$option_value = get_network_option( $object_id, $meta_key, false );
if ( false !== $option_value ) {
return false;
}
}


_doing_it_wrong( 'add_metadata', __( 'This function is deprecated. Use add_network_option() instead.' ), '6.8.0' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, these deprecation warnings are not necessary, and they would be confusing because those functions are actually not deprecated.


return add_network_option( $object_id, $meta_key, $meta_value );
}

/**
* Filters site metadata deletion in multisite installations.
*
* This function ensures that metadata deletion adheres to multisite-specific rules.
* It also deprecates specific parameters and suggests using `delete_network_option()` instead.
*
* @param mixed $current The value to return if the function short-circuits.
* @param int $object_id The object ID, typically the site ID.
* @param string $meta_key The meta key to delete.
* @param mixed $meta_value The meta value to delete. Not supported in this function.
* @param bool $delete_all Whether to delete the matching metadata entries for all objects, ignoring the object ID. Not supported in this function.
*
* @return mixed The filtered result of the metadata deletion operation.
*/
function filter_delete_site_metadata( $current, $object_id, $meta_key, $meta_value, $delete_all ) {
if ( ! is_multisite() ) {
_doing_it_wrong( 'delete_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
return false;
}

if ( $delete_all ) {
_doing_it_wrong( 'delete_metadata', __( 'The $delete_all parameter is not supported in this function.' ), '6.8.0' );
return $current;
}

if ( $meta_value ) {
_doing_it_wrong( 'delete_metadata', __( 'The $meta_value parameter is not supported in this function.' ), '6.8.0' );
return $current;
}

_doing_it_wrong( 'delete_metadata', __( 'This function is deprecated. Use delete_network_option() instead.' ), '6.8.0' );

return delete_network_option( $object_id, $meta_key );
Comment on lines +3321 to +3338
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the above feedback, this can be simplified to:

Suggested change
if ( ! is_multisite() ) {
_doing_it_wrong( 'delete_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
return false;
}
if ( $delete_all ) {
_doing_it_wrong( 'delete_metadata', __( 'The $delete_all parameter is not supported in this function.' ), '6.8.0' );
return $current;
}
if ( $meta_value ) {
_doing_it_wrong( 'delete_metadata', __( 'The $meta_value parameter is not supported in this function.' ), '6.8.0' );
return $current;
}
_doing_it_wrong( 'delete_metadata', __( 'This function is deprecated. Use delete_network_option() instead.' ), '6.8.0' );
return delete_network_option( $object_id, $meta_key );
if ( $delete_all ) {
return false;
}
if ( $meta_value ) {
$option_value = get_network_option( $object_id, $meta_key, false );
if ( $meta_value !== $option_value ) {
return false;
}
}
return delete_network_option( $object_id, $meta_key );

}
Loading