Make WordPress Core

Opened 7 years ago

Closed 4 years ago

#39459 closed defect (bug) (fixed)

Database upgrade is triggered on AJAX request

Reported by: schlessera's profile schlessera Owned by: schlessera's profile schlessera
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.7
Component: Upgrade/Install Keywords: good-first-bug has-patch 2nd-opinion reporter-feedback
Focuses: Cc:

Description

After an update of WordPress Core from 4.5 to 4.7 on a multisite (not sure whether that's relevant or not here), my AJAX requests started failing. Looking at the responses I got back from the server showed that the server tried to trigger a database upgrade procedure on each AJAX request.

I'm not sure whether this is intended behavior or not, but it was certainly unexpected for me.

Should an AJAX request trigger the upgrade procedure? As the upgrade procedure is not meant to be needed immediately after an update, my first guess would be that AJAX requests should not trigger it and be able to complete successfully.

Attachments (3)

39459.diff (844 bytes) - added by jgrodel 7 years ago.
checks if doing ajax before doing upgrades
39459.1.diff (906 bytes) - added by elrae 4 years ago.
The patch here needed a refresh so added 39459.1.diff which should apply cleanly to 5.5 trunk. Left the conditionals on one line since multiple statements are on one line in other parts of core.
39459.2.diff (794 bytes) - added by Hareesh Pillai 4 years ago.
Patch refreshed against trunk

Download all attachments as: .zip

Change History (18)

#1 @johnbillion
7 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#2 @adamsilverstein
7 years ago

  • Keywords good-first-bug added

@jgrodel
7 years ago

checks if doing ajax before doing upgrades

#3 @jgrodel
7 years ago

  • Keywords has-patch added; needs-patch removed

#4 @johnbillion
7 years ago

  • Owner set to schlessera
  • Status changed from new to reviewing

#5 @schlessera
7 years ago

  • Keywords 2nd-opinion added

Patch looks good and tests are still passing.

However, I would personally prefer to have the multi-line conditions be split up before the boolean operator, so that these are the first element for each line. I think this is easier to read and reason about:

} elseif ( ! wp_doing_ajax()
        && get_option( 'db_version' ) != $wp_db_version
        && empty($_POST)
) {

I didn't find anything conclusive in the handbook, so not sure whether this is defined somewhere.

Also, I couldn't find integration tests that test mechanisms like this. What is the usual approach for testing something similar?

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

#7 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.5

@elrae
4 years ago

The patch here needed a refresh so added 39459.1.diff which should apply cleanly to 5.5 trunk. Left the conditionals on one line since multiple statements are on one line in other parts of core.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#9 @davidbaumwald
4 years ago

  • Keywords needs-refresh added

Latest patch fails against trunk, so marking for a refresh.

@Hareesh Pillai
4 years ago

Patch refreshed against trunk

This ticket was mentioned in PR #350 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#10

  • Keywords needs-refresh removed

#11 follow-up: @adamsilverstein
4 years ago

  • Keywords reporter-feedback added

@schlessera Hi! Thanks for opening this ticket. While your description and the code changes make sense, I was unable to reproduce the issue you describe.

To test, I left the editor open which makes regular heartbeat admin-ajax request. Then in another window I upgraded WordPress (or used wp-cli to set the db version to an older version). in my editor window continued to make successful ajax requests.

I also tried on a network install, leaving one site in the edit window and upgrading at the network level.

Do you have any additional steps to reproduce? where were you seeing the ajax errors (it is hard to get anywhere unless you leave a window open since all request redirect to the upgrade page)? I'm hesitant to change the code here without being able to confirm the issue.

#12 in reply to: ↑ 11 ; follow-up: @SergeyBiryukov
4 years ago

Replying to adamsilverstein:

I'm hesitant to change the code here without being able to confirm the issue.

Yes, wp-admin/admin.php (the file being patched here) doesn't seem to be loaded for AJAX requests in my testing, only wp-admin/includes/admin.php is.

So I would also be curious to know any specific steps to reproduce.

#13 in reply to: ↑ 12 @SergeyBiryukov
4 years ago

Replying to SergeyBiryukov:

Yes, wp-admin/admin.php (the file being patched here) doesn't seem to be loaded for AJAX requests in my testing

Ah, just noticed it is loaded in wp-admin/async-upload.php, so this might be reproducible with file uploads.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#15 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 48565:

Upgrade/Install: Don't trigger database upgrade on Ajax requests via wp-admin/async-upload.php.

Props schlessera, jgrodel, elrae, davidbaumwald, hareesh-pillai, adamsilverstein, SergeyBiryukov.
Fixes #39459.

Note: See TracTickets for help on using tickets.