Sync Posts: is_whitelisted_post_meta: Allow post metas beginning with attribute_ (WooCommerce Variation attributes)

[mreishus]

Proposed changes:

• When looking at [Search: Include Support for WooCommerce Variable Products], I found that the Cache Sites don't have enough information on them to generate the permalinks needed for Variation Products. The attribute meta, like if you have a blue variation of a shirt, is missing.

Without this, WooCommerce cannot make the url ending with ?attribute_color=blue. Solution: Change the is_whitelisted_post_meta() function to allow metas belonging with attribute_.

There is a corresponding change on the WPCOM side on 185272-ghe-Automattic/wpcom.

Goal

• To eventually allow Woo Product Variations to generate proper permalinks when we ES index them.

Other information:

• [ ] Have you written new tests for your changes, if applicable?
• [ ] Have you checked the E2E test CI results, and verified that your changes do not break them?
• [ ] Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

• Go to '..'

[github-actions[bot]]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the reish20250612/post-meta-attribute-sync branch.
• To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack reish20250612/post-meta-attribute-sync

bin/jetpack-downloader test jetpack-mu-wpcom-plugin reish20250612/post-meta-attribute-sync

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

[github-actions[bot]]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• :white_check_mark: Include a description of your PR changes.
  
• :white_check_mark: Add a "[Status]" label (In Progress, Needs Review, ...).
  
• :white_check_mark: Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
  
• :white_check_mark: Add testing instructions.
  
• :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  
• :white_check_mark: Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:

---

Follow this PR Review Process:

1. Ensure all required checks appearing at the bottom of this PR are passing.
2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
3. You can use GitHub's Reviewers functionality to request a review.
4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

[jp-launch-control[bot]]

Code Coverage Summary

Coverage changed in 2 files.

File	Coverage	Δ%	Δ Uncovered
projects/packages/sync/src/modules/class-woocommerce.php	0/123 (0.00%)	0.00%	12 💔
projects/packages/sync/src/modules/class-posts.php	23/247 (9.31%)	-0.08%	2 ❤️‍🩹

Full summary · PHP report · JS report

