Make WordPress Core

Opened 7 months ago

Closed 5 months ago

Last modified 4 months ago

#45975 closed enhancement (fixed)

wp_check_mysql_version should use wp_die

Reported by: spacedmonkey Owned by: SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version: 2.1
Component: Database Keywords: needs-dev-note has-patch
Focuses: Cc:


wp_check_mysql_version should use wp_die

Attachments (2)

45975.diff (500 bytes) - added by spacedmonkey 7 months ago.
45975.1.diff (800 bytes) - added by spacedmonkey 5 months ago.

Download all attachments as: .zip

Change History (13)

7 months ago

#1 @afercia
7 months 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 months ago by afercia (previous) (diff)

#2 @spacedmonkey
7 months ago

  • Milestone changed from Awaiting Review to 5.2

Marking as 5.2. See arguments for this ticket here.

#3 @SergeyBiryukov
5 months ago

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

#4 follow-up: @SergeyBiryukov
5 months 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
5 months 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 5 months ago by SergeyBiryukov (previous) (diff)

#6 @desrosj
5 months ago

  • Keywords needs-refresh added

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

#7 @spacedmonkey
5 months 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
5 months 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
5 months 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
5 months 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
4 months ago

  • Keywords needs-dev-note added
Note: See TracTickets for help on using tickets.