improve cache keys for acf-value-functions.php

[rburgst]

• this avoids having groups with the same field names from clashing
• in the case where you query via wp-graphql-acf and
  you have multiple field groups with repeaters that resolve to the
  same field name, then without this patch the first field group
  will load the field value and store it inside the cache
  with the wrong sub-field ids.
• for more info see
  Acf fields return null with repeater/group wp-graphql/wp-graphql-acf#170

[elliotcondon]

Hi @rburgst

Thanks for submitting this PR.

It's quite interesting, but suggests that it be possible to register multiple fields with the same name on a single object.
This goes against the grain of WP meta tables, so I'm interested to better understand your use-case.

In theory, a single post should only be able to contain 1 meta value for "featured_image", not multiple.

[rburgst]

Hi, the problem is explained in wp-graphql/wp-graphql-acf#170. When querying the data via graphql the default location rules do not properly apply. Therefore it is possible that you can query multiple field groups on the same page even if in ACF they do not really apply (however, you can always obviously map 2 field groups to the same page which achieves a similar problem).
In the graphql case, the very first field that is queried will yield the results stored in the ACF (in case the name of the fields match). The cache entry will then map the loaded field values with the corresponding field ids (in my case the first field was the "wrong" field and therefore the wrong sub-field ids got cached). When you then get to the point where the 2nd ACF field group (the "correct one") is read, the cache will already contain an entry containing the wrong sub-field ids and therefore nothing will be returned since the sub fields do not match.
IMHO this change will not break anything but fix the somewhat specific case for wp-graphql-acf.

[elliotcondon]

Thanks @rburgst

This explanation really helped to better understand the issue. You are right to identify this as a problem within acf_get_value() and I wonder if we should reconsider using the Field's name here. Instead, it might make more sense to use the Field's key.

If you change line 63 from $field_name = $field['name']; to $field_name = $field['key'];, does this solve the issue completely?

I'll need to add this to our dev to-do list, and make sure to run this through A LOT of testing, but am very open to making the change 👍.

[elliotcondon]

Hi @rburgst

Upon further review, I can confirm the suggestion made in my previous comment would prove problematic for sub field values. Please disregard it 🙃.

The solution proposed in your PR does appear to be the best way to avoid this issue. In a lot of ways, the addition of the Field's key makes sense, as we are not only loading the Post's meta value (field name), but we are also modifying it based on the field (field key). The combination of field name + field key creates a more specific cache key for that desired instance.

I'll need to consider how this cache key change would effect the plugin from a larger point of view, and will mark this tentatively for the 5.10 release.

[rburgst]

@elliotcondon can you provide an ETA?

[elliotcondon]

Hi @rburgst. No update just yet.

Did you ever receive a response from the "wp-graphql" devs on the bug? This issue should be easily avoidable by better checking which fields are mapped to which objects. At the moment, this is a simple process of checking for reference values (which are saved alongside the meta value).

[rburgst]

I fear a wp-graphql-acf fix wont happen any time soon. wp-graphql/wp-graphql-acf#135

[rburgst]

any news here?

[wbrauneis]

Important patch for us. Any ETA on this issue so that we no longer need to patch new versions.

[devoiutd]

Error still happen on:
Wordpress 5.7
WP Gatsby 1.0.8
WP GraphQL 1.3.4
WPGraphQL for ACF 0.3.4

Can you merge as soon as possible?

[rburgstaller]

@elliotcondon I checked with wp-graphql 0.5.0 and the problem persists, so this patch is still relevant and needed.

[cromosom]

@elliotcondon
any ETA here? We are looking forward to see this patched anytime soon.