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

403 URLs arenot removed from caching table #5810

Closed
4 tasks done
Mai-Saad opened this issue Mar 13, 2023 · 4 comments · Fixed by #5865
Closed
4 tasks done

403 URLs arenot removed from caching table #5810

Mai-Saad opened this issue Mar 13, 2023 · 4 comments · Fixed by #5865
Assignees
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: preload priority: low Issues that can wait severity: moderate Feature isn't working as expected but has work around to get same value type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@Mai-Saad
Copy link

Mai-Saad commented Mar 13, 2023

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

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

Describe the bug
Using Ultimate Member plugin, will add URL to cache table like https://new.rocketlabsqa.ovh/void(0);, this URL isnot removed once processed

To Reproduce

  • Ultimate Member plugin is activated
  • WP sitemap is disabled (URLs fetched from home page)
    Steps to reproduce the behavior:
  1. install and activate WPR
  2. Check cache table => mentioned URL is added and once processed will fail

Expected behavior
403 URLs shall be removed as same as 404 URLs

Screenshots
If applicable, add screenshots to help explain your problem.
Screenshot from 2023-03-13 11-55-03

Additional context
Add any other context about the problem here.
Screenshot from 2023-03-13 12-25-17
We shouldn't fetch this kind of URL.

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@Mai-Saad Mai-Saad added type: bug Indicates an unexpected problem or unintended behavior 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting module: preload priority: low Issues that can wait severity: moderate Feature isn't working as expected but has work around to get same value labels Mar 13, 2023
@mehedihasanziku
Copy link

#5543 maybe it should tackle also with this case

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

Reproduce the problem ✅

I was able to reproduce this issue on NGINX.

Identify the root cause ✅

We are not removing urls that don't have status of 200

Scope a solution ✅

We can make a request to get the response status of the url and bail out if not 200 in preload_url

public function preload_url( string $url ) {

$response = wp_remote_get( $url );

if ( is_wp_error( $response ) ) {
	$this->query->delete_by_url( $url );
	return;
}

$response_code = wp_remote_retrieve_response_code( $response );

if ( 200 !== $response_code ) {
	$this->query->delete_by_url( $url );
	return;
}

Estimate the effort ✅

[S]

@jeawhanlee jeawhanlee added effort: [S] 1-2 days of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Apr 4, 2023
@jeawhanlee
Copy link
Contributor

Reproduce the problem ✅

I was able to reproduce this issue on NGINX.

Identify the root cause ✅

We are not removing urls that don't have status of 200

Scope a solution ✅

We can make a request to get the response status of the url and bail out if not 200 in preload_url

public function preload_url( string $url ) {

$response = wp_remote_get( $url );

if ( is_wp_error( $response ) ) {
	$this->query->delete_by_url( $url );
	return;
}

$response_code = wp_remote_retrieve_response_code( $response );

if ( 200 !== $response_code ) {
	$this->query->delete_by_url( $url );
	return;
}

Estimate the effort ✅

[S]

Following a point from @mostafa-hisham, the above grooming might be resource consuming on the customer site as we will be doing 2 requests to each url:

  • one to check for status code.
  • and if response code is 200 another again to preload.

As suggested by @mostafa-hisham we can also have a compatibility class for ultimate members and exclude urls like that void(0);
or I'm thinking we can utilize this request here and check the response code and perform necessary actions, don't know how well that will work.

@piotrbak @wp-media/php what do you think?

@piotrbak
Copy link
Contributor

piotrbak commented Apr 5, 2023

@jeawhanlee I believe that both options are not good enough:

  1. As Mostafa mentioned, it'll create additional request
  2. We're sending not-blocking request, it means that we're not waiting for the response

Maybe it's good time to move this array to the backend?

$regexes = (array) apply_filters( 'rocket_preload_exclude_urls', [ $pagination_regex ] );

Pinging @mostafa-hisham

@piotrbak piotrbak added this to the 3.13.3 milestone Apr 10, 2023
@mostafa-hisham mostafa-hisham self-assigned this Apr 13, 2023
@piotrbak piotrbak removed this from the 3.13.3 milestone Apr 23, 2023
@piotrbak piotrbak added this to the 3.13.2 milestone Apr 24, 2023
This was referenced May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: preload priority: low Issues that can wait severity: moderate Feature isn't working as expected but has work around to get same value type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants