WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 9 months ago

#46910 accepted defect (bug)

Allow wp_die to called after html has started output

Reported by: spacedmonkey Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-patch
Focuses: Cc:

Description

Currently if wp_die is called in either front end or in the cms, after html has started to render html, it makes an extremely broken looking page. This is because the default wp_die will output another html / body tag and css.

The wp_die function could somehow detect that html render had started, by saying checking an action has been run or a constraint was set, then instead of outputting the html start tags and would instead just output Javascript. This Javascript could display either an overlay dialog or even an a pure Javascript based alert displaying the message.

This feature would be specially useful for the fatal error protection feature in WordPress 5.2.

Attachments (1)

46910.diff (1.1 KB) - added by spacedmonkey 18 months ago.

Download all attachments as: .zip

Change History (19)

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


19 months ago

#2 @ocean90
19 months ago

The default wp_die handler already checks for did_action( 'admin_head' ) to not print the opening tags and CSS in admin. But it still always prints the closing tags which should probably only be done if exit is set.

#4 @spacedmonkey
19 months ago

@ocean90 Good spot. But doesn’t do the same for the thing front end. The check for did_action(‘admin_head’) could be changed for check for wp_head as well. Meaning, the wp_die would work much better on the front end. But it might mean styling would look weird, but this maybe fixable by theme developers...

#5 @SergeyBiryukov
19 months ago

  • Component changed from General to Bootstrap/Load
  • Milestone changed from Awaiting Review to 5.2
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


19 months ago

#7 @flixos90
19 months ago

#46811 fixed this for the admin, let's continue exploring here for how to cover the frontend.

@spacedmonkey
18 months ago

#8 @spacedmonkey
18 months ago

  • Keywords has-patch added; needs-patch removed

Uploaded the first patch 46910.diff as a talking point.

This is a very simple patch, that just makes sure that html / body tags are not re-opened on the front end. All it does it makes you an unstyle div with the error message is displayed on the page. Personally, I think this is what the end patch should look like. I don't like the idea of trying to build a dialog that works on every theme ever write. I think we should just output a div with an id, and leave it up the theme creates to style it (if they so wish), in a way that fits in with the styling of the theme.

#9 @pento
18 months ago

  • Milestone changed from 5.2 to 5.3

This patch needs further discussion, so bumping to 5.3.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


18 months ago

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


17 months ago

#12 @dimadin
16 months ago

I have reviewed this patch and here is what I found:

  • First check doesn't work on frontend until second && (before ! did_action( 'wp_head' )) is not replaced with ||.
  • when you use wp_die() on wp_head, there is no any styling as you don't get CSS styles anymore.
  • wp_body_open check will fail on most themes.
  • Any reason why wrapper for error message is part of this?

#13 @spacedmonkey
16 months ago

  • Component changed from Bootstrap/Load to Site Health

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


13 months ago

#15 @afragen
13 months ago

  • Milestone changed from 5.3 to 5.4

#16 @afragen
13 months ago

  • Keywords servehappy removed

This ticket was mentioned in Slack in #core-site-health by clorith. View the logs.


9 months ago

#18 @Clorith
9 months ago

  • Milestone changed from 5.4 to Future Release

As the ticket hasn't had any activity lately, and was moved previously for needing more discussions on the proposed solution, I'm moving it to Future Release.

if discussions happen, and it's agreed upon how to proceed with time for 5.4, please feel free to move it back into the milestone at such a time.

Note: See TracTickets for help on using tickets.