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

ACF Post Object filter: guard against empty post id #407

Closed
wants to merge 2 commits into from

Conversation

jacobarriola
Copy link
Contributor

What does this implement/fix? Explain your changes.

Adds a guard against the post id when attempting to set the correct Type source for an ACF Post Object field. This guard will return the correct value (null) instead of an unexpected query result (incorrect/duplicate product/order/coupon).

There could be instances where the post id is empty because an end-user saved an empty value in the WP admin UI. It is better to get a null value instead of the incorrect product/order/coupon result.

For more details, see #406

Does this close any currently open issues?

#406

This guard prevents unexpected query results. There could be instances where the post id isn't available because an end-user saved an empty value in the WP admin UI. It is better to get a null value instead of the incorrect product/order/coupon result.

Related to wp-graphql#406
@jacobarriola
Copy link
Contributor Author

Hey @kidunot89 👋🏽

I'm going to be stepping away during the holidays soon, so let me know whether this PR seems acceptable to you or whether I need to make some changes. Thanks!

Copy link
Member

@kidunot89 kidunot89 left a comment

Choose a reason for hiding this comment

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

@jacobarriola Sorry, for the delay on this review. Great work on this 👍

Copy link
Member

@kidunot89 kidunot89 left a comment

Choose a reason for hiding this comment

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

@jacobarriola Upon further review, this function may not be necessary anymore.

I suggest changing Line 26 to add_filter( 'graphql_acf_post_object_source', array( '\WPGraphQL\WooCommerce\Data\Loader\WC_CPT_Loader', 'resolve_model' ), 10, 2 );

@jacobarriola
Copy link
Contributor Author

@kidunot89 Sounds good. I'll take look and test it out.

@kidunot89
Copy link
Member

@jacobarriola There is however an core design issue that is preventing Products and other WPGraphQL Costs from being registered to WPGraphQL core PostObjectUnion type which is typically the type resolved for the ACF post_object relationship fields. After much discussion with @jasonbahl we still haven't fully resolved this. There's a lot of moving pieces in play on this.

@jacobarriola
Copy link
Contributor Author

@kidunot89 With regards to the design issue, is this the issue you're referring to? wp-graphql/wp-graphql-acf#177

@kidunot89
Copy link
Member

@jacobarriola This shouldn't be necessary after the next release. Even less so have the next major release of WPGraphQL ACF.

@kidunot89 kidunot89 closed this Dec 7, 2022
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