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 acf fields from relationships #95

Closed
wants to merge 4 commits into from

Conversation

rainboxx
Copy link

This feature was quickly discussed in #94.

Idea: Expose acf fields from realtionships to the REST API as well.

Solution: Check for relationships in get_fields and fetch acf fields from all of them. This might be a performance issue for bigger objects, so it could be enabled via an option. The object is added to the relationship object with either the default key acf or the one set for the plugin.

Enhancement ideas:

  • Add option for enabling fetching related acf fields which defaults to false in order to not break current API (while it shouldn't, because it's an additional field) as well as expected performance.
  • I only use the free version of acf for now so I don't know whether this change will conflict with some other field that exposes an array. The enhancement could be to check for an ID key (and maybe more?)
  • Recursively call the protected get_fields. Might be possible but I don't know much of WP internals so I might need help here.

@airesvsg
Copy link
Owner

Hi @rainboxx,
Thanks for your PR.
Please, use the PHP coding standards of WordPress and create the filter acf/rest_api/{type}/get_fields/recursive.

PHP coding standards:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/

Filter:

if ( apply_filters( 'acf/rest_api/' . $this->type . '/get_fields/recursive', false, $request, $response, $object ) ) {
// your code here
}

@rainboxx
Copy link
Author

Hi @airesvsg, I'm happy to make the mentioned changes.

I tried to be consistent with the coding style of the file. Can you point out what exactly you mean within my PR? It'll be hard to see what was intentionally and what not on this file (eg. spaces around PHP-native functions are missing, so I left them out as well). Code review comments would be awesome 😊.

About the filter: the condition should be wrapped around the recursive fetching of the acf fields? If so, I put the "old" code in an else block? How would I activate this filter so that my code is being run?

@rainboxx
Copy link
Author

Hm sorry, I was mixing some styles because the loops were initially place at the ACF plugin directly for testing purposes. I fixed some line breaks and whitespaces. If something is still missing, please point out the location. Thanks 😊.

@airesvsg
Copy link
Owner

Now, you need to create a new method and use them recursively and in your current code, you need to pass the values by reference ( &$rel_array ).

@rainboxx
Copy link
Author

rainboxx commented Jan 2, 2017

Not sure whether I got what you meant. I added a new commit that calls fetching AC fields from relations recursive.

Still not clear what to do with the proposed filter, though.

Can you check and maybe elaborate a bit more?

Thanks and a happy new year!

@airesvsg
Copy link
Owner

airesvsg commented Jan 2, 2017

Hi @rainboxx,
Thanks for your time and a happy new year too!
You need to improve your code, because if you use the fields types ( relationship or taxonomy ) the values are numeric and not object.

How to implement the filter:

if ( apply_filters( 'acf/rest_api/' . $this->type . '/get_fields/recursive', false, $request, $response, $object ) ) {
	$this->add_relation_fields( $acf_data, $field );
}

Thanks

@rainboxx
Copy link
Author

rainboxx commented Jan 2, 2017

@airesvsg, I added the filter. It works but I don't know why 😉.

@@ -229,6 +236,24 @@ protected function get_fields( $request, $response = null, $object = null ) {
return apply_filters( 'acf/rest_api/' . $this->type . '/get_fields', $data, $request, $response, $object );
}

protected function add_relation_fields( &$acf_data, $field ) {
foreach ( $acf_data as $k => $rel_array ) {
Copy link
Author

Choose a reason for hiding this comment

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

@airesvsg, do you mean I have to take care here? Taking care = checking $acf_data for being an object?

Copy link
Owner

Choose a reason for hiding this comment

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

Nope. You must have check the $rel_array variable.
When I said: "you need to pass the values by reference" in this case also I refer the $rel_array. #95 (comment)
I prefer that the method add_relation_fields returns the data.
Thanks

Copy link
Author

Choose a reason for hiding this comment

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

I understood the comment with the reference but wanted to avoid having a foreach outside and inside the function but I can change that of course.

@rainboxx
Copy link
Author

rainboxx commented Jan 2, 2017

In case $rel_array is not an array, the value will currently not be transformed. What should be happen if $rel_array is numeric? How can I distinguish between a plain number field and such an "numeric" field?

@airesvsg airesvsg closed this Jan 11, 2017
@rainboxx
Copy link
Author

Hey @airesvsg, I'll still provide a change here. Internally we had a priority change and upgraded to the Pro v5 version, which provides us with enough possibilities for getting started. But unfortunately this PR here doesn't work at all with the Pro v5 version so I have to investigate a bit more. I'll update you once I have something.

@dmnsgn dmnsgn mentioned this pull request Apr 3, 2018
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.

None yet

2 participants