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

Update @wordpress packages #1967

Closed
wants to merge 32 commits into from

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Nov 29, 2021

Update packages to include these bug fixes from Gutenberg:

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


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.

@noisysocks
Copy link
Member Author

noisysocks commented Nov 29, 2021

I've yet to test this. It's possible that some or all of these need to be committed first:

@noisysocks
Copy link
Member Author

I've merged #1937 into this branch.

@noisysocks
Copy link
Member Author

noisysocks commented Nov 29, 2021

@Mamaduka: Could you please fix the is_custom PHP unit test failures here? I probably messed up porting the PHP changes in WordPress/gutenberg#36911. It's confusing because Tests_REST_WpRestTemplatesController seems to have some pretty significant differences between Core and the plugin.

@adamziel: Do you know why Tests_REST_WpRestTemplatesController::test_get_item_works_with_a_single_slash from WordPress/gutenberg#36881 fails here?

@Mamaduka
Copy link
Member

@noisysocks we should probably sync Tests_REST_WpRestTemplatesController tests back into plugin.

@hellofromtonya
Copy link
Contributor

Noting this Trac ticket https://core.trac.wordpress.org/ticket/54532 and Gutenberg issue WordPress/gutenberg#36962 for consideration before Beta 1.

@oandregal
Copy link
Member

oandregal commented Nov 29, 2021

Robert, JFYI, in case is not merged by the time you read this. #1971 would be nice to have for Beta 1 (Nov 30) as it adds a couple of missing theme.json flags missing from the the last backport.

Update: I've noticed we were missing another flag (appearanceTools) in this backport, so I've prepared a single PR to add them all at #1973

@oandregal
Copy link
Member

Hi, looking at the class-theme-json.php file I've found a thing to fix and a couple to improve. Apparently, I can't prepare a PR on top of this one, so I thought it'd be best in this case to share a patch to be applied by git apply <file.patch>:

The contents of file.patch are:

