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

spacedmonkey · 2025-01-21T23:33:38Z

Add filters for site metadata handling in multisite setups

Introduce filters for get, update, add, and delete site metadata, specific to multisite installations. These functions extend metadata API compatibility while marking their use as deprecated in favor of network options functions. Warnings are added to guide developers toward preferred alternatives.

Trac ticket: https://core.trac.wordpress.org/ticket/61467

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Introduce filters for get, update, add, and delete site metadata, specific to multisite installations. These functions extend metadata API compatibility while marking their use as deprecated in favor of network options functions. Warnings are added to guide developers toward preferred alternatives.

The $unique parameter was passed but not used in add_network_option. This change cleans up the function call for consistency and clarity. It aligns with the intended usage of add_network_option.

github-actions · 2025-01-21T23:33:54Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props spacedmonkey.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2025-01-21T23:48:36Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

felixarntz

@spacedmonkey I like the overall idea behind this, routing the site metadata function calls through to the corresponding *_network_option() function. However, I think we can improve the approach a little:

I think all the warnings and messages related to deprecation are unnecessary.
We should always either use the corresponding *_network_option() function or return a specific short-circuit value, but we should never return $current here, because that would create inconsistent behavior where sometimes the regular metadata logic would still run.

felixarntz · 2025-01-22T19:29:08Z

src/wp-includes/option.php

+	if ( ! is_multisite() ) {
+		_doing_it_wrong( 'get_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
+		return false;
+	}


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.

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.

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 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.

felixarntz · 2025-01-22T19:33:40Z

src/wp-includes/option.php

+		_doing_it_wrong( 'get_metadata', __( 'The $single parameter is not supported in this function.' ), '6.8.0' );
+		return $current;


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();

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.

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.

felixarntz · 2025-01-22T19:34:29Z

src/wp-includes/option.php

+		return $current;
+	}
+
+	_doing_it_wrong( 'get_metadata', __( 'This function is deprecated. Use get_network_option() instead.' ), '6.8.0' );


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

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.

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.

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.

felixarntz · 2025-01-22T19:35:12Z

src/wp-includes/option.php

+	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 );


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 );

felixarntz · 2025-01-22T19:37:46Z

src/wp-includes/option.php

+	if ( ! is_multisite() ) {
+		_doing_it_wrong( 'add_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
+		return false;
+	}


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

felixarntz · 2025-01-22T19:41:01Z

src/wp-includes/option.php

+	if ( ! $unique ) {
+		_doing_it_wrong( 'get_metadata', __( 'The $unique parameter is not supported in this function.' ), '6.8.0' );
+		return $current;
+	}


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;

}

}

felixarntz · 2025-01-22T19:41:33Z

src/wp-includes/option.php

+		return $current;
+	}
+
+	_doing_it_wrong( 'add_metadata', __( 'This function is deprecated. Use add_network_option() instead.' ), '6.8.0' );


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

felixarntz · 2025-01-22T19:44:18Z

src/wp-includes/option.php

+	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 );


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 );

spacedmonkey

I disagree, that the doing it wrong messages are not needed. I think we want to deprecate the miss use of metadata api for network options.

spacedmonkey · 2025-01-23T17:07:18Z

src/wp-includes/option.php

+	if ( ! is_multisite() ) {
+		_doing_it_wrong( 'get_metadata', __( 'This function is only available in multisite installations.' ), '6.8.0' );
+		return false;
+	}


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.

spacedmonkey · 2025-01-23T17:09:11Z

src/wp-includes/option.php

+		_doing_it_wrong( 'get_metadata', __( 'The $single parameter is not supported in this function.' ), '6.8.0' );
+		return $current;


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.

spacedmonkey · 2025-01-23T17:10:22Z

src/wp-includes/option.php

+		return $current;
+	}
+
+	_doing_it_wrong( 'get_metadata', __( 'This function is deprecated. Use get_network_option() instead.' ), '6.8.0' );


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.

spacedmonkey added 2 commits January 21, 2025 18:30

Remove unused $unique parameter from add_metadata call.

26d62da

The $unique parameter was passed but not used in add_network_option. This change cleans up the function call for consistency and clarity. It aligns with the intended usage of add_network_option.

spacedmonkey requested review from felixarntz, JJJ and peterwilsoncc January 21, 2025 23:42

felixarntz requested changes Jan 22, 2025

View reviewed changes

spacedmonkey commented Jan 23, 2025

View reviewed changes

spacedmonkey requested a review from swissspidy January 23, 2025 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

spacedmonkey commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

felixarntz left a comment

felixarntz Jan 22, 2025

spacedmonkey Jan 23, 2025

felixarntz Jan 23, 2025

spacedmonkey Jan 23, 2025

felixarntz Jan 22, 2025

spacedmonkey Jan 23, 2025

felixarntz Jan 23, 2025

felixarntz Jan 22, 2025

spacedmonkey Jan 23, 2025

felixarntz Jan 23, 2025

spacedmonkey Jan 23, 2025

felixarntz Jan 22, 2025

felixarntz Jan 22, 2025

felixarntz Jan 22, 2025

felixarntz Jan 22, 2025

felixarntz Jan 22, 2025

spacedmonkey left a comment

spacedmonkey Jan 23, 2025

spacedmonkey Jan 23, 2025

spacedmonkey Jan 23, 2025

		_doing_it_wrong( 'get_metadata', __( 'The $single parameter is not supported in this function.' ), '6.8.0' );
		return $current;

-		_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();

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

Are you sure you want to change the base?

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

Conversation

spacedmonkey commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

Test using WordPress Playground

Some things to be aware of

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacedmonkey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment