Make WordPress Core

Opened 5 years ago

Last modified 8 months ago

#46418 reviewing enhancement

Use wp_die() instead of die() function in wordpress

Reported by: immeet94's profile immeet94 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch close
Focuses: Cc:

Description

When use the wp_json_encode() function that time you should use wp_die function instead of die() function.
Because wp_die() function do Kill WordPress execution and display HTML message with error message.

Attachments (7)

#46418.diff (461 bytes) - added by immeet94 5 years ago.
Please see this file for patch
46418.2.diff (1.3 KB) - added by garrett-eclipse 5 years ago.
Updated all instances of die to wp_die in class-wp-list-table.php
46418.3.diff (1.3 KB) - added by garrett-eclipse 5 years ago.
Refresh for 5.4, same code just reduces hunk offsets where were exceeding 25 lines.
48676.4.diff (1.2 KB) - added by garrett-eclipse 5 years ago.
Refresh to account for commit#46739
46418.5.diff (21.6 KB) - added by immeet94 4 years ago.
46418.6.diff (23.0 KB) - added by immeet94 4 years ago.
46418.diff (23.1 KB) - added by whyisjake 4 years ago.

Download all attachments as: .zip

Change History (28)

@immeet94
5 years ago

Please see this file for patch

#1 @immeet94
5 years ago

  • Severity changed from normal to major

#2 @immeet94
5 years ago

  • Severity changed from major to normal
  • Version trunk deleted

#3 @desrosj
5 years ago

  • Component changed from General to Administration
  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to Future Release

Hi @immeet94,

Welcome to Trac!

Thanks for this ticket. It seems that there are several more occurrences of die() in WP_List_Table. Are you able to look into fixing those as well?

@garrett-eclipse
5 years ago

Updated all instances of die to wp_die in class-wp-list-table.php

#4 @garrett-eclipse
5 years ago

  • Keywords needs-testing added; needs-refresh removed

In 46418.2.diff I've replaced all instances of die with wp_die in class-wp-list-table.php

#5 @sncoker
5 years ago

At the #WCUS Contributor Day. The patch applies cleanly to trunk.

#6 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@garrett-eclipse
5 years ago

Refresh for 5.4, same code just reduces hunk offsets where were exceeding 25 lines.

#7 @garrett-eclipse
5 years ago

  • Keywords commit added; needs-testing removed

Thanks for testing @sncoker I uploaded 46418.3.diff just because the offset was starting to get larger.

Marking for committer review, @SergeyBiryukov I see you were just looking at this as well, will leave in your hands.

Sidenote - In the affected lines I noticed the use of sub-class and over-ridden which are incorrect and should be subclass and overridden. To address this I've setup #48676 as those strings are seen in several other files as well so best to address seperately.

@garrett-eclipse
5 years ago

Refresh to account for commit#46739

#8 @garrett-eclipse
5 years ago

With r46739 updating the version being committed (thanks @SergeyBiryukov) I've updated the patch in 48676.4.diff.

While looking at this I wonder what other instances of die in core should become wp_die;
wp-activate.php line#18
wp-admin/admin-footer.php line#11
wp-admin/async-upload.php line#34
wp-admin/comment.php lines#95,100,106
wp-admin/edit-form-advanced.php line#11
wp-admin/edit-form-blocks.php line#13
wp-admin/edit-form-comment.php line#11
wp-admin/edit-link-form.php line#11
wp-admin/edit-tag-form.php line#11
wp-admin/export.php line#122
wp-admin/includes/class-pclzip.php line#221
wp-admin/includes/class-wp-site-health.php line#196
wp-admin/includes/image-edit.php line#28
wp-admin/includes/network.php lines#120,133,148
wp-admin/install.php lines#217,264,269,282
wp-admin/link-parse-opml.php line#10
wp-admin/upgrade.php line#35
wp-admin/user-edit.php lines#109,117
wp-admin/user-new.php lines#44,50,150,174,217
wp-cron.php lines#32,81,152
wp-includes/canonical.php lines#349,394,611
wp-includes/class-simplepie.php line#665
wp-includes/class-wp-ajax-response.php line#160
wp-includes/class-wp-widget.php line#113
wp-includes/comment.php line#716,930
wp-includes/embed.php line#728
wp-includes/functions.php lines#3419,3459,3499,3541,3573,3617,3639,3641,4513
wp-includes/IXR/class-IXR-server.php line#47
wp-includes/load.php lines#35,219,614,1449
wp-includes/ms-deprecated.php lines#116,615
wp-includes/ms-files.php lines#15,24,30
wp-includes/pluggable.php lines#1130,1184
wp-includes/rest-api.php line#308
wp-includes/theme-compat/comments.php line#20
wp-includes/wp-db.php line#1678
wp-settings.php line#530
wp-signup.php lines#14,34,39,412
wp-trackback.php line#35

