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

fix: handle 404s in NodeResolver.php #2752

Merged

Conversation

justlevine
Copy link
Collaborator

@justlevine justlevine commented Mar 6, 2023

What does this implement/fix? Explain your changes.

This PR fixes a bug where NodeResolver::resolve_uri() falls back to the home page when trying to resolve a non-existent URI.

This issue is due to our refactor of resolve_uri() to use WP_Query (#2680), which didnt take into account that our existing parse_request() shim deviated from WP::parse_request().

With the PR, parse_request() now correctly sets the $query_vars['error'] value in line with WP core. Additionally, queries directly to the home page ('/'), are resolved early. This increases performance, but more importantly addresses the fact that most WP sites dont have an explicit redirect rule for the home page (which causes $query_vars['error'] to be set for those requests as well.

To Test.

  1. Create a post and a page.
  2. (Optional) Set your permalink structure to something custom e.g. /posts/%postname%/.
  3. Run the following query (change the URIs to what you created.
fragment NodeData on Node {
  __typename
  id
  ... on UniformResourceIdentifiable {
    uri
  }
  ... on ContentType {
    name
  }
  ... on NodeWithTitle{
    title
  }
}
query {
  firstNonExistingPage: nodeByUri(uri:"/fake-page") {
    ...NodeData
  }
	existingPage: nodeByUri(uri:"/home-page") {
    ...NodeData
  }
  secondNonExistingPage: nodeByUri(uri:"/another-fake-page") {
    ...NodeData
  }
}

Does this close any currently open issues?

Fixes #2751

Any relevant logs, error output, GraphiQL screenshots, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
image

Any other comments?


Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.0.26)

WordPress Version: 6.1.1

@justlevine justlevine changed the base branch from develop to release/v1.14.0 March 7, 2023 00:17
@justlevine justlevine force-pushed the fix/handle-404s-in-node-resolver branch from 4e5b4dc to c443643 Compare March 7, 2023 00:24
@justlevine justlevine force-pushed the fix/handle-404s-in-node-resolver branch from c443643 to 162a569 Compare March 7, 2023 00:28
@justlevine justlevine changed the base branch from release/v1.14.0 to develop March 7, 2023 00:28
@justlevine justlevine closed this Mar 7, 2023
@justlevine justlevine reopened this Mar 7, 2023
@coveralls
Copy link

coveralls commented Mar 7, 2023

Coverage Status

Coverage: 84.873% (+0.05%) from 84.825% when pulling f7315e1 on justlevine:fix/handle-404s-in-node-resolver into ead0acb on wp-graphql:develop.

@justlevine justlevine changed the title fix: handle 404s in node resolver [WIP] fix: handle 404s in NodeResolver.php Mar 7, 2023
@justlevine justlevine marked this pull request as ready for review March 7, 2023 12:37
@codeclimate
Copy link

codeclimate bot commented Mar 8, 2023

Code Climate has analyzed commit f7315e1 and detected 0 issues on this pull request.

View more on Code Climate.

@jasonbahl jasonbahl merged commit f73eaa0 into wp-graphql:develop Mar 8, 2023
@jasonbahl jasonbahl mentioned this pull request Mar 8, 2023
@justlevine justlevine deleted the fix/handle-404s-in-node-resolver branch March 8, 2023 17:25
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.

nodeByUri resolving incorrectly when permalinks are set to a custom structure
3 participants