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

Setup postdata on field levels #714

Merged
merged 4 commits into from Mar 11, 2019

Conversation

Projects
None yet
3 participants
@epeli
Copy link
Contributor

commented Mar 6, 2019

We noticed that the "Excerpt is always of the first post" (#503) issue has regressed and it was not caught by tests because the test added in #625 does not properly test the usage of the global post.

It does not use it because the get_the_excerpt filter uses the wp_trim_excerpt function by default and if a pre-existing excerpt is passed it just returns it:

https://github.com/WordPress/WordPress/blob/cddee8b72e59c9968b1c786c01992dae6f71ee13/wp-includes/formatting.php#L3685-L3686

Setting the post_excerpt string to an empty string causes wp_trim_excerpt to call get_the_content( '' ); and thus the test fails.

I don't know how this should be really fixed but adding this to the excerpt resolver makes it pass again:

epeli@d174a6c

but I'm pretty sure it's not the right solution so I didn't include it in this PR.

UPDATE: I have now added a proper fix to this PR

Where has this been tested?

The latest dev branch.

Improve global post test
The global post is not used if the post_excerpt is already set.
Change it to empty string to force post_excerpt generation via
the global post.

@epeli epeli changed the title Improve global post test Fix regression with "excerpt is always of the first post" Mar 6, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 6, 2019

Codecov Report

Merging #714 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #714      +/-   ##
===========================================
+ Coverage    60.12%   60.14%   +0.01%     
===========================================
  Files          110      110              
  Lines         6741     6744       +3     
===========================================
+ Hits          4053     4056       +3     
  Misses        2688     2688
Impacted Files Coverage Δ
src/Data/DataSource.php 83.4% <ø> (-0.15%) ⬇️
src/Request.php 63.51% <100%> (+0.49%) ⬆️
src/Utils/InstrumentSchema.php 94.82% <100%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7750d33...3569055. Read the comment docs.

@epeli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

I have now added a fixing commit to this PR too. It ensures that the global post is set for all field resolvers.

@epeli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Did some more digging and found setup_postdata call from the resolve_post_object

/**
* Set the resolving post to the global $post. That way any filters that
* might be applied when resolving fields can rely on global post and
* post data being set up.
*/
$GLOBALS['post'] = $post_object;
setup_postdata( $post_object );

I'm pretty sure this is too early to call it because during the testPostExcerptsAreDifferent test it is called immediately for both test posts before the excerpt resolvers are called so it makes sense it returns wrong values because the last one "gets stuck" there for all resolver calls.

In the InstrumentSchema's resolveFn it is always setup up right before each resolver is called. There might be room for some optimization here but this seems ensure the right behaviour.

So I've added an commit that removes setup_postdata call from the resolve_post_object.

@epeli epeli force-pushed the epeli:global-post-test branch from 8726e84 to 624259e Mar 7, 2019

@epeli epeli force-pushed the epeli:global-post-test branch from 624259e to 3d5a330 Mar 7, 2019

@epeli epeli changed the title Fix regression with "excerpt is always of the first post" Setup and reset postdata on field levels Mar 7, 2019

@epeli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

So yeah... I basically rewrote this PR. We found more bugs when do_graphql_request is called within the loop because it does not reset the global query properly. I improved the existing tests to cover that case and removed one extraneous test that didn't make much sense (explained it in the commit message).

So this PR now rewrites how WPGraphQL calls setup_postdata. It's now done on the field level.

@epeli epeli force-pushed the epeli:global-post-test branch 2 times, most recently from 234d116 to f95a855 Mar 7, 2019

@epeli epeli changed the title Setup and reset postdata on field levels Setup postdata on field levels Mar 7, 2019

@CodeProKid
Copy link
Member

left a comment

@epeli a few questions surrounding tests here.

];
$post_2_args = [
'post_content' => 'Post content 2',
'post_excerpt' => 'Post excerpt 2',
'post_excerpt' => '',

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Mar 7, 2019

Member

I feel like we should leave the explicitly set post_excerpt arg here so we don't get some false positives on this working when it's actually not. The best way to test the global $post data is being setup correctly is by testing the content & excerpt data is actually different. With this change the excerpts are the same for both of the posts we're querying.

This comment has been minimized.

Copy link
@epeli

epeli Mar 7, 2019

Author Contributor

What I found was exactly the opposite. This test was passing when it should have failed. When excerpt has a value it does not need to generate the excerpt as it can return the value directly. In that case the get_the_excerpt filter here:

$excerpt = apply_filters( 'get_the_excerpt', $excerpt, $post );

does not use the global post for anything because the default filter in WP uses the wp_trim_excerpt function:

https://github.com/WordPress/WordPress/blob/d9a1f99d9ce352a0e151169c3612b00c4c146ddf/wp-includes/default-filters.php#L185

And the wp_trim_excerpt function checks for empty string here and bails out if there is something already:

https://github.com/WordPress/WordPress/blob/cddee8b72e59c9968b1c786c01992dae6f71ee13/wp-includes/formatting.php#L3685-L3712

This comment has been minimized.

Copy link
@epeli

epeli Mar 7, 2019

Author Contributor

But we could probably write a completely new and better test for this situation.

For example we could just assert that when querying multiple posts the global post is set to it inside the graphql_resolve_field filter.

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Mar 7, 2019

Member

Ahhhhh, I see. So in this test, the post_excerpt should be the same as the post_content for each, shouldn't it?

This comment has been minimized.

Copy link
@epeli

epeli Mar 7, 2019

Author Contributor

Yeah, if the excerpt field is empty it will be generated from the content field on demand so it should be always different in this test.

$this->create_example_request()->execute();
$this->assertEquals( 'testing', $GLOBALS['post'] );
}

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Mar 7, 2019

