Ensure no errors on CPTs without Yoast SEO metabox #103
Conversation
| @@ -223,4 227,20 @@ protected function get_blacklist_type() { | |||
| protected function get_blacklist_name() { | |||
| return new Yoast_ACF_Analysis_String_Store(); | |||
| } | |||
|
|
|||
| /** | |||
| * Adds a CPT for tests | |||
There was a problem hiding this comment.
Can you make this line more clear. I know where CPT stands for, but somebody who's new to the codebase probably won't know.
|
|
||
|
|
||
| /** | ||
| * @return bool |
There was a problem hiding this comment.
It would be nice to have some documentation here.
| @@ -100,4 100,42 @@ | |||
| ), | |||
| 'menu_order' => 0, | |||
| )); | |||
|
|
|||
| register_field_group(array ( | |||
There was a problem hiding this comment.
Can you add a space before array
There was a problem hiding this comment.
As this is coming out of the ACF Export function like this I'd rather leave it like this as it makes it easy to just C&P stuff on changes and having clean diffs. What do you think about that?
| @@ -336,4 336,50 @@ | |||
| 'description' => '', | |||
| )); | |||
|
|
|||
| acf_add_local_field_group(array ( | |||
There was a problem hiding this comment.
Can you add a space before array
|
CR Done 👍 |
|
I'm having an issue with the testing code being inside the plugin, but I understand that the total approach needs to be revised in order to change this. As we don't want to ship the |
|
I absolutely agree with that. This was kind of ok while it was just one simple thing. But as this grows we'll need to get this out at some point. |
|
See #111 |
|
I think the question here and now is if we get this out and clean up later or put this on hold until we have #111 sorted out. |
|
Let's add this functionality first and remove the testing from the plugin afterwards. |
|
I think it would be better the other way around. Because I'd need to fix the conflicts here, merge this, which then in turn will cause conflicts in #117 plus the need to rework everything in this PR anyway. Sounds like a lot of wasted effort. Is there any reason you see with #117 why it can't be merged before? |
Conflicts: inc/class-ac-yoast-seo-acf-content-analysis.php
|
Updated PR based on changed test data loading going into |
|
As #117 has been merged, this can be implemented. |
|
Could you also merge |
fixes #88