_{If appropriate, add one of these labels to override the failing coverage check: https://github.com/Automattic/jetpack/labels/Covered%20by%20non-unit%20tests https://github.com/Automattic/jetpack/labels/Coverage%20tests%20to%20be%20added%20later https://github.com/Automattic/jetpack/labels/I%20don%27t%20care%20about%20code%20coverage%20for%20this%20PR}

[darssen]

Hi @mreishus! Thanks for the proposal.

Unfortunately, I don't think that would work for all sync scenarios.

• Checksums will consider the post_meta_whitelist settings here
• Full Syncs will do similarly here

[mreishus]

Hi @mreishus! Thanks for the proposal.

Unfortunately, I don't think that would work for all sync scenarios.

* Checksums will consider the `post_meta_whitelist` settings [here](https://github.com/Automattic/jetpack/blob/trunk/projects/packages/sync/src/class-settings.php#L429)

* Full Syncs will do similarly [here](https://github.com/Automattic/jetpack/blob/trunk/projects/packages/sync/src/modules/class-posts.php#L891)

• Can you explain more about these systems?
• Does str_starts_with( $meta_key, '_wpas_skip_' ) work? Can we use the same strategy? ( 948dd72d403 )
• Do you have any suggestions on how to move forward?

[darssen]

• Can you explain more about these systems?

• Full Sync is a type of Sync that brings all the data required in WPCOM. There's more than one scenario where it's useful, but the easiest to understand would be once the site connects for the first time. We want to bring all the relevant data from the site.
• Checksums will basically do a comparison between cached data and remote data, so if there are any inconsistencies, we will try to fix them.

• Does str_starts_with( $meta_key, '_wpas_skip_' ) work? Can we use the same strategy? ( 948dd72 )

Unless I'm missing something, there is a bug with that code and these metas don't work for above.

• Do you have any suggestions on how to move forward?

Ideally, we should know the exact meta, but I'm assuming that is not an option since those are user-defined metas. We could try to add some kind of hard-coded wildcards for both full sync and checksums, but I'm worried about the impact on wildcard queries for large sites for both checksums and full syncs. This is not something that should be done without considering the performance toll.

[mdbitz]

We could try to add some kind of hard-coded wildcards for both full sync and checksums, but I'm worried about the impact on wildcard queries for large sites for both checksums and full syncs. This is not something that should be done without considering the performance toll.

This definitely is a bug with the _wpas_skip_ meta already and we need to correct it within the Full Sync and Checksum flows.

As a temporary work-around, while performance is being considered/testing, how about we add a hook so that when a product variant is saved/updated it generates and persists the url in meta. This new meta can be added to allow lists and synced?

[mreishus]

Investigating a way to get all 3 systems working using wc_get_attribute_taxonomies().

[mreishus]

wc_get_attribute_taxonomies() - won't work

I got excited, thinking I found a way to get a list of all the "attributes_", but this doesn't work - there are multiple types of attributes and this doesn't get the per-product ones.

[mreishus]

Generating permalinks on save - working but need a place to add it

As a temporary work-around, while performance is being considered/testing, how about we add a hook so that when a product variant is saved/updated it generates and persists the url in meta. This new meta can be added to allow lists and synced?

This is promising. Instead of syncing all attributes, let's write out a "permalink" meta and sync only that one, which is a non-dynamic name. This bit of code seems to work:

function jp_save_variation_permalink( $variation_id, $i = null ) {
	$variation = wc_get_product( $variation_id );
	if ( ! $variation || ! $variation->is_type( 'variation' ) ) {
		return;
	}

	$permalink = wp_make_link_relative( $variation->get_permalink() );
	update_post_meta( $variation_id, '_jp_variation_permalink', $permalink );
}
add_action( 'woocommerce_save_product_variation', 'jp_save_variation_permalink', 10, 1 );

function jp_refresh_variation_permalinks_on_parent_save( $product_id, $product ) {
	if ( 'variable' !== $product->get_type() ) {
		return;
	}
	foreach ( $product->get_children() as $variation_id ) {
		jp_save_variation_permalink( $variation_id );
	}
}
add_action( 'woocommerce_update_product', 'jp_refresh_variation_permalinks_on_parent_save', 20, 2 );

However, I don't know where to put it. Logically, it seems like it belongs to "JP Sync", but on my test site, I wasn't able to find files in /jetpack-sync/ that were running as I was editing files in Woo admin. Any suggestions on how to place this code @mdbitz @darssen ?

[mreishus]

Alt Idea - Query DB for keys

Here's one other idea, but I think setting the _jp_variation_permalink as above is better. Query the DB directly to find the keys, then whitelist them:

public function add_dynamic_attribute_post_meta_whitelist( $list ) {
	global $wpdb;

	$cache_key = 'jp_sync_wc_attribute_meta_keys';
	$keys      = wp_cache_get( $cache_key, 'jetpack' );

	if ( false === $keys ) {
		$keys = $wpdb->get_col(
			"SELECT DISTINCT meta_key
				FROM $wpdb->postmeta
				WHERE meta_key LIKE 'attribute_%'
				LIMIT 1000"
		);

		wp_cache_set( $cache_key, $keys, 'jetpack', 3 * HOUR_IN_SECONDS );
	}

	return array_merge( $list, $keys );
}

add_filter(
	'jetpack_sync_post_meta_whitelist',
	array( $this, 'add_dynamic_attribute_post_meta_whitelist' ),
	9
);

There is a key on the column, so the query shouldn't be bad.

However, I am a bit hesitant to add a sql query here, and it seems like we can't guarantee that there is a persistent object cache on the client site. So maybe not..

[darssen]

Generating permalinks on save - working but need a place to add it

...

However, I don't know where to put it. Logically, it seems like it belongs to "JP Sync", but on my test site, I wasn't able to find files in /jetpack-sync/ that were running as I was editing files in Woo admin. Any suggestions on how to place this code @mdbitz @darssen ?

If this is related to Search I'd assume it will need to live in the Search Sync Module. Not sure if it makes sense to have some check ensuring Woo is active in the Site. (Something like this)

Of course, we will also need to ensure that the _jp_variation_permalink is a synced meta from the package end which is validated in WPCOM.

Alt Idea - Query DB for keys

Here's one other idea, but I think setting the _jp_variation_permalink as above is better. Query the DB directly to find the keys, then whitelist them:

This one would be really easy from a Sync perspective, but as you said we can't guarantee that there is a persistent object cache on the client site, so I would advise against it.

[darssen]

We could try to add some kind of hard-coded wildcards for both full sync and checksums, but I'm worried about the impact on wildcard queries for large sites for both checksums and full syncs. This is not something that should be done without considering the performance toll.

This definitely is a bug with the _wpas_skip_ meta already and we need to correct it within the Full Sync and Checksum flows.

As a temporary work-around, while performance is being considered/testing, how about we add a hook so that when a product variant is saved/updated it generates and persists the url in meta. This new meta can be added to allow lists and synced?

I did create an internal task to consider fixing the bug and maybe adding the ability to have wildcards with a Setting. This creates some hurdles, especially for checksums since the way queries are prepared as of now don't consider wildcards nor having the same column (meta_key) with different filter_values.

[mreishus]

If this is related to Search I'd assume it will need to live in the Search Sync Module. Not sure if it makes sense to have some check ensuring Woo is active in the Site. (Something like this)

On my test site with jetpack, there are these files:

• ./woocommerce-payments/vendor/automattic/jetpack-sync/src/modules/class-search.php
• ./jetpack-search/jetpack_vendor/automattic/jetpack-sync/src/modules/class-search.php
• ./jetpack/jetpack_vendor/automattic/jetpack-sync/src/modules/class-search.php

, but I'm seeing that none of these files are running when I edit WooCommerce products. (if I add an error log to the files, it never triggers). So I think the ./jetpack-sync/ files are active only when sync related requests are occurring, but this hook needs to set the meta when the admin edits the WooCommerce products.

[mreishus]

For the Elastic Search side, we're looking at putting the variations into the main document, and not making them separate documents. This means we don't really need the permalink, but it would still be nice to put attribute values in the search. I put the DB call method in this PR, but I'm open to other methods. @darssen Do you have any other ideas how to approach this, or how to measure the extra # of queries done by this approach? I can push the WPCOM side changes if it helps.

[darssen]

For the Elastic Search side, we're looking at putting the variations into the main document, and not making them separate documents. This means we don't really need the permalink, but it would still be nice to put attribute values in the search. I put the DB call method in this PR, but I'm open to other methods. @darssen Do you have any other ideas how to approach this, or how to measure the extra # of queries done by this approach? I can push the WPCOM side changes if it helps.

If we don't really need them as of now. I would wait a bit since I'm trying to investigate a solution for the wpas_skip_ and understand if a wildcard whitelist would be ok that we will leverage in all Sync processes (Full Sync, Incremental and Checksums)

As per the PR, as in my previous comment for the other approach. If it is related to Search I'd assume it will need to live in the Search Sync Module. Not sure if it makes sense to have some check ensuring Woo is active in the Site. (Something like this)

Regarding how many queries. Without cache mechanisms preventing them, this will happen every time we do Settings::get_setting( 'post_meta_whitelist' ) in is_whitelisted_post_meta, which means it will happen for every added_post_meta, updated_post_meta and deleted_post_meta actions, assuming the post type is allowed.

[github-actions[bot]]

This PR has been marked as stale. This happened because:

• It has been inactive for the past 3 months.
• It hasn't been labeled `[Pri] BLOCKER`, `[Pri] High`, `[Status] Keep Open`, etc.

If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation.

If the PR is not updated (or at least commented on) in another month, it will be automatically closed.

[github-actions[bot]]

This PR has been automatically closed as it has not been updated in some time. If you want to resume work on the PR, feel free to restore the branch and reopen the PR.