WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 5 months ago

#46418 reviewing enhancement

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

Reported by: immeet94 Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch needs-refresh
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 21 months ago.
Please see this file for patch
46418.2.diff (1.3 KB) - added by garrett-eclipse 18 months 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 13 months 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 13 months ago.
Refresh to account for commit#46739
46418.5.diff (21.6 KB) - added by immeet94 8 months ago.
46418.6.diff (23.0 KB) - added by immeet94 8 months ago.
46418.diff (23.1 KB) - added by whyisjake 6 months ago.

Download all attachments as: .zip

Change History (26)

@immeet94
21 months ago

Please see this file for patch

#1 @immeet94
21 months ago

  • Severity changed from normal to major

#2 @immeet94
21 months ago

  • Severity changed from major to normal
  • Version trunk deleted

#3 @desrosj
18 months 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
18 months ago

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

#4 @garrett-eclipse
18 months 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
13 months ago

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

#6 @SergeyBiryukov
13 months ago

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

@garrett-eclipse
13 months ago

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

#7 @garrett-eclipse
13 months 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
13 months ago

Refresh to account for commit#46739

#8 @garrett-eclipse
13 months 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
10 months 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
10 months ago

  • Milestone changed from 5.4 to 5.5

#11 @immeet94
8 months 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
8 months ago

#12 @garrett-eclipse
8 months 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
8 months 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
8 months ago

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


6 months ago

@whyisjake
6 months ago

#15 @whyisjake
6 months ago

Refreshed patch added.

#16 @garrett-eclipse
6 months 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.


5 months ago

#18 @thimalw
5 months 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
5 months ago

  • Milestone changed from 5.5 to Future Release
Note: See TracTickets for help on using tickets.