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

Added filter for request path #5976

Conversation

adrianosferreira
Copy link

@adrianosferreira adrianosferreira commented Apr 4, 2018

Description

This fix is needed so third party plugins will be able to append URL arguments on internal requests, as Gutenberg concatenate the rest_url avoiding us to filter it on PHP side.

Types of changes

New feature (non-breaking change which adds functionality)

Issue: #5958

This fix is needed so third party plugins will be able to append URL arguments on internal requests, as Gutenberg concatenate the rest_url avoiding us to filter it on PHP side.
@adrianosferreira adrianosferreira force-pushed the filtering-request-path branch from 7753426 to adfbeb5 Compare April 4, 2018 13:24
@danielbachhuber
Copy link
Member

This seems like a very liberal filter whose abuse could lead to some unexpected results further down the road. Can you share how you would use it?

@adrianosferreira
Copy link
Author

@danielbachhuber something like this:

addFilter(
	'request.path',
	path,
	path => {
		const params = new URLSearchParams(path);
		return params.append( 'lang', icl_vars.current_lang );
	}
);

@danielbachhuber
Copy link
Member

@adrianosferreira Makes sense, thanks for clarifying.

@WordPress/gutenberg-core Have an opinion on this? I'm leaning towards 👎 because I think it's too liberal of a filter, but I don't have a reasonable alternative to suggest.

@youknowriad
Copy link
Contributor

Also, this is limited to withAPIData and we're in the process of refactoring these entirely in favor of core-data resolvers.

@danielbachhuber
Copy link
Member

@adrianosferreira Let's hold off on this particular change for now, and we can make sure we discuss #5958 again as a part of the merge proposal process

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.

3 participants