WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#30002 closed enhancement (fixed)

ms_not_installed returns 200

Reported by: spacedmonkey Owned by: craig-ralston
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.0
Component: Networks and Sites Keywords: good-first-bug has-patch
Focuses: multisite Cc:
PR Number:

Description

The ms_not_installed function returns apache state 200. It should return 500. Also there should been an action / filter to modify behaviour within this code.

Attachments (3)

30002.patch (644 bytes) - added by Craig Ralston 5 years ago.
Replaces die() with wp_die() in ms_not_installed()
30002.2.patch (740 bytes) - added by Craig Ralston 5 years ago.
use dead_db() with custom $wpdb->error if admin
30002.2.1.patch (737 bytes) - added by Craig Ralston 5 years ago.

Download all attachments as: .zip

Change History (17)

#1 @jeremyfelt
5 years ago

  • Type changed from defect (bug) to enhancement

In [14317], we moved from die(), which returns 200, to wp_die(), which returns 500 by default - but only in admin views. I'm not sure if there's a good reason to stick with die().

If we did switch to wp_die(), then a filter is available for wp_die_handler to customize that message.

We could also consider an action in ms_not_installed().

#2 @nacin
5 years ago

We should probably follow whatever dead_db() does. It's possible that dead_db() can be updated to take an arbitrary WP_Error|string as a first argument, defaulting to $wpdb->error otherwise (which is also WP_Error|string). If dead_db() is calling wp_die() early then we should be able to also.

#3 @spacedmonkey
5 years ago

I thought it would be better to just call dead_db function from ms_not_installed. It means less code and translation to maintain.

#4 @nacin
5 years ago

  • Keywords good-first-bug added

#5 @jeremyfelt
5 years ago

  • Milestone changed from Awaiting Review to Future Release

#6 @soulseekah
5 years ago

Seems it's possible to replace the die with wp_die inside that function, no? Just wp_die( $msg, $title );. dead_db is a bit messy and should probably be itself calling wp_die.

#7 @SergeyBiryukov
5 years ago

#30680 was marked as a duplicate.

@Craig Ralston
5 years ago

Replaces die() with wp_die() in ms_not_installed()

#8 @Craig Ralston
5 years ago

  • Keywords has-patch added

If I'm not mistaken, this is as simple as updating to use wp_die() in ms_not_installed().
Note: dead_db() also calls wp_die().

Adding an action to ms_not_installed() is also a good idea but probably should be in a separate ticket, I imagine.

Last edited 5 years ago by Craig Ralston (previous) (diff)

#9 @DrewAPicture
5 years ago

  • Owner set to craig-ralston
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-multisite by craigralston. View the logs.


5 years ago

#12 @jeremyfelt
5 years ago

  • Milestone changed from Future Release to 4.2

#13 @jeremyfelt
5 years ago

I think we should go with calling dead_db() instead. It provides the ability to show a custom db-error.php page and handles the 500 error exactly the same as ms_not_installed() would otherwise.

We can also take a cue from is_blog_installed() and set a custom $wpdb->error so that a db-error.php template can act accordingly. Everything we currently store in $msg could be passed this way and ms_not_installed() can just use dead_db(). A more generic message that doesn't rely on a DB query can be used when not in is_admin().

@Craig Ralston
5 years ago

use dead_db() with custom $wpdb->error if admin

#14 @jeremyfelt
5 years ago

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

In 31657:

Return HTTP status code 500 by default in ms_not_installed()

In admin views, specify a response code of 500 when using wp_die() to show an expanded message for a broken or missing multisite installation.

On front end views, use dead_db() rather than die() to generate the generic "Error establishing a database connection" message. dead_db() sets a status code of 500 by default and allows for the override of this generic error with a db-error.php template.

Props craig-ralston, jeremyfelt.

Fixes #30002.

Note: See TracTickets for help on using tickets.