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

Add filter for search_pattern in handle_generate_endpoint #1872

Open
cguagenti opened this issue Apr 8, 2024 · 5 comments
Open

Add filter for search_pattern in handle_generate_endpoint #1872

cguagenti opened this issue Apr 8, 2024 · 5 comments
Assignees

Comments

@cguagenti
Copy link

Context:

I'm testing Faust with WordPress Bedrock, and due to the different structure, when previewing content, it results in a 404 error.

This issue is caused by the $search_pattern in handle_generate_endpoint().

function handle_generate_endpoint() {
$search_pattern = ':^' . site_url( '/generate', 'relative' ) . ':';
if ( ! preg_match( $search_pattern, $_SERVER['REQUEST_URI'] ) ) { // phpcs:ignore WordPress.Security
return;
}

Because Bedrock returns the prefix /wp/ in site_url( '/generate', 'relative' ) (/wp/generate) while $_SERVER['REQUEST_URI'] contains only /generate[...], it always fails at the statement below:

if ( ! preg_match( $search_pattern, $_SERVER['REQUEST_URI'] ) ) { // phpcs:ignore WordPress.Security
	return;
}

Suggestion

Since site_url() returns the URL for the current site where WordPress applications are accessible, the best way to move forward is to use a callback filter (apply_filters) so anyone can customize it with a callback filter.

/**
* Filter 'faustwp_generate_endpoint_search_pattern'.
*
* @param string  $value   The search pattern.
*/
$search_pattern = apply_filters( 'faustwp_generate_endpoint_search_pattern', $search_pattern );

Example of how to use the filter

add_filter('faustwp_generate_endpoint_search_pattern', function ($value) {
	return str_replace('/wp/', '/', $value);
});
@theodesp theodesp added good first issue Issue that doesn't require previous experience with codebase help wanted labels Apr 9, 2024
@jan-clockworkwp
Copy link

Hey @theodesp, I am confirming that @cguagenti suggestion solves previews pages 404 issue on non-WordPress standard installations where admin url differs from the siteurl. Can we hope to have that filter added to the official package, or maybe as an option in the plugin options? Many thanks.

@ChrisWiegman
Copy link
Contributor

I like your idea here but I don't think it solves the whole problem. We actually have a 2nd ticket right now, #1882, that looks to solve Bedrock paths with a JS filter in Faust. In isolation both approaches would solve the specific symptoms but the larger issue of Bedrock and similar architecture remains. I would rather solve this upstream where 1. a single change will make sure Bedrock and similar work without multiple filters, etc and 2. it should be solvable whether you're using Faust or not. To further discuss an approach I've opened wp-graphql/wp-graphql#3145 as a result.

@justlevine
Copy link
Contributor

justlevine commented May 31, 2024

On quick review, the underlying issue is that Faust is conflating/inconsistently using the site_url, home_url, and internal Faust frontend_uri, both on the backend and frontend sides.

(I assume the decision to create this endpoint on parse_request instead of as a real endpoint (RESTful or otherwise) was borne out of this as well ).

The immediate solution from a code quality, tech debt and DX pov would be to drop the optimistic home_url === site_url === wpUrl, and use the correct ones on the frontend (long term also in the plugin, but that's beyond the scope if this particular SERVER['REQUEST_URI] check).

I'm assuming that's the premise for the exploration happening in wp-graphql/wp-graphql#3145, but I'm not seeing the why we would want to query the siteUrl via GraphQL before we use it elsewhere in the frontend (the request in that ticket).

This would seem to make much more sense as an optional .env constant (that inherits the home url if unset)

@ChrisWiegman can you confirm/correct my assessment?

@ChrisWiegman ChrisWiegman assigned theodesp and unassigned ChrisWiegman Jun 5, 2024
@jan-clockworkwp
Copy link

@ChrisWiegman @theodesp was there any progress done on handling different than default WordPress wp-admin setups? I can see @theodesp has done PR that could help with FE issues, but it is in review stage.

Currently, we need to apply various tweaks to the front-end and back-end, but these tweaks cannot solve everything we would like to address. As you can imagine, making tweaks to the plugin and Faust.js packages is not ideal for projects in staging or production phases.

Thank you for your input.

@ChrisWiegman
Copy link
Contributor

I’m no longer with WP Engine. Removing myself from this ticket.

@josephfusco josephfusco removed the good first issue Issue that doesn't require previous experience with codebase label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants