Make WordPress Core

Opened 6 years ago

Last modified 5 years ago

#46910 accepted defect (bug)

Allow wp_die to called after html has started output

Reported by: spacedmonkey's profile spacedmonkey Owned by: sergeybiryukov's profile 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 6 years ago.

Download all attachments as: .zip

Change History (19)

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


6 years ago

#2 @ocean90
6 years 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
6 years 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
6 years 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.


6 years ago

#7 @flixos90
6 years ago

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

@spacedmonkey
6 years ago

#8 @spacedmonkey
6 years 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
6 years 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.


6 years ago

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


6 years ago

#12 @dimadin
6 years 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
6 years ago

  • Component changed from Bootstrap/Load to Site Health

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


5 years ago

#15 @afragen
5 years ago

  • Milestone changed from 5.3 to 5.4

#16 @afragen
5 years ago

  • Keywords servehappy removed

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


5 years ago

#18 @Clorith
5 years 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.