Add a "type" field to the meta and options tables #3124

petitphp · 2022-08-23T22:44:57Z

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

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.

…d formatting the type of values saved and retrieved from metadata and options.

This was causing issue with dbDelta when parsing existing SQL schema where the field type would be truncated after the first space in the enum list.

barryhughes · 2022-09-28T01:01:43Z

src/wp-includes/database-types.php

+ *
+ * @return array List of supported database types.
+ */
+function wp_get_database_types() {


The name of this function, and its related functions, is just a little confusing to me. Could something like wp_get_meta_storage_types() be a better choice?

(I realize this PR is still a draft—this is just a small spot of early feedback.)

Yep, needs a better name imho too. How about wp_get_database_data_storage_types() or maybe wp_get_db_storage_types_for_data() as this will be for meta and options.

azaozz · 2023-03-11T01:54:38Z

src/wp-admin/includes/schema.php

+	$enum_col     = "'" . implode( "','", array_map( 'esc_sql', wp_get_database_types() ) ) . "'";
+	$default_enum = esc_sql( wp_get_database_default_type() );
+


Thinking better if these are hard-coded. This will be using the PHP data types, right? What's the point to support changing them? Option and meta values are processed in PHP.

Even if we assume that there is an edge case that might need different types, changing that from a plugin and then disabling the plugin will "wreak havoc" in a big way :)

azaozz

Thanks for working on this!

Added quite a few inline comments, mostly to start a discussion about how exactly to implement this without breaking back-compat, and about what should be filterable/changeable by plugins and why.

azaozz · 2023-03-11T01:56:01Z

src/wp-admin/includes/schema.php

+		$value_type = wp_get_database_type_for_value( $value );
+		$value      = wp_prepare_value_for_db( $value );


Same as https://github.com/WordPress/wordpress-develop/pull/3124/files#r1133008585.

azaozz · 2023-03-11T02:00:02Z

src/wp-includes/database-types.php

