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

New update breaks flexible content ACF #60

Closed bhuisman1992 closed 6 years ago

bhuisman1992 commented 7 years ago

Hi there,

After the update to 2.0 most functionality on my flexible content pages stopt working.

For example: "I have a flexible content group with content blocks. The first field is a select field which gives you the ability to select a specific block. But after the update it won't view the choosen block." And i can only add two flexible blocks instead of unlimited blocks. Besides that it's not possible to delete any flexible blocks.

I've turned on WP_debug but it doesn't give me any alerts/errors.

I know the update is causing this problem because when i deactivate it.. the flexible content works as it should. Currently i've rolled back the version.. but it would be nice if this issue can be fixed.

My website is running on PHP 7.1

Greetz and many thx.

kraftner commented 7 years ago

Are you using ACF 5 or ACF 4 with the Flexible Content Field add on? (The latter is not supported) Do you see any JavaScript Errors when you open the console? (Press F12 and select Console)

bhuisman1992 commented 7 years ago

Sorry for the lack of information. I am using ACF 5 PRO (latest version). And yes i can see the errors in the console:

TypeError: replaceVars[field.name] is undefined

schermafdruk 2017-08-23 14 55 23

kraftner commented 7 years ago

Okay that brings us closer. :wink: Can you give me the field definition of your Flexible Content? I am currently having a hard time replicating this here.

bhuisman1992 commented 7 years ago

Ehmmm don't know if this is what you mean:

I have a select field with in total 3 options: schermafdruk 2017-08-23 15 05 43

Based on those options i enable a WYSIWYG editor or a WYSIWYG editor + img field (conditional logic)

schermafdruk 2017-08-23 15 08 18

If there is a way to share my information (private) so you can check it.. i can do that!

kraftner commented 7 years ago

Ah, maybe it has to do with the conditional logic. I'll let you know if I need more info.

kraftner commented 7 years ago

Okay I can reproduce this now. It seems as if it is an incompatibility with Yoast SEO 5.3 that was released simultaneously with 2.0 of this plugin.

bhuisman1992 commented 7 years ago

Ok thx for checking it out.. what should i/we do next?

kraftner commented 7 years ago

This is a bug introduced by #34.

The reason is that #34 assumes that all fields already exist from the very beginning which is not true for e.g. Flexible Content as we see here. As a result when doing the replacement this fails because it is looking for a field that doesn't have Replace Vars prepared.

The options to go from here:

  1. Do not pre-calculate the Replace Vars. This is what the collector does.
  2. Do pre-calculate but add missing fields on-demand inside updateReplaceVars().

I know nothing about the API of the ReplaceVars plugin, maybe someone more involved has an opinion on how to approach. @herregroen or @moorscode maybe?

Some things that should probably also be approached when already in that file as they were overlooked in the initial implementation:

kraftner commented 7 years ago

@bhuisman1992 Until this is fixed you have two options as a quick fix:

  1. Downgrade Yoast SEO to 5.2
  2. Downgrade ACF Content Analysis for Yoast SEO to 1.2.1

If you are a developer you might get your hands dirty and try filing a PR based on what I explained above.

viktorfroberg commented 7 years ago

Same issue here

valentinchevalier commented 7 years ago

I had the same issue. Downgrade to ACF Content Analysis for Yoast version 1.2.1 fix it.

herregroen commented 7 years ago

@kraftner

Looking at the issue now.

Field names are actually unique. It's just that the field name of fields in a layout isn't actually what ACF says it it is. The actual field name of a field in a layout is #{layout_name}_#{layout_index}_#{field_name} but ACF reports it as simply #{field_name}.

fieldData and replaceVars were indeed errors, fixing them now.

Nightwatch tests would indeed be good, but will probably take more time.

For now I'm focussing on how to find out in if a field is in a layout so that we can either properly support them or simply ignore them.

I hope to have a PR ready either today or tomorrow, this probably won't include tests at first.

herregroen commented 7 years ago

@kraftner

I've got a WIP branch here: https://github.com/Yoast/yoast-acf-analysis/tree/stories/60-handle-acf-nested-fields.

This seems to fix the issue and correctly name the nested fields which also means they can be used and will be picked up in the frontend.

Code's a bit ugly though and it has no additional tests for the issue so could use improvements. I hope to be able to spend some more time on the issue tomorrow.

kraftner commented 7 years ago

Let me start by saying sorry I didn't make it more explicit here that I already added a PR for this in #63.

I think we also have a misunderstanding here:

When you are talking about field names your are talking about the name of the post meta field that is saved in the DB. What I am talking about here is the field name as returned by the ACF JS API with calls to acf.get_fields() and acf.get_data(). This is also what is used throughout this plugin, e.g. for the blacklist_name filter.

As everything else in this plugin works with those names, they are the "official" names as returned by the ACF JS API as well as the names being displayed in the UI of ACF when editing fields I think we should not be altering those internally.

No matter if we are talking about ACF field names or post meta fields they definitely aren't unique. Try creating two fields with the same name in ACF and use them on the same post. The last one will overwrite all previous ones as they save to the same post meta field. Only keys are unique. This is a ACF flaw/bug and probably won't ever be fixed because it would require changes to the basic architecture of ACF.

Finally I think as we now need to create Replace Vars on the fly anyway I do not see any reason to pre-calculate them at all by duplicating logic in createReplaceVars() and updateReplaceVars(). Let them just be created on first usage.

So, as I said, sorry I didn't make it more obvious that I already have a working PR for this (that also includes tests). Maybe you can just give #63 a test run.

kraftner commented 7 years ago

Thanks @herregroen for your PR. I now realised that the reason you wanted to change the field names to the names that are used as post meta keys is due to the way the replace vars work in Yoast SEO. This is of course a valid point, but I addressed it by introducing another internal field property. As already explained above we shouldn't alter the ACF fields names as it would affect other features like the blacklist.

The PR should now also properly cover this.

FerryKobus commented 7 years ago

Same! +1 for fix

bhuisman1992 commented 7 years ago

Hi guys, i'm new to this.. where can i download the fix?

moorscode commented 6 years ago

Hi @bhuisman1992,

We will be releasing a new version of the plugin shortly, which will contain the fix.

nickgardnermedia commented 6 years ago

Makes me happy that I was having this same issue and then see that a new release is on the way. @moorscode any idea how shortly shortly is? Just curious I am implementing a workaround in the meantime. Appreciate your efforts this plugin is great.