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

Proposal: Fix incorrectly entered prefetches #2255

Closed
4 tasks done
webtrainingwheels opened this issue Jan 15, 2020 · 4 comments · Fixed by #2413
Closed
4 tasks done

Proposal: Fix incorrectly entered prefetches #2255

webtrainingwheels opened this issue Jan 15, 2020 · 4 comments · Fixed by #2413
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: preload priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@webtrainingwheels
Copy link

webtrainingwheels commented Jan 15, 2020

I see incorrect prefetch entries all the time:

wrong_prefetch

Could we automatically fix them?

Steps to Reproduce:

  • In Preload tab > Prefetch DNS Requests
  • Add an incorrect prefetch entry like http://example.com (or https://example.com)
  • This should be automatically corrected to //example.com (i.e. no protocol) when a user clicks Save Settings

Backlog Grooming

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@Tabrisrp Tabrisrp added module: preload type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels Jan 15, 2020
@GeekPress GeekPress added this to the 3.5.1 milestone Feb 27, 2020
@Tabrisrp
Copy link
Contributor

Tabrisrp commented Feb 28, 2020

Reproduce the issue
Reproduced on my local test site

Identify the root cause
In the sanitize_callback() method, we sanitize the field but we don't have anything to remove the scheme from the entries:

$input['dns_prefetch'] = rocket_sanitize_textarea_field( 'dns_prefetch', $input['dns_prefetch'] );

Scope a solution
We can add an additional sanitize to remove the http(s) scheme from each URL in the textarea field.

Estimate the effort
The code change itself is low effort. The current state of the sanitize_callback() method though might make writing automatic tests for this change very complicated.

@Tabrisrp
Copy link
Contributor

@hellofromtonya will need your input for the testing part of sanitize_callback()

@Tabrisrp Tabrisrp added effort: [S] 1-2 days of estimated development time and removed needs: grooming labels Mar 2, 2020
@arunbasillal
Copy link
Contributor

arunbasillal commented Mar 6, 2020

Acceptance criteria

Customer inputs any of these:

  • http://domain.com
  • https://domain.com
  • http://domain.com/example

WP Rocket converts this to:

  • //domain.com

What if:

  • customer inputs http:///domain.com (triple or more slashes)
    ^ My suggestion is that we ignore this for now. This is invalid input. The output will be ///domain.com

@GeekPress Inviting your thought on this ^

  • customer inputs http://domain.com/ (trailing slash)
    ^ We keep the trailing slash. It's okay for dns-prefetch to have trailing slash.

  • customer inputs ftp://domain.com (other protocols)
    ^ we can ignore all other protocols for now and do nothing. Just as we do in WP Rocket 3.5.0.x

@hellofromtonya
Copy link
Contributor

Moving this one to 3.5.2 for us to do the following requirements:

  • Detect malformed URLs, such as http:///domain.com or more more \
  • Repair these URLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: preload priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants