Devel backtraces should follow Drupal core's handling of E_DEPRECATED

No, I think we should rather work to remove the deprecated stuff. And since there aren't so many of us running PHP 5.3, we should report/fix the issues rather than ignore them.

The thing is, it is impossible for Drupal 6 to replace all ereg() functions since that would break the API, which is not allowed. See for example http://api.drupal.org/api/function/file_scan_directory/6, where $mask is passed directly to ereg().

Since ereg will not be removed from d6 core and instead the "fix" to core is to suppress E_DEPRECATED errors (see #233, #258, and #296 on #360605: PHP 5.3 Compatibility), it'd be nice if the 6.x branch of Devel conformed to core's handling of E_DEPRECATED.

Failing that, it's easy to modify devel.module to suppress E_DEPRECATED. Change line 494 from:

if ($errno & (E_ALL ^ E_NOTICE)) {

to

if ($errno & (E_ALL ^ E_NOTICE ^ E_DEPRECATED)) {

It's curious that devel suppresses notices but not deprecated messages.

Folks really use this custom error handler. I was thinking of removing it. Anyway, please roll a patch here for D6.

Here you go. The solution in #360605: PHP 5.3 Compatibility checks whether E_DEPRECATED is defined before using it, but I'm not sure why that's necessary. This patch doesn't.

It is necessary because the code still needs to run on PHP < 5.3, where that constant is not defined.

@Berdir: ah, I think I see my misunderstanding: includes/common.inc does define E_DEPRECATED if it's not supported, but your patch is to includes/boostrap.inc, and would be called before it's defined in common.inc?

If that's right, the patch in #6 is good. We don't need to define E_DEPRECATED in devel since includes/common.inc already does. This does introduce a requirement of Drupal 6.14 however.

Ah ok, yeah, I haven't actually looked at the patch, sorry ;)

Yes, the two E_DEPRECATED related things in bootstrap.inc and common.inc are not really related. First, we check in bootstrap.inc if we are on PHP 5.3 (in that case, E_DEPRECATED is defined) and forcefully remove it from the error_reporting setting until our own error handler is in place. Then in common.inc, we define E_DEPRECATED if it is not yet defined so that we are able to exclude it in our own error_handler.

Please review and rtbc if all is well here. thanks.

Note that several weeks ago I posted a variant of this issue as http://drupal.org/node/814998

Arguably, you may find my suggested fix an improvement over the one posted above, but it remains the case that the user should be able to choose what errors to backtrace through menu options rather than forcing a single policy, no matter what it is.

I believe that the error in Devel extends beyond treatment of E_DEPRECATED; it's handling of errors is in general inappropriate and idiosyncratic.
See my issue report for more information.

Debating whether deprecated issues should be removed is entirely beside the point, as quite clearly they are present in Drupal core and Devel is completely unusable under PHP 5.3 because of its behaviour.

Devel should not be attempting to drive Drupal core development policy by the back door. For that matter, it should not even be coercing users to avoid deprecated functions. The purpose of this module is (should be?) to assist debugging, not to enforce a coding standard.

Devel doesn't try to do error handling in its own "idiosyncratic" way: backtrace_error_handler() is practically identical to core's drupal_error_handler() with only a few minor alterations after the E_* check.

Devel as released now mimics core's error handling from before PHP 5.3 support was added (and I believe the original comments were reflecting core policy prior to the decision to add PHP 5.3 support to d6); the patch above brings it in line with core's current handling. I don't see the need for Devel to handle errors differently than core.

..."with only a few minor alterations after the E_* check."

Minor alterations with significant consequences.

A phrase like "practically identical" is unhelpful here. It's the way that Devel chooses for the user which errors to handle (or fail to handle) rather than the final processing of them which is idiosyncratic. Nevertheless, taken as a whole this renders the Devel handler problematic at times.

I think it's clear from my code snippet what I think is wrong here, which would allow an argument about facts rather than the appropriate language to describe them :) It is quite trivial to handle only the errors that the user has asked PHP to generate in the first place rather than forcing hardcoded values. One line of code... Or I could say "practically identical."

Clearly, I also believe that the lack of a feature where a user can select the errors to handle is something of a shortcoming in a module that is very much focused on error handling behaviour.

Perhaps it'd be helpful if you looked at the code for core's drupal_error_handler() and devel's backtrace_error_handler().

As you'll notice, with the patch above, the functions are identical except for two things:

• backtrace_error_handler() doesn't impose itself on update.php (since modules can't do that), and
• backtrace_error_handler() pretty prints the entire backtrace instead of showing the top of the stack like core does.

You'll also note that Devel isn't choosing to do anything: it's merely following core's error handling. Drupal core suppresses certain errors, so Devel does, too. A better place to voice your concern with how Drupal handles errors might be on the core issue queue.

Patch still applies.
Looks good to me.

Any chance of getting this into a Devel release? It's a bit annoying to have to patch it for every new installation.

Updating title

You'll also note that Devel isn't choosing to do anything: it's merely following core's error handling. Drupal core suppresses certain errors, so Devel does, too. A better place to voice your concern with how Drupal handles errors might be on the core issue queue.

The simplest way I can put it is that my expectations of Drupal core error handling and 'Devel' are different: one is for production, the other is for development.

On a production site almost all errors will be disabled at the PHP level; their reporting is not a big deal. However, when developing, fine control over errors is a good thing and probably the sort of feature that people expect from a module that provides enhanced error reporting functionality.

Personally, I find myself in a situation where I can't just install vanilla 'Devel' any longer, and instead have to apply my own patches to the error handling, which remains, even in the latest update, somewhat inflexible. Maybe my expectations are unrealistic...

Why don't we follow current error_reporting() settings?

for example:

if ($errno & (E_ALL ^ E_NOTICE) & error_reporting()) {

@nachinius because that's not how Drupal handles it.

However, if it were to use that code, why is E_NOTICE suppressed but not E_DEPRECATED?

@Mark Trapp

IMHO, I think drupal should handle too, since error_reporting() has the preferred options specified by the system/user/admin/coder elsewhere. Just to note, error_reporting is modifiable everywhere (has a PHP_INI_ALL changeable flag), i.e. can be modified from everywhere, from a php.ini, from a .htaccess, from a php-script.
I don't understand why Devel and Drupal,don't follow what is supposed to happen in the system: i.e. report only what was set previously on the error_reporting().

Given the previous arguments, I insist (until further evidence/arguments) that should be

if ($errno & error_reporting()) {

However, we may add more specific, like &~E_NOTICE if it makes sense to do so.

Cheers,

Bikeshedding how Drupal should handle errors is really superfluous to this issue, which is that—without the patch in #6—using backtraces in Devel on Drupal 6 results in dozens of site-killing backtraces for errors Drupal core already suppresses. Feel free to open a Drupal core issue or another issue in Devel to get how errors are handled changed.

The number of backtraces is not because there is an infinite loop inside devel.module. The "error" plain simply happens that many times because it inside a recursive function. So it is not finite, not infinite, just very often :)

The reason for excluding E_DEPRECATED is simple: Drupal 6 is not E_DEPRECATED compatible and *can not be made so* because that would be an API break and these are not allowed.

Not repeating backtraces is a nice idea but doesn't belong into this issue, because even a single backtrace on every page is one backtrace too much :)

@Berdir: you're right; I realized I misspoke and edited my post. It's not that drastic. :-P

I'm okay with an option, as long as it's enabled by default to conform to Drupal core's handling. The main issue with an option is that it would suppress/allow all E_DEPRECATED errors, both in core and contrib. So, while the intent of the option would be to suppress errors that will never be fixed, it'd actually have the effect of suppressing errors that a user might want (i.e. CatInTheHat and nachinius's concerns).

So, if there's an option to suppress it, the wording would have to be something like:

[x] Suppress E_DEPRECATED notices
Drupal normally suppresses E_DEPRECATED errors because some parts of core use functions that have been deprecated in PHP 5.3. Disabling this option will show all E_DEPRECATED errors, including ones in core that will not be fixed.

Although, if you add an option for E_DEPRECATED, it stands to reason there should be an option to suppress/enable all the different types of errors. Giving the user the ability to suppress/enable which errors to show seems like out of scope for this specific issue (i.e. getting rid of errors a user of Devel can do nothing about).

i committed this. let me know if we should do more. i am somewhat surprised that folks really use this feature.

There's some confusion here. moshe shut up E_DEPRECATED for D7 by changing

  if ($errno & (E_ALL ^ E_NOTICE)) {

into

  if ($errno & (E_ALL ^ E_NOTICE ^ E_DEPRECATED)) {

and this resulted in #953522: Use E_DEPRECATED only conditionally.

The way I see it, we should revert the D7 change and change D6 to

  if ($errno & (E_ALL ^ E_NOTICE ^ (defined('E_DEPRECATED') ? E_DEPRECATED : 0))) {

We want to shut up E_DEPRECATED messages for D6, not for D7, and we want to do this only if E_DEPRECATED is known.

Comments?

Oh, and the feature IS useful, even though a bit buggy. I'll fix it once we have this confusion cleaned up.

If it got into Drupal 7, it should be reverted: Drupal 7 core doesn't dictate what the error levels should be anymore, instead relying on error_reporting().

Starting with Drupal 6.14, Drupal defines E_DEPRECATED in includes/common.inc (see comment #8), so Devel shouldn't have to check for it. I'm not sure how necessary it is to support Drupal <= 6.14, as >= 6.14 were all security updates.

Ok, then we want only

  if ($errno & (E_ALL ^ E_NOTICE ^ E_DEPRECATED)) {

for D6. We don't need to support D6.14.

Let's take this one step at a time — can we agree that #28 and #31 is a step in the right direction and will allow us to close this issue?

I agree: Drupal 7 should have the change reverted, and Drupal 6 should stay the way it is. I don't think the way Devel handled it in Drupal 7 is correct, but I'll leave a comment in the other issue about that.

Arghh!!!

D6 currently has

  if ($errno & (E_ALL ^ E_NOTICE)) {

We want to change it to

  if ($errno & (E_ALL ^ E_NOTICE ^ E_DEPRECATED)) {

That's what this issue is all about! Read the title!

So, roll D7 back and instead roll D6 forward. Ok?

@salvis your comments seemed to indicate that the patch in #6 was committed to both the Drupal 6 and Drupal 7 branches, not that the patch missed the Drupal 6 entirely. I apologize for the confusion.

The patch in #6 was intended for Drupal 6, and should've been applied to the Drupal 6 branch. Drupal 7 should have what I mentioned in #953522-3: Use E_DEPRECATED only conditionally: even reverted, the Drupal 7 version is incorrect.

One step at a time. This issue is about D6 and the title says it all.

Let's fix this and the mess it created, and please stop talking in this issue about other shortcomings of other versions.

@salvis I'm trying to address what you're requesting: you brought up the Drupal 7 version when you re-opened this issue, and in every comment since. For Drupal 6, the patch in #6 is correct. It needs to be committed.

I mentioned D7 as far as that we should undo the erroneous commit #27 that caused #953522: Use E_DEPRECATED only conditionally.

D7 undone, D6 applied, both committed.