+ *
+ * @return array List of supported database types.
+ */
+function wp_get_database_types() {


Yep, needs a better name imho too. How about wp_get_database_data_storage_types() or maybe wp_get_db_storage_types_for_data() as this will be for meta and options.

azaozz · 2023-03-11T02:07:33Z

src/wp-includes/default-constants.php

+
+	/**#@+
+	 * Constants for declaring data types for metadata and option tables.
+	 *
+	 * @since n.e.x.t
+	 */
+	define( 'WP_TYPE_BOOLEAN', 'boolean' );
+	define( 'WP_TYPE_INTEGER', 'integer' );
+	define( 'WP_TYPE_FLOAT', 'float' );
+	define( 'WP_TYPE_STRING', 'string' );
+	define( 'WP_TYPE_ARRAY', 'array' );
+	define( 'WP_TYPE_OBJECT', 'object' );
+	define( 'WP_TYPE_UNKNOWN', 'unknown' );
+	/**#@-*/
+
+	define( 'WP_DEFAULT_TYPE', WP_TYPE_UNKNOWN );


Been wondering if these constants are actually needed. Anybody thinks it is possible to ever change WP_TYPE_BOOLEAN to be something else than boolean?

Seems having a function (see above) to return these types and having adequate docblock/explanation what they are and why would be enough?

azaozz · 2023-03-11T02:14:16Z

src/wp-includes/database-types.php

+ *
+ * @return string
+ */
+function wp_get_database_type_for_value( $value ) {


May need changes if the WP_TYPE_* constants are removed. Perhaps something like:

$original_value_type = gettype( $value ); // May need adjusting for float/double if ( in_array( $original_value_type, wp_get_database_types(), true ) ) { return $original_value_type; } return 'unknown';

azaozz · 2023-03-11T02:17:48Z

src/wp-includes/database-types.php

+	 * @param string $original_value_type value's PHP type
+	 * @param mixed $value current value
+	 */
+	return apply_filters( 'database_type_for_value', $value_type, $original_value_type, $value );


Not sure a filter here is a good idea. Who would use it and why? DB types should match PHP types and be hard-coded imho.

azaozz · 2023-03-11T02:55:03Z

src/wp-includes/database-types.php

+	} elseif ( WP_TYPE_UNKNOWN === $type ) {
+		$formatted_value = maybe_unserialize( $value );


This is the place/code that should handle pre-existing values (where the type is not set).

Thinking this can be improved significantly by making it possible to distinguish when we're looking at an old value without a type, and when we're looking at newly saved value that is of unknown type. maybe_unserialize() should run for old values, but is pointless for new, "unknown" data. It also is pretty slow, and a little buggy, better to skip it as much as possible.

This means that the default value for the new column in the DB would have to be different from "unknown". Can perhaps be empty string or null.

azaozz · 2023-03-11T03:01:49Z

src/wp-includes/meta.php

+
+/**
+ * Check if the object support database type.
+ *
+ * @since n.e.x.t
+ *
+ * @param string $object_type Type of object.
+ *
+ * @return bool True mean the object support database type, false otherwise.
+ */
+function wp_check_object_support_meta_value_type( $object_type ) {
+	$object_type_supported = in_array( $object_type, array( 'post', 'comment', 'term', 'user' ), true );
+
+	/**
+	 * Filters if the object type support database type.
+	 *
+	 * Any object type declaring support database type must have the corresponding column in its meta table.
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param bool $object_type_supported Support flag for the object type.
+	 * @param string $object_type Type of object.
+	 */
+	return apply_filters( '', $object_type_supported, $object_type );
+}


Not sure why this function is needed.

This is to ensure that calling add_metadata or update_metadata with a custom $meta_type wouldn't cause an SQL error if the schema of the custom metadata table hadn't been updated yet with the new column for the type.

azaozz · 2023-03-11T03:02:48Z

src/wp-includes/meta.php

+/**
+ * Prepare metadata value for the database.
+ *
+ * Check if the object supports database type and default back to using `maybe_serialize` otherwise.
+ *
+ * @since n.e.x.t
+ *
+ * @param string $object_type Type of object.
+ * @param mixed $meta_value Metadata value.
+ *
+ * @return mixed
+ */
+function wp_prepare_metadata_value_for_db( $object_type, $meta_value ) {
+	if ( ! wp_check_object_support_meta_value_type( $object_type ) ) {
+		return maybe_serialize( $meta_value );
+	}
+
+	return wp_prepare_value_for_db( $meta_value );
+}


Not sure why this is needed. Should use wp_prepare_value_for_db() directly?

azaozz · 2023-03-11T03:03:39Z

src/wp-includes/meta.php

+/**
+ * Format a metadata value from the database.
+ *
+ * Check if the object supports database type and default back to using `maybe_unserialize` otherwise.
+ *
+ * @since n.e.x.t
+ *
+ * @param string $object_type Type of object.
+ * @param mixed $meta_value Raw metadata value from database.
+ * @param string $meta_value_type Metadata value type from database.
+ *
+ * @return mixed
+ */
+function wp_format_metadata_from_db( $object_type, $meta_value, $meta_value_type ) {
+	if ( ! wp_check_object_support_meta_value_type( $object_type ) ) {
+		return maybe_unserialize( $meta_value );
+	}
+
+	return wp_format_value_from_db( $meta_value_type, $meta_value );
+}


Here too, not sure why this is needed.

azaozz · 2023-03-11T03:06:29Z

src/wp-settings.php

@@ -114,6 +114,7 @@
 require ABSPATH . WPINC . '/class-wp.php';
 require ABSPATH . WPINC . '/class-wp-error.php';
 require ABSPATH . WPINC . '/pomo/mo.php';
+require ABSPATH . WPINC . '/database-types.php';


Typo? The tests/phpunit/tests/database-types.php file is included automatically when running PHPUnit.

petitphp force-pushed the ticket/55942 branch from 07978f8 to d40fcd9 Compare September 5, 2022 13:06

petitphp added 5 commits September 20, 2022 10:46

Introduce a new database types API to handle evaluating, preparing an…

0a3592c

…d formatting the type of values saved and retrieved from metadata and options.

Update metadata and option tables schema to include the new type column.

5377921

Implement database type for metadata and options.

5c91d80

Remove space after coma in database type enum value

04db85a

This was causing issue with dbDelta when parsing existing SQL schema where the field type would be truncated after the first space in the enum list.

Update tests to use the correct type in assertions

aa5080b

petitphp force-pushed the ticket/55942 branch from 6f8d36f to aa5080b Compare September 20, 2022 10:42

PHPCS fix

6ba3ffe

barryhughes reviewed Sep 28, 2022

View reviewed changes

azaozz reviewed Mar 11, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/trunk' into pr/3124

7806bd6

petitphp closed this Jul 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a "type" field to the meta and options tables #3124

Add a "type" field to the meta and options tables #3124

petitphp commented Aug 23, 2022

barryhughes Sep 28, 2022

azaozz Mar 11, 2023

azaozz Mar 11, 2023

azaozz left a comment

azaozz Mar 11, 2023

azaozz Mar 11, 2023

azaozz Mar 11, 2023

azaozz Mar 11, 2023

azaozz Mar 11, 2023

azaozz Mar 11, 2023 •

edited

Loading

azaozz Mar 11, 2023 •

edited

Loading

petitphp Mar 20, 2023

azaozz Mar 11, 2023

azaozz Mar 11, 2023

azaozz Mar 11, 2023 •

edited

Loading

		$enum_col = "'" . implode( "','", array_map( 'esc_sql', wp_get_database_types() ) ) . "'";
		$default_enum = esc_sql( wp_get_database_default_type() );

		$value_type = wp_get_database_type_for_value( $value );
		$value = wp_prepare_value_for_db( $value );

		} elseif ( WP_TYPE_UNKNOWN === $type ) {
		$formatted_value = maybe_unserialize( $value );

Add a "type" field to the meta and options tables #3124

Add a "type" field to the meta and options tables #3124

Conversation

petitphp commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz 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

azaozz Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

azaozz Mar 11, 2023 • edited Loading

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

azaozz Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

azaozz Mar 11, 2023 •

edited

Loading

azaozz Mar 11, 2023 •

edited

Loading

azaozz Mar 11, 2023 •

edited

Loading