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

Trailingslash prevents WP Rest API from being cached #5900

Closed
4 tasks done
engahmeds3ed opened this issue May 5, 2023 · 5 comments · Fixed by #5906
Closed
4 tasks done

Trailingslash prevents WP Rest API from being cached #5900

engahmeds3ed opened this issue May 5, 2023 · 5 comments · Fixed by #5906
Assignees
Labels
effort: [XS] < 1 day of estimated development time module: cache priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@engahmeds3ed
Copy link
Contributor

engahmeds3ed commented May 5, 2023

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version Yes
  • Used the search feature to ensure that the bug hasn’t been reported before Yes

Describe the bug
When using our helper plugin which allows caching WP Rest API and permalinks has trailing slash so this blocks the following url from being cached at all:

http://example.com/wp-json/wp/v2

and to make it works we add the trailing slash for those RestAPI urls like:

http://example.com/wp-json/wp/v2/

the line that blocks it from being cached is:

if ( $this->maybe_allow_wp_redirect() ) {
return;
}

To Reproduce
Steps to reproduce the behavior:

  1. Activate our helper plugin
  2. Visit any WP Rest API url like http://example.com/wp-json/wp/v2
  3. Check the cache directory
  4. You will not find this json url cached
  5. Visit the same url but by adding trailing slash to it like http://example.com/wp-json/wp/v2/
  6. Check cache directory
  7. You will find it's cached

Expected behavior
I think WP Rest API needs to be excluded from this bailout so it can be cached if this helper is used

Additional context
Slack discussion: https://wp-media.slack.com/archives/C43T1AYMQ/p1682629639240929

Through this discussion, @joejoe04 tried a good solution related to force redirect the urls from not trailingslash to the trailingslash one by using a helper plugin and this worked like a charm.

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added type: bug Indicates an unexpected problem or unintended behavior module: cache priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available labels May 5, 2023
@piotrbak
Copy link
Contributor

piotrbak commented May 6, 2023

Acceptance Criteria

  • When using the helper plugin both versions, with and without / should be cached
  • Other trailing slash redirections inside the plugin should not be affected
  • Custom Rest API prefix is not redirected, the scope of this PR is only related to /wp-json/. If we see that setting custom Rest API route is being set often, we'll need to create option in our config file to store its value.

@jeawhanlee jeawhanlee added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels May 8, 2023
@jeawhanlee
Copy link
Contributor

Reproduce the problem ✅

I was able to reproduce the problem

Identify the root cause ✅

Exactly like @engahmeds3ed stated.

Scope a solution ✅

We can simply do an early return when the helper plugin is activated and API urls like that are accessed in

private function maybe_allow_wp_redirect(): bool {

if (! preg_match( '#wp\\-json#', $this->config->get_config( 'cache_reject_uri' ) ) && preg_match( '#wp-json/wp/v[0-9]+(/|)$#', $this->tests->get_request_uri_base() ) ) {
    return false;
}

Estimate the effort ✅

[XS]

@jeawhanlee jeawhanlee added effort: [XS] < 1 day of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels May 8, 2023
@jeawhanlee jeawhanlee self-assigned this May 8, 2023
@vmanthos
Copy link
Contributor

vmanthos commented May 9, 2023

A comment for QA.

The steps Ahmed described require a / to be there in the permalinks. If there isn't, REST API caching works as expected if the REST URI doesn't have a trailing /.

@piotrbak
Copy link
Contributor

piotrbak commented May 9, 2023

Yes, the problem is that before 3.13 both versions were cached, and WordPress doesn't offer redirection on its own :(

@DahmaniAdame
Copy link
Contributor

Temporary fix for this one until the update is released - https://drive.google.com/file/d/19sFnP9NX5van86ZGI-nKmm8tlpVGYa9C/view?usp=sharing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time module: cache priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants