WordPress.org
Get WordPress

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#45975 closed enhancement (fixed)

wp_check_mysql_version should use wp_die

Reported by: spacedmonkey's profile spacedmonkey Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version: 2.1
Component: Database Keywords: has-patch
Focuses: Cc:

Description

wp_check_mysql_version should use wp_die

Attachments (2)

45975.diff​ (500 bytes) - added by spacedmonkey 7 years ago.
45975.1.diff​ (800 bytes) - added by spacedmonkey 7 years ago.

Download all attachments as: .zip

Change History (14)

@spacedmonkey
7 years ago

#1 @afercia
7 years ago

  • Focuses accessibility removed

Not sure why this ticket focuses accessibility. Going to remove the focus to clean-up the accessibility report. If you feel this issue is related to web content accessibility and universal design, please do feel free to re-add it.

Last edited 7 years ago by afercia (previous) (diff)

#2 @spacedmonkey
7 years ago

  • Milestone changed from Awaiting Review to 5.2

Marking as 5.2. See arguments for this ticket here.

#3 @SergeyBiryukov
7 years ago

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

#4 follow-up: @SergeyBiryukov
7 years ago

  • Keywords needs-patch added; has-patch removed

This would need some more changes.

wp_check_mysql_version() is currently called from wp_upgrade() and wp_install():

  • wp_upgrade() runs before any screen output, so far so good.
  • wp_install() runs after the opening <body> tag on wp-admin/install.php.

Currently, the die() call in wp_check_mysql_version() leaves a broken markup on wp-admin/install.php with no closing </body> tag.

With the patch, there are two <html> and <body> tags on the page, which is also invalid markup.

If we're going with wp_die() here, we should make sure it happens before any output.

#5 @SergeyBiryukov
7 years ago

Looking at display_header() usage in wp-admin/install.php, there are some more die() messages that could be converted to wp_die(): "Already Installed", "Insufficient Requirements", "Configuration Error", etc.

Last edited 7 years ago by SergeyBiryukov (previous) (diff)

#6 @desrosj
7 years ago

  • Keywords needs-refresh added

@spacedmonkey are you able to refresh the patch for the additional instances @SergeyBiryukov noticed?

@spacedmonkey
7 years ago

#7 @spacedmonkey
7 years ago

  • Keywords has-patch added; needs-patch needs-refresh removed

@SergeyBiryukov Amazing catch!

I have updated the patch to move the version check before the header is outputted.

As your other feedback about the other dies, I think you are right, I will follow up with another ticket.

#8 @SergeyBiryukov
7 years ago

  • Milestone changed from 5.2 to 5.3

Missed the 5.2 Beta 1 deadline, moving to 5.3.

#9 in reply to: ↑ 4 @SergeyBiryukov
7 years ago

  • Milestone changed from 5.3 to 5.2

Replying to SergeyBiryukov:

Currently, the die() call in wp_check_mysql_version() leaves a broken markup on wp-admin/install.php with no closing </body> tag.

Looks like I was wrong here. In my testing, I edited $wpdb->check_database_version() to always return an error. I should have edited $required_mysql_version instead.

Turned out wp-admin/install.php already checks PHP and MySQL requirements​ before running wp_install(), so 45975.diff​ is fine as is, no need for a second check there.

#10 @SergeyBiryukov
7 years ago

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

In 45030:

Database: Use wp_die() instead of die() in wp_check_mysql_version(), for more flexibility and consistency with other error messages in core.

Props spacedmonkey.
Fixes #45975.

#11 @spacedmonkey
7 years ago

  • Keywords needs-dev-note added

#12 @desrosj
6 years ago

  • Keywords needs-dev-note removed

Looks like this one never received a dev note. Going to remove needs-dev-note as the ship has already sailed.

Note: See TracTickets for help on using tickets.