Yoast / yoast-acf-analysis

WordPress plugin that adds the content of all ACF fields to the Yoast SEO score analysis.
https://wordpress.org/plugins/acf-content-analysis-for-yoast-seo/
GNU General Public License v3.0
61 stars 20 forks source link

Reconsider decision to base replacement variables on field name #62

Open kraftner opened 7 years ago

kraftner commented 7 years ago

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.

herregroen commented 7 years ago

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.

kraftner commented 7 years ago

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.