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

Prevent auto-reloading on error pages in response to POST requests #763

Merged
merged 3 commits into from
Apr 15, 2022

Conversation

westonruter
Copy link
Collaborator

Originally in #697 (comment):

As part of #728, I just realized that this check will never do anything:

if ( isset( $_SERVER['REQUEST_METHOD'] ) && 'GET' !== $_SERVER['REQUEST_METHOD'] ) {
return;
}

This is because the offline template is always fetched via a GET request by the service worker when it is installing, so it will never not be GET. We need an alternative way of preventing the code from running on responses to non-GET requests.

Contrary to what I thought above, each reload does not cause the browser re-POST message: #728 (comment)

We need to prevent this from happening:

infinite-reload-on-post-submission-failure.mov

We need to export the request method from the service worker fetch handler to the JS in the wp_service_worker_offline_page_reload() so that it can decide whether or not to try reloading.

@westonruter westonruter added this to the 0.7 milestone Apr 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #763 (54ed60b) into develop (f93356d) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             develop     #763      +/-   ##
=============================================
- Coverage      19.03%   18.96%   -0.07%     
  Complexity       346      346              
=============================================
  Files             57       57              
  Lines           2322     2320       -2     
=============================================
- Hits             442      440       -2     
  Misses          1880     1880              
Flag Coverage Δ
php 18.96% <ø> (-0.07%) ⬇️
unit 18.96% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
wp-includes/template.php 7.69% <ø> (-3.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f93356d...54ed60b. Read the comment docs.

@westonruter westonruter marked this pull request as ready for review April 15, 2022 01:30
@westonruter westonruter requested a review from thelovekesh April 15, 2022 01:30
Copy link
Collaborator

@thelovekesh thelovekesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM 👍🏼

@westonruter westonruter merged commit 4133d08 into develop Apr 15, 2022
@westonruter westonruter deleted the fix/auto-reload-on-post-request-responses branch April 15, 2022 03:54
@pooja-muchandikar
Copy link

QA Passed ✅

The changes are working as expected. Automatic page reload is not happening if user is making the POST requests in offline mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants