Clashing keys when using the `bbl_translated_meta_fields` filter

[simonwheatley]

There is an API for specifying what meta keys can be translated, and what UI the translation requires. So you can take a plugin with a whole bunch of meta fields and say: "this meta key is translated using an RTE, this one using a text field, and this one with a text area" (unspecified keys don’t need translation so the values remain the same across all translations (e.g. for a map reference, or a colour)).

The API allows you to filter an array for a post, with each index on the array being a meta key.

Here’s the filter: https://github.com/Automattic/babble/blob/49a5140dbb8caf56e932c41f66674b6e9aebbb85/class-jobs.php#L1227

Here’s the example plugin: https://github.com/Automattic/babble/blob/master/translation-fields.php

I am not sure we have considered that you could have two post types which use the same meta key for completely different purposes, and I’m wondering if we should switch to dynamic filter names, which include the post type name in the filter; e.g.

$fields = (array) apply_filters( 'bbl_translated_meta_fields-' . $post->post_type, array(), $post );

This way a developer has to pick the filter name corresponding to the required post type, rather than expecting them to check the second WP_Post parameter, which they probably won’t… and then when there’s a meta key clash :boom: disaster.

[johnbillion]

As you mention, the $post parameter is passed for context so it is possible to avoid a clash as long as it's used, but there's no reason to expect a developer to consider a post meta key clash so maybe we can improve this.

I'm not usually a fan of dynamic filter names, but it's probably appropriate here.

[simonwheatley]

Now I'm looking, we have the bbl_sync_meta_key filter, which also takes no account of the post_type with similar potential issues. Perhaps we need to deprecate that, and add a similar dynamic filter which accounts for post_type… that could look something like:

$post = get_post( $post_id );
// Some metadata shouldn't be synced
$block_meta_key_globally = ! apply_filters( 'bbl_sync_meta_key', true, $meta_key );
$block_meta_key_for_type = ! apply_filters( 'bbl_sync_meta_key' . $post->post_type, true, $meta_key );
if ( $block_meta_key_globally || $block_meta_key_by_type ) {
    return;
}

Alternatively we could just introduce a breaking change, remove the bbl_sync_meta_key filter and replace it with a dynamically named filter, to simplify the logic and remove the possibility of clashes.

Sidenote: there's an argument to say we should abstract all these filter calls into a method.