WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 months ago

Last modified 5 weeks ago

#26262 closed defect (bug) (fixed)

Ajax requests shouldn't display errors

Reported by: pento Owned by: nacin
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords:
Focuses: Cc:

Description

Similar to #23811, displaying PHP errors in an Ajax response will probably cause a critical error in the client code. Instead, we should hide errors.

This is particularly important as #21663 is unlikely to make WordPress 3.8, which means all folks using WP_DEBUG on PHP 5.5 will be getting warnings they can't do anything about.

Attachments (3)

26262.diff (387 bytes) - added by pento 3 years ago.
26262.2.diff (592 bytes) - added by ocean90 3 months ago.
26262.3.diff (607 bytes) - added by ocean90 3 months ago.

Download all attachments as: .zip

Change History (28)

@pento
3 years ago

#1 @pento
3 years ago

  • Keywords has-patch dev-feedback added

#3 follow-ups: @SergeyBiryukov
3 years ago

I think this would make debugging AJAX issues harder. Code in production shouldn't have WP_DEBUG on.

#4 in reply to: ↑ 3 @pento
3 years ago

Replying to SergeyBiryukov:

I think this would make debugging AJAX issues harder.

It will make doing anything with PHP 5.5 impossible, unless #21663 makes 3.8.

#5 @bjornjohansen
3 years ago

What if we introduce a new constant (e.g. WP_DEBUG_LEVEL), and if defined, we use its value for error_reporting()?
Like this: http://pastebin.com/M5HU8y40

#6 follow-up: @dd32
3 years ago

It will make doing anything with PHP 5.5 impossible, unless #21663 makes 3.8.

FWIW, Developing with WP_DEBUG + PHP 5.5 since 3.7 has been a pain.. I don't think it's that much of a problem for a savvy person to install the mysqli plugin or pdo plugin though.

#7 in reply to: ↑ 6 @bjornjohansen
3 years ago

Replying to dd32:

FWIW, Developing with WP_DEBUG + PHP 5.5 since 3.7 has been a pain.. I don't think it's that much of a problem for a savvy person to install the mysqli plugin or pdo plugin though.

The MySQLi/PDO plugins aren't production ready, which means you'll have to maintain two separate codebases for dev and prod. Shouldn't be hard in this case, but still not a good principle.

There is also something wrong about installing a plugin to fix something which is outright wrong in core. It should be fixed in core.

A savvy person can also disable the error output and tail the error log instead. Or write their own error handler.

#8 @markoheijnen
3 years ago

Both plugins are production ready. MySQLi more then the PDO one obviously.

Hiding error on AJAX requests is wrong to do. When you enable WP_DEBUG you would expect that things break when something goes wrong. And we shouldn't do it because of 1 issue that can be solved.

#9 @ocean90
3 years ago

  • Milestone changed from 3.8 to Awaiting Review

#10 @nacin
2 years ago

  • Component changed from General to Bootstrap/Load

#11 follow-up: @Justin Folvarcik
2 years ago

I would suggest adding this information to the AJAX documentation pages. It could have saved me over five hours of trying to debug something was wasn't even a bug, and it's likely to confuse more people in the future because this information is not documented anywhere but here (that I could find).

#12 in reply to: ↑ 11 @adamsilverstein
2 years ago

Replying to Justin Folvarcik:

I would suggest adding this information to the AJAX documentation pages. It could have saved me over five hours of trying to debug something was wasn't even a bug, and it's likely to confuse more people in the future because this information is not documented anywhere but here (that I could find).

Good suggestion Justin! I know how frustrating debugging these types of issues can be, only to realize hours later the problem was something out-of-sight you couldn't observe directly.

I think this issue (corrupted response due to warning messages) is covered on the codex ajax instructions in the debugging section here- http://codex.wordpress.org/AJAX_in_Plugins#Debugging.

It looks like this section could use some improvements based on your experience. Note that the codex is community driven, meaning you can help improve it! Once you are logged into wordpress.org, you should see an edit link that will allow you to make changes. I will keep my eye on the page to review what you add.

Thanks for raising the issue - improving the documentation may help the next person with a similar problem.

#13 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
2 years ago

Replying to SergeyBiryukov:

I think this would make debugging AJAX issues harder. Code in production shouldn't have WP_DEBUG on.

On second thought, I saw some users with AJAX issues on support forums (Media Library not loading in the modal due to a broken plugin, for example). Looks like 26262.diff would have prevented that.

#14 in reply to: ↑ 13 @adamsilverstein
2 years ago

Replying to SergeyBiryukov:

Replying to SergeyBiryukov:

I think this would make debugging AJAX issues harder. Code in production shouldn't have WP_DEBUG on.

On second thought, I saw some users with AJAX issues on support forums (Media Library not loading in the modal due to a broken plugin, for example). Looks like 26262.diff would have prevented that.

I agree with the first part: I would be hesitant to suppress this output as it will make debugging more difficult. The errors can actually be seen when viewing the response in a browser debugger. Plenty of instruction out there about how to turn of error messages.

#15 follow-up: @Justin Folvarcik
2 years ago

I've updated the documentation to hopefully make it a bit clearer. I really hope it helps others avoid the fiasco that I went through.

#16 in reply to: ↑ 15 @adamsilverstein
2 years ago

Replying to Justin Folvarcik:

I've updated the documentation to hopefully make it a bit clearer. I really hope it helps others avoid the fiasco that I went through.

Nice work, thank you for your contribution!

#17 @pento
22 months ago

  • Keywords has-patch dev-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

This can also be worked around by using WP_DEBUG_LOG instead of sending the messages to the browser. There are sufficient alternatives that we shouldn't do anything about this in core.

#18 @johnbillion
22 months ago

Gratuitous self-promotion: my Query Monitor plugin catches errors in AJAX requests, alerts you to them in the admin toolbar, and displays their details in the browser console. Prevents the sort of breakage mentioned in this ticket.

#19 @nacin
6 months ago

  • Milestone set to 4.5
  • Resolution wontfix deleted
  • Status changed from closed to reopened

#20 @nacin
6 months ago

  • Owner set to nacin
  • Status changed from reopened to accepted

@ocean90
3 months ago

@ocean90
3 months ago

#22 @ocean90
3 months ago

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

In 36571:

Don't display errors during Ajax requests.

See #34915 for REST and #23811 for XML-RPC.

Props pento.
Fixes #26262.

#23 @ocean90
7 weeks ago

#36472 was marked as a duplicate.

#24 follow-up: @pavelevap
5 weeks ago

This change probably caused problems for many sites, see for example: https://wordpress.org/support/topic/problems-with-45-upgrade-add-media-not-working/

You can also Google for:

ini_set() has been disabled for security reasons in wp-includes/load.php on line 305

I found that it is caused by this change for sites which have diplay_errors set to "On" and disabled ini_set() for security reasons. In this case Ajax calls are returning and displaying PHP error and that is why Ajax calls fail. And for users it means that it is not possible to open Media modal window, insert media into post, etc. Should I create a new ticket for this?

#25 in reply to: ↑ 24 @SergeyBiryukov
5 weeks ago

Replying to pavelevap:

This change probably caused problems for many sites, see for example: https://wordpress.org/support/topic/problems-with-45-upgrade-add-media-not-working/

Thanks for the report, I've created #36708 as a follow-up (#36692 is also related).

Note: See TracTickets for help on using tickets.