diff --git a/src/wp-includes/class-wp-theme-json.php b/src/wp-includes/class-wp-theme-json.php
index 4cdb44e457..635f670cff 100644
--- a/src/wp-includes/class-wp-theme-json.php
+++ b/src/wp-includes/class-wp-theme-json.php
@@ -75,17 +75,28 @@ class WP_Theme_JSON {
 	 * This contains the necessary metadata to process them:
 	 *
 	 * - path       => where to find the preset within the settings section
-+	 * - override   => whether a theme preset with the same slug as a default preset
-+	 *                 can override it
+	 * - override   => whether a theme preset with the same slug as a default preset
+	 *                 can override it
 	 * - value_key  => the key that represents the value
-	 * - value_func => the callback to render the value (either value_key or value_func should be present)
+	 * - value_func => optionally, instead of value_key, a function to generate
+	 *                 the value that takes a preset as an argument
+	 *                 (either value_key or value_func should be present)
 	 * - css_vars   => template string to use in generating the CSS Custom Property.
 	 *                 Example output: "--wp--preset--duotone--blue: <value>" will generate as many CSS Custom Properties as presets defined
 	 *                 substituting the $slug for the slug's value for each preset value.
-	 * - classes    => array containing a structure with the classes to generate for the presets.
-	 *                 Each key is a template string to resolve similarly to the css_vars and each value is the CSS property to use for that class.
-	 *                 Example output: ".has-blue-color { color: <value> }"
-	 * - properties  => a list of CSS properties to be used by kses to check the preset value is safe.
+	 * - classes    => array containing a structure with the classes to
+	 *                 generate for the presets, where for each array item
+	 *                 the key is the class name and the value the property name.
+	 *                 The "$slug" substring will be replaced by the slug of each preset.
+	 *                 For example:
+	 *                 'classes' => array(
+	 *                   '.has-$slug-color'            => 'color',
+	 *                   '.has-$slug-background-color' => 'background-color',
+	 *                   '.has-$slug-border-color'     => 'border-color',
+	 *                 )
+	 * - properties => array of CSS properties to be used by kses to
+	 *                 validate the content of each preset
+	 *                 by means of the remove_insecure_properties method.
 	 *
 	 * @since 5.8.0
 	 * @since 5.9.0 Added new presets and simplified the metadata structure.
@@ -305,6 +316,8 @@ class WP_Theme_JSON {
 	);
 
 	/**
+	 * The valid elements that can be found under styles.
+	 *
 	 * @since 5.8.0
 	 * @var string[]
 	 */
@@ -479,6 +492,7 @@ class WP_Theme_JSON {
 
 		return $output;
 	}
+
 	/**
 	 * Returns the metadata for each block.
 	 *

@@ -75,6 +75,8 @@ class WP_Theme_JSON {
* This contains the necessary metadata to process them:
*
* - path => where to find the preset within the settings section
+ * - override => whether a theme preset with the same slug as a default preset
Copy link
Member

Choose a reason for hiding this comment

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

There're a couple of extra + here. I've sent a patch to fix that and other minor changes at #1967 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I've also prepared this Gutenberg PR WordPress/gutenberg#36973 to make sure that the order of constants, methods, etc. as well as the PHPDoc comments are the same between the Gutenberg's lib/class-wp-theme-json-gutenberg.php and WordPress core wp-includes/class-wp-theme-json.php.

This way the backports are way easier and require minimal manual intervention.

@oandregal
Copy link
Member

Got to review the class-wp-theme-json-resolver.php class as well. I've only noticed minor changes to the comments:

diff --git a/src/wp-includes/class-wp-theme-json-resolver.php b/src/wp-includes/class-wp-theme-json-resolver.php
index 6a7e6a5bf9..659365b6ef 100644
--- a/src/wp-includes/class-wp-theme-json-resolver.php
+++ b/src/wp-includes/class-wp-theme-json-resolver.php
@@ -230,7 +230,6 @@ class WP_Theme_JSON_Resolver {
         * @param array    $post_status_filter Filter Optional. custom post type by
         *                                     post status.  ['publish'] by default,
         *                                     so it only fetches published posts.
-        *
         * @return array Custom Post Type for the user's origin config.
         */
        public static function get_user_data_from_custom_post_type( $theme, $should_create_cpt = false, $post_status_filter = array( 'publish' ) ) {
@@ -333,8 +332,8 @@ class WP_Theme_JSON_Resolver {
 
        /**
         * There are three sources of data (origins) for a site:
-        * default, theme, and user. The user's has higher priority
-        * than the theme's, and the theme's higher than core's.
+        * default, theme, and custom. The custom's has higher priority
+        * than the theme's, and the theme's higher than defaults's.
         *
         * Unlike the getters {@link get_core_data},
         * {@link get_theme_data}, and {@link get_user_data},

I'm making some changes to the equivalent Gutenberg class to make the backports easier at WordPress/gutenberg#36974

@spacedmonkey
Copy link
Member

We need to ensure that WordPress/gutenberg#36981 is merged and backported as well.

@noisysocks
Copy link
Member Author

Thanks @oandregal for the review and fixes. I'm going to apply those patches and commit this patch, and then follow up with a second commit to address WordPress/gutenberg#36962 and WordPress/gutenberg#36981. That way if I run into roadblocks we'll still have something in for beta 1 😀

@hellofromtonya
Copy link
Contributor

@noisysocks need to make sure that the blocks/navigation-area folder and file are also removed from Core on package update.

@noisysocks
Copy link
Member Author

@noisysocks need to make sure that the blocks/navigation-area folder and file are also removed from Core on package update.

This patch doesn't modify navigation-area. I've rebased this branch though so that that folder doesn't exist in this branch.

@noisysocks
Copy link
Member Author

Gave this a smoke test locally and nothing seems amiss. Committing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants