Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional Filter-Hooks #3

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

piscis
Copy link

@piscis piscis commented Mar 16, 2017

Hello Angrycreative Team,

thanks for the great work on this plugin! We would like to have additional filter hooks in the plugin.

The reason we need this is because some of the output of our ACF fields depend on conditional logic within ACF and will affect the keyword density calculation as well as keyword suggestions.

Therefor we introduced two new filter one is for modifying the ACF field values and the other one is to control the final output that get's send back to the Yoast SEO backend.

What do you think?

Thanks,

Alex

Copy link

@pelmered pelmered left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agree that more filters are necessary. We have some comments on the implementation. See the inline comments.

Also please update the the readme accordingly.

foreach($fields as $key =>$item) {

if(in_array((string)$key, $this->get_excluded_fields()) ){
continue;
} else {


$item = apply_filters('ysacf_field_overwrite', $key, $item, $pid);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter order here is wrong. You should always filter the first parameter in an apply_filters call. I.e. $key and $item should switch place. This makes also makes the filter on line 70 unnecessary.

I also think it would be better for both performance and usability to filter all the fields at once before the loop instead of filtering every field individually.
Would it work for your use case if you put the filter in the ajax_get_fields method and filter the $fields variable?

@@ -66,6 +66,15 @@ function __construct(){
add_action( 'admin_print_scripts-post-new.php', array($this, 'enqueue_admin_scripts') , 999 );
add_action( 'admin_print_scripts-post.php', array($this, 'enqueue_admin_scripts'), 999 );
add_action( 'wp_ajax_' . $this->plugin_slug . '_get_fields', array($this, 'ajax_get_fields') );

add_filter('ysacf_field_overwrite', function($key, $item, $postID) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not make sense. See comment on line 106.

return $item;
}, 1, 3);

add_filter('ysacf_finalize', function($data, $postID) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This filter hook does nothing and can be removed.

composer.json Outdated
@@ -0,0 +1,8 @@
{
"name": "all3dp/Yoast-SEO-ACF-Content-Analysis",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Angrycreative/Yoast-SEO-ACF-Content-Analysis

Update package name for compatibility reason
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants