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

graphql_pre_resolve_uri does not receive extra query vars #2515

Closed
maurocolella opened this issue Sep 12, 2022 · 9 comments
Closed

graphql_pre_resolve_uri does not receive extra query vars #2515

maurocolella opened this issue Sep 12, 2022 · 9 comments
Assignees
Labels
close candidate Needs confirmation before closing effort: low Around a day or less has: workaround A temporary workaround has been provided scope: api Issues related to access functions, actions, and filters type: enhancement Improvements to existing functionality

Comments

@maurocolella
Copy link

maurocolella commented Sep 12, 2022

What problem does this address?

I am trying to write custom resolution logic for single post queries based on slug.

Spec: https://github.com/wp-graphql/wp-graphql/blob/develop/src/Data/NodeResolver.php#L75

public function resolve_uri( string $uri, $extra_query_vars = '' ) {

/**
* When this filter return anything other than null, it will be used as a resolved node
* and the execution will be skipped.
*
* This is to be used in extensions to resolve their own nodes which might not use
* WordPress permalink structure.
*
* @param mixed|null $node The node, defaults to nothing.
* @param string $uri The uri being searched.
* @param AppContext $content The app context.
* @param WP $wp WP object.
*/
$node = apply_filters( 'graphql_pre_resolve_uri', null, $uri, $this->context, $this->wp);

However I want to preserve all of the logic, but in our situation we have a meta field, per object (posts including custom post types and users) that specifies a geographic region, and we want each user to only have access to content for their specific region.

But the hook doesn't receive $extra_query_vars and as a result, it is not aware of which content type is being passed; hence much of the resolution logic defaults to post or page, and fails for custom post types.

What is your proposed solution?

Ideally, if this hook is to substitute the original resolution, it should also receive the original arguments passed to the resolver function.

$node = apply_filters( 'graphql_pre_resolve_uri', null, $uri, $this->context, $this->wp, $extra_query_vars);

What alternatives have you considered?

I have considered:

  1. Using numeric IDs exclusively in single queries, but while it should have been the initial approach, it's too much of a refactor.
  2. re-declaring RootQuery single types, originally declared in register_post_object_fields and routing them to my own resolution function, but this defeats the point.
  3. Other manners of monkey patching which seemed evil (ie. "pluggable" WP Core functions to disambiguate on the resolution).
  4. A custom "where" GraphQL arg on single queries, inelegant.

And suggesting other ways/hooks to break page resolution down.

For example: a filter or function that "proxies" get_page_by_path (WP Core) and enables more granular implementations of custom singles resolution.

Depending on feasibility, this last suggestion might be the best long term option, but in the interim, I think simply passing $extra_query_vars through could do the job.

Additional Context

No response

@justlevine
Copy link
Collaborator

CC: #2366

@justlevine justlevine added type: enhancement Improvements to existing functionality scope: api Issues related to access functions, actions, and filters effort: low Around a day or less labels Sep 12, 2022
@justlevine
Copy link
Collaborator

In the interim as a workaround, you can add your necessary $query_vars using WP's built in request filter (source code).

E.g. (written from my phone, double check for typos ).

add_filter( 'request',
  function add_my_custom_query_vars( array $query_vars ) : array {
    if ( ! is_graphql_request() ) {
      return $query_vars;
   }
   
   if( 'my_cpt' ===$query_vars['post_type' ] ) {
     $query_vars = array_merge( $query_vars,
       [
         // my custom query vars here.
       ]
     );
   }
   
   return $query_vars;
    
  },
  10, 2 );

@justlevine justlevine added the has: workaround A temporary workaround has been provided label Sep 12, 2022
@jasonbahl
Copy link
Collaborator

@maurocolella I actually ran into this the other day as well!

I think the filter is placed too early.

The top of the function is (mostly) setting up context, then starting at line 352 most of the resolution begins and the function starts to return things.

I think we could move the filter down lower, right above line 352 (after all the context is established), but before the resolvers begin. Either that or introduce a new filter in that location.

@maurocolella
Copy link
Author

@jasonbahl I think that sounds nice. Thanks for taking the time!

@markkelnar
Copy link
Contributor

This filter, during development, appears to have been lower in the function, but moved to the top for specific reasons.

@jasonbahl
Copy link
Collaborator

@markkelnar good find!

This might be a good argument to introduce a second filter lower then.

One that happens after all the query args are determined, but before all (or at least most of) the returns are determined?

@justlevine
Copy link
Collaborator

@jasonbahl

I think we could move the filter down lower, right above line 352 (after all the context is established), but before the resolvers begin. Either that or introduce a new filter in that location.

I'm unclear why the best solution isnt what @maurocolella proposed initially (adding $extra_query_vars to the preexisting graphql_pre_resolve_uri filter). If want to to use short-circuit the method, its best to do that as early on as possible.

@yanmorinokamca
Copy link
Contributor

Hi, my company is trying to use nextjs - faustwp - wp-graphql - WPML for an existing wordpress website. WPML is installed and some custom post types are using the same slug in french / english.

It seems that WPML is overriding parse_query habitually to use the good slug for the current language, there is also another hacky patch to rewrite the query use by get_page_by_path in some content parsing.

My current solution, is to use graphql_pre_resolve_uri, check if the uri doesn't contains a "/", and do a query on wp_posts and wp_icl_translations for the slug in the current language. I've added a condition to reject post_types page, revision, nav_menu_item, attachment.

It seems to work for now, but it could create conflict with some special uri / taxonomies ?

With extra_query_vars in the filter, I would be a lot more easy to know if the request is using the "slug" and check for the post_type since name/post_type are sent in extra_query_vars.

@justlevine justlevine added close candidate Needs confirmation before closing and removed status: actionable Ready for work to begin labels Dec 7, 2022
@jasonbahl
Copy link
Collaborator

closed by #2582

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
close candidate Needs confirmation before closing effort: low Around a day or less has: workaround A temporary workaround has been provided scope: api Issues related to access functions, actions, and filters type: enhancement Improvements to existing functionality
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants