-
Notifications
You must be signed in to change notification settings - Fork 36
Get the formatted_value
from the original field value
#211
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
Why will this fix your fatal error on the plugin? The change, as far as I can see, is just changing a variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for approving the pull request.
Here is my explanation.
The original code from SCF:
// Get raw value.
$value = acf_get_value( $post_id, $field );
// First format for rest API for backward compatibility.
$value = acf_format_value_for_rest( $value, $post_id, $field, $format );
// We keep this one for backward compatibility with existing code that expects the field value to be.
$fields[ $field['name'] ] = $value;
$fields[ $field['name'] . '_source' ] = array(
'label' => $field['label'],
'type' => $field['type'],
// This line re-formats the already formatted REST value instead of the raw value, which causes issues.
'formatted_value' => acf_format_value( $value, $post_id, $field ),
);
I fixed it by renaming the formatted REST value to $rest_value and leaving the original $value untouched. This way, when calling acf_format_value() to populate the formatted_value property of _source, it’s based on the raw value, not on the already formatted value:
// Format value for rest API for backward compatibility.
$rest_value = acf_format_value_for_rest( $value, $post_id, $field, $format );
// We keep this one for backward compatibility with existing code that expects the field value to be.
$fields[ $field['name'] ] = $rest_value;
$fields[ $field['name'] . '_source' ] = array(
'label' => $field['label'],
'type' => $field['type'],
'formatted_value' => acf_format_value( $value, $post_id, $field ),
);
In my plugin (Meta Field Block), I used the 'acf/rest/format_value_for_rest' filter to change the format of the REST value to support displaying all field types (even Group, Repeater, Flexible, Relationship, etc). When SCF re-format some multi-value field types like select, it treats the altered REST value as multiple values, which is why the error occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Mr2P, thanks again for engaging here 🙂
After a thorough read of the full method, I agree that the new variable name is helping not to overwrite the existing
$value
variable, great! My only concern is that I am not sure about the side effects this effect can have for other users; would you mind adding some tests to ensure backwards compatibility?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @priethor,
I’m not sure what needs to be tested for this fix. The root cause of the issue is that, starting with SCF 6.5.2, the wrong value has been assigned to the new formatted_value property.
It should be:
$raw_value = acf_get_value( $post_id, $field );
$fields[ $field['name'] . '_source' ] = array(
'formatted_value' => acf_format_value( $raw_value, $post_id, $field ),
);
not:
$raw_value = acf_get_value( $post_id, $field );
$formatted_value_for_rest = acf_format_value_for_rest( $raw_value, $post_id, $field, $format );
$fields[ $field['name'] . '_source' ] = array(
'formatted_value' => acf_format_value( $formatted_value_for_rest, $post_id, $field ),
);
The value returned by acf_format_value_for_rest is filtered by:
apply_filters( 'acf/rest/format_value_for_rest', $value_formatted, $post_id, $field, $value, $format );
If any third-party plugin uses this filter to change the REST value, the formatted_value property may end up with the wrong value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! From a logical point of view, it makes sense to me to run the formatting on the raw value instead of the already-formatted-for-REST value, especially considering you can hook into the latter.
I'm going to go ahead and proceed, but I'd still love it if @cbravobernal of @ockham could verify that there are no side effects in block bindings, as the
formatted_value
property, even if introduced in 6.5.2, is used there.Thanks for bearing with us, @Mr2P, and please keep the feedback and testing coming ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priethor Thanks for the approving. I'll keep testing on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no side effects on block bindings. As far as I tested. Let's merge it.