yoast-acf-analysis icon indicating copy to clipboard operation
yoast-acf-analysis copied to clipboard

Reconsider decision to base replacement variables on field name

Open kraftner opened this issue 8 years ago • 2 comments

As already said in #60 I do not think that it was a smart decision to do so. Field names are not unique in ACF, so the last usage wins and overwrites everything else.

What would need to be done is replace all occurrences of field.name in replacevars.js

As this breaks functionality introduced in 2.0 I think we should first discuss how to approach this.

kraftner avatar Aug 24 '17 10:08 kraftner

Field names are unique.

It's just that when ACF nests fields it doesn't give the fields their real name ( as in the database ).

The real name of a nested field that can be repeated is: #{parent_name}_#{field_index}_#{field_name}, for example: flexible-content-field_0_text-field.

The real name of a nested field that can't be repeated is: #{parent_name}_#{field_name}, for example: group-field_text-field.

https://github.com/Yoast/yoast-acf-analysis/tree/stories/60-handle-acf-nested-fields includes a first WIP of picking up on this inconsistency in ACF and replacing the field names with their correct values.

herregroen avatar Aug 30 '17 10:08 herregroen

I've already explained a big part of the difference between ACF field names and the post meta keys ACF saves to here including the fact that neither of them are forced to be unique.

Anyway, the fact that the placeholder variables are based on custom fields is a Yoast SEO feature that we can't do anything about here.

What remains is the fact that with ACF it is possible to define multiple fields that all write to the same post meta overwriting each other. As Yoast SEO only gets one post meta we should probably try to somehow determine which value survives (the last?) and use that, otherwise the preview might show something else than what is actually used.

But as this really is an edge case I'll put it on the backlog.

kraftner avatar Sep 20 '17 13:09 kraftner