Member

Seems like it would be good to keep this test? Why delete it?

This comment has been minimized.

Copy link
@epeli

epeli Mar 7, 2019

Author Contributor

I originally removed it because it wasn't working with my implementation because it does exercise a weird use case: Setting global post to a string but not calling setup_postdata which doesn't make any sense?

The current implementation in this PR passes it now but due to its weirdness and because it seems that testPostQueryPostDataReset already tests this situation well enough I decided to leave it removed.

public function testPostQueryPostDataReset() {

But if you think there's some merit to it I'll restore it.

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Mar 11, 2019

Collaborator

@epeli Ya, the use for this test was to make sure whatever is set as the Global post would be the same after a GraphQL request was made.

Since you can use a GraphQL Query within the context of a post, such as within a Shortcode or whatever, we want to make sure that whatever the Global post is before GraphQL execution is the same as it is after GraphQL execution. In this case, we set the global to testing and assert that it's the same after GraphQL executes.

I believe at one point we weren't properly resetting, so if you used GraphQL within a shortcode, anything on the page after that (such as related posts after the conent, etc) would think the Global Post was the last item resolved by GraphQL.

This test should prevent that regression.

This comment has been minimized.

Copy link
@epeli

epeli Mar 11, 2019

Author Contributor

Fair enough. The test will indeed ensure wp-graphql stays resilient even if the user does something wonky with the global post. I'll restore it now.

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Mar 11, 2019

Collaborator

ya, we don't really care what the global post was set to before a GraphQL request, we just want to ensure it's the same before and after. 😄 Thanks for fixing!

@jasonbahl
Copy link
Collaborator

left a comment

@epeli PR looks pretty good. I ended at a similar spot locally as well.

I left one note about the test that was removed. If we can add that back, unless it's actually problematic, that would be 👌

$this->create_example_request()->execute();
$this->assertEquals( 'testing', $GLOBALS['post'] );
}

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Mar 11, 2019

Collaborator

@epeli Ya, the use for this test was to make sure whatever is set as the Global post would be the same after a GraphQL request was made.

Since you can use a GraphQL Query within the context of a post, such as within a Shortcode or whatever, we want to make sure that whatever the Global post is before GraphQL execution is the same as it is after GraphQL execution. In this case, we set the global to testing and assert that it's the same after GraphQL executes.

I believe at one point we weren't properly resetting, so if you used GraphQL within a shortcode, anything on the page after that (such as related posts after the conent, etc) would think the Global Post was the last item resolved by GraphQL.

This test should prevent that regression.

epeli added some commits Mar 7, 2019

Setup postdata on field levels
Setting it up in resolve_post_object is too late because it fires before
the resolvers as resolvers can be called with different posts.

This also ensures that the global post is reset correctly after the
request.

@epeli epeli force-pushed the epeli:global-post-test branch from 2684b2d to 3569055 Mar 11, 2019

@jasonbahl jasonbahl merged commit b1caf66 into wp-graphql:develop Mar 11, 2019

3 checks passed

codecov/patch 100% of diff hit (target 60.12%)
Details
codecov/project 60.14% (+0.01%) compared to 7750d33
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasonbahl jasonbahl added this to the 0.2.3 milestone Mar 18, 2019

This was referenced Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.