#9 @davidbaumwald
4 years ago

@SergeyBiryukov The latest patch still good. Is this still a commit candidate for 5.4 with Beta 1 in a few days?

#10 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.4 to 5.5

#11 @immeet94
4 years ago

Hello

as per the @garrett-eclipse suggestion try to replace the wp_die() in all files so please review the patch @SergeyBiryukov and @davidbaumwald

Thanks.

@immeet94
4 years ago

#12 @garrett-eclipse
4 years ago

Thanks @immeet94 taking a look one thing to be cautious about is when wp_die the function becomes available which is in wp-includes/functions.php. In index.php the wp-includes/load.php is required before the functions one so the changes there may fail so I would ensure especially the changes in wp-includes/load.php are tested thoroughly.

#13 @immeet94
4 years ago

Hello @garrett-eclipse

i miss the file wp-includes/functions.php for the wp_die so updated with wp_die

please check the patch

thanks

@immeet94
4 years ago

This ticket was mentioned in Slack in #core by immeet94. View the logs.


4 years ago

@whyisjake
4 years ago

#15 @whyisjake
4 years ago

Refreshed patch added.

#16 @garrett-eclipse
4 years ago

  • Keywords needs-refresh added; commit removed

Taking a look my main hesitation is load order and when wp_die becomes available. It seems to be loaded here;
https://github.com/WordPress/wordpress-develop/blob/master/src/index.php#L25
So to my understanding anything loaded before this such as the WPINC/load.php will give an error and shouldn’t use wp_die.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#18 @thimalw
4 years ago

The comment above is right, we need to consider from which point on the wp_die function is available.

There are instances of wp_die being used in load.php already, but the functions.php file is loaded in just in that spot each time.

The index.php file in trunk is actually only used in development, so in production where _index.php is used, it actually seems to be loaded from https://core.trac.wordpress.org/browser/trunk/src/wp-load.php#L54 if wp-config.php doesn't exist, and from https://core.trac.wordpress.org/browser/trunk/src/wp-settings.php#L111 if wp-config.php exists. And there are quite a bit of files that are loaded in before wp_die is available, especially in the case where wp-config.php is available.

So if we're going to start using wp_die in all or most instances, my suggestion is to move the function to the load.php file. There already seems to be other functions in load.php that were moved in from other files including functions.php.

#19 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.5 to Future Release

#20 @SergeyBiryukov
8 months ago

#58420 was marked as a duplicate.

#21 @SergeyBiryukov
8 months ago

  • Keywords close added; needs-refresh removed
  • Owner SergeyBiryukov deleted

As previously noted in comment:5:ticket:58420, replacing die() with wp_die() everywhere does not seem to work as expected in my testing, as the $message parameter in wp_die() is an empty string by default, so it just displays an empty page without any message.

Looking at 46418.diff, unless I'm missing something, the usage of die() is intentional in pretty much all of these instances, as the output is already handled in a different way and we only need to terminate the script.

Using wp_die() there would cause unexpected output, additional overhead, or lead to "headers already sent" warnings when called after wp_redirect(). It also does not display any helpful error messages by itself, as the message is empty by default.

I believe there are no plans for replacing all the instances of die() at this time. If there are instances where wp_die() would be particularly helpful, those may be better addressed on a case-by-case basis.

Note: See TracTickets for help on using tickets.