# [patch] error masks - Thread: https://externals.io/message/45348 - Original archive: https://news-web.php.net/php.internals/45348 - Started by Stanislav Malyshev on 2009-08-24T18:17:26+00:00 - Messages: 57 - Page 1 of 3: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/45348.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #45348 — Stanislav Malyshev — 2009-08-24T18:17:26+00:00 URL: https://externals.io/message/45348#45348 Hi! I've implemented a patch that allows to "mask" certain types of errors - i.e. make PHP engine completely ignore them. Now even if the error is not reported, it passes full cycle through message string creation, all allocations on the way, etc. even though ultimately the result of it will be thrown out. So I offer a way to make unwanted errors just disappear. The patch uses new directive - "error_mask", and reuses orig_error_reporting global (which is not used anywhere in PHP source so I wonder why it exists at all, but that's certainly convenient :) which makes it binary-compatible and fit for 5.3. Of course, in HEAD we could rename it. If the value in not 0, that's the mask which would filter out all errors that do not fit the mask (i.e. you could say in production that everything but warnings and errors should be discarded), and if the value is 0 then the mask is the same as error_reporting - i.e. it can be more dynamic and account for @, etc. and all non-reported errors will be discarded. Fatal errors will never be discarded. Alternative approach may be to just discard all non-reported errors, but that could be a problem with user error handlers and extension-supplied error callbacks, so the proposed approach is more flexible as you could control discarding and reporting separately. So, what do you think? -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45349 — Ilia Alshanetsky — 2009-08-24T18:22:04+00:00 URL: https://externals.io/message/45348#45349 · In reply to: #45348 Stas, While the patch is certainly interesting, I think masking errors is a really really bad idea. > Hi! > > I've implemented a patch that allows to "mask" certain types of > errors - i.e. make PHP engine completely ignore them. Now even if > the error is not reported, it passes full cycle through message > string creation, all allocations on the way, etc. even though > ultimately the result of it will be thrown out. So I offer a way to > make unwanted errors just disappear. > The patch uses new directive - "error_mask", and reuses > orig_error_reporting global (which is not used anywhere in PHP > source so I wonder why it exists at all, but that's certainly > convenient :) which makes it binary-compatible and fit for 5.3. Of > course, in HEAD we could rename it. > If the value in not 0, that's the mask which would filter out all > errors that do not fit the mask (i.e. you could say in production > that everything but warnings and errors should be discarded), and if > the value is 0 then the mask is the same as error_reporting - i.e. > it can be more dynamic and account for @, etc. and all non-reported > errors will be discarded. Fatal errors will never be discarded. > Alternative approach may be to just discard all non-reported errors, > but that could be a problem with user error handlers and extension- > supplied error callbacks, so the proposed approach is more flexible > as you could control discarding and reporting separately. > > So, what do you think? > -- > Stanislav Malyshev, Zend Software Architect > stas@zend.com http://www.zend.com/ > (408)253-8829 MSN: stas@zend.com > Index: main/main.c > =================================================================== > --- main/main.c (revision 287183) > +++ main/main.c (working copy) > @@ -640,6 +640,10 @@ > char *message; > int is_function = 0; > > + if(!ZEND_CAN_REPORT(type)) { > + return; > + } > + > /* get error text into buffer and escape for html if necessary */ > buffer_len = vspprintf(&buffer, 0, format, args); > if (PG(html_errors)) { > @@ -827,6 +831,9 @@ > char *params; > va_list args; > > + if(!ZEND_CAN_REPORT(type)) { > + return; > + } > spprintf(¶ms, 0, "%s,%s", param1, param2); > va_start(args, format); > php_verror(docref, params ? params : "...", type, format, args > TSRMLS_CC); > Index: Zend/zend.c > =================================================================== > --- Zend/zend.c (revision 287462) > +++ Zend/zend.c (working copy) > @@ -76,6 +76,17 @@ > } > /* }}} */ > > +static ZEND_INI_MH(OnUpdateErrorMask) /* {{{ */ > +{ > + if (!new_value) { > + EG(orig_error_reporting) = EG(error_reporting); > + } else { > + EG(orig_error_reporting) = atoi(new_value); > + } > + return SUCCESS; > +} > +/* }}} */ > + > static ZEND_INI_MH(OnUpdateGCEnabled) /* {{{ */ > { > OnUpdateBool(entry, new_value, new_value_length, mh_arg1, mh_arg2, > mh_arg3, stage TSRMLS_CC); > @@ -90,6 +101,7 @@ > > ZEND_INI_BEGIN() > ZEND_INI_ENTRY("error_reporting", NULL, ZEND_INI_ALL, > OnUpdateErrorReporting) > + ZEND_INI_ENTRY("error_mask", "-1", ZEND_INI_ALL, > OnUpdateErrorMask) > STD_ZEND_INI_BOOLEAN("zend.enable_gc", "1", ZEND_INI_ALL, > OnUpdateGCEnabled, gc_enabled, zend_gc_globals, > gc_globals) > #ifdef ZEND_MULTIBYTE > STD_ZEND_INI_BOOLEAN("detect_unicode", "1", ZEND_INI_ALL, > OnUpdateBool, detect_unicode, zend_compiler_globals, compiler_globals) > @@ -971,6 +983,10 @@ > zend_class_entry *saved_class_entry; > TSRMLS_FETCH(); > > + if(!ZEND_CAN_REPORT(type)) { > + return; > + } > + > /* Obtain relevant filename and lineno */ > switch (type) { > case E_CORE_ERROR: > Index: Zend/zend.h > =================================================================== > --- Zend/zend.h (revision 287462) > +++ Zend/zend.h (working copy) > @@ -766,6 +766,13 @@ > ZEND_API void zend_replace_error_handling(zend_error_handling_t > error_handling, zend_class_entry *exception_class, > zend_error_handling *current TSRMLS_DC); > ZEND_API void zend_restore_error_handling(zend_error_handling *saved > TSRMLS_DC); > > +#define ZEND_FATAL_ERROR_MASK (E_CORE_ERROR|E_PARSE|E_COMPILE_ERROR| > E_ERROR|E_USER_ERROR) > +#define ZEND_CAN_REPORT(type) (\ > + ((type & ZEND_FATAL_ERROR_MASK) != 0) || \ > + (EG(orig_error_reporting) == 0 && (type & EG(error_reporting)) != > 0) || \ > + (type & EG(orig_error_reporting)) != 0 \ > +) > + > #endif /* ZEND_H */ > > /* --- ## #45350 — Stanislav Malyshev — 2009-08-24T18:30:56+00:00 URL: https://externals.io/message/45348#45350 · In reply to: #45349 Hi! > While the patch is certainly interesting, I think masking errors is a > really really bad idea. Any argument as to why? -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45351 — Ilia Alshanetsky — 2009-08-24T18:57:58+00:00 URL: https://externals.io/message/45348#45351 · In reply to: #45350 Well, couple of reasons: 1) Once you completely hide an error there is absolutely no trace what so ever that it has even occurred, even if its the slowdown of the native error handler being. 2) Debugging code becomes that much more trickier, since a single option effectively turns off PHP's error handling completely. As far as I can tell the only benefit of this patch is to allow error prone code to run faster by avoiding the slow down of generating error message strings when errors occurs, but the downside is abuse by people who just want errors to go away... IMHO the downside outweighs the upside. > Hi! > > > […] > > Any argument as to why? > -- > Stanislav Malyshev, Zend Software Architect > stas@zend.com http://www.zend.com/ > (408)253-8829 MSN: stas@zend.com --- ## #45352 — Stanislav Malyshev — 2009-08-24T19:30:40+00:00 URL: https://externals.io/message/45348#45352 · In reply to: #45351 Hi! > 1) Once you completely hide an error there is absolutely no trace what > so ever that it has even occurred, even if its the slowdown of the > native error handler being. That's the point. And how the slowdown would be useful? > 2) Debugging code becomes that much more trickier, since a single option > effectively turns off PHP's error handling completely. That's not correct. It doesn't turn off php error handling - it allows you to specify which errors you handle and which not. That's what error_reporting should be doing from the start, but it does not. Of course, if you want notices, etc. to be displayed - display them, nobody prevents you from doing so and you can see the default is very permissive. But right now there's no way to save the busywork that happens on errors that you *don't* want to be displayed. How it helps your debugging if PHP allocates a bunch of memory, spends the cycles and throws out the result without ever keeping it anywhere? > As far as I can tell the only benefit of this patch is to allow error > prone code to run faster by avoiding the slow down of generating error > message strings when errors occurs, but the downside is abuse by people > who just want errors to go away... IMHO the downside outweighs the upside. This is not correct. There is a lot of code that deliberately ignores errors - with @, etc. - because in that particular context these errors are not useful. However there is no way to have the engine not to do all the work it does on the errors - even if you *know* you don't want this error to be displayed. There are a lot of completely legitimate cases where you want some errors temporary or permanently suppressed - but right now you don't have any means to do that, since error_reporting does not suppress the whole generation process, but only the very last stage of reporting. Which, frankly, makes very little sense - if you don't ever want to report it, why generate it? As for "abuse" - by which you mean running code with errors disabled I guess - everybody that ever ran third-party code in production knows that it is an everyday reality that a lot of code out there is not E_NOTICE-safe, even more is not E_STRICT/E_DEPRECATED/etc. safe. And production settings - including ones we recommend - do not include reporting these errors. So they serve absolutely no useful purpose but spending cycles on useless work - humans never see them, nobody debugs them, only thing they do is waste time. So one may preach that shouldn't happen as much as one wants, and call it "abuse" as much as one wants, but that's what happens and that's what will always happen. The question is only if people's sites would run slow or run faster. I don't see any gain in denying people means to run their sites faster. -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45353 — Ilia Alshanetsky — 2009-08-24T19:40:43+00:00 URL: https://externals.io/message/45348#45353 · In reply to: #45352 > Hi! > > > […] > > That's the point. And how the slowdown would be useful? Because it may force to fix the errors, rather then ignore them? > > > […] > > That's not correct. It doesn't turn off php error handling - it > allows you to specify which errors you handle and which not. That's > what error_reporting should be doing from the start, but it does > not. Of course, if you want notices, etc. to be displayed - display > them, nobody prevents you from doing so and you can see the default > is very permissive. But right now there's no way to save the > busywork that happens on errors that you *don't* want to be > displayed. How it helps your debugging if PHP allocates a bunch of > memory, spends the cycles and throws out the result without ever > keeping it anywhere? > > > […] > > This is not correct. There is a lot of code that deliberately > ignores errors - with @ There is very little modern code that is @ ridden like the stuff written before. The way to have the engine not do the work, would be as simple as fixing the code, which is the right solution here. > As for "abuse" - by which you mean running code with errors disabled > I guess - everybody that ever ran third-party code in production > knows that it is an everyday reality that a lot of code out there is > not E_NOTICE-safe, even more is not E_STRICT/E_DEPRECATED/etc. safe. The production value we use according to php.ini-production is `E_ALL` & ~E_DEPRECATED, which means `E_NOTICE` will not be hidden. > And production settings - including ones we recommend - do not > include reporting these errors. So they serve absolutely no useful > purpose but spending cycles on useless work - humans never see them, > nobody debugs them, only thing they do is waste time. Well, if you are going to hide them, the no one will ever debug them, let alone know that they exist. --- ## #45354 — Stanislav Malyshev — 2009-08-24T19:59:55+00:00 URL: https://externals.io/message/45348#45354 · In reply to: #45353 Hi! > Because it may force to fix the errors, rather then ignore them? Come on. That doesn't make any sense. If people deliberately disabled the error, they don't want to see it, and they will not see it. Making it slow doesn't do anything useful. Let's insert some `sleep()`'s into zend_error() too, would it make it better? Let's make `E_DEPRECATED` take 5 seconds, that'll show them. Making engine work slow to force people into some behavior is very misguided idea. I can't believe it is proposed to make invisible slowdowns in unknown places as a means to make people write better code. > There is very little modern code that is @ ridden like the stuff written If we discard emotional terms like "ridden" - there is such code in almost every major app out there. Just search for @fopen or @include or @xml or @eval - tons of examples. Ans the reason is simple - people do not always want PHP to display/log errors for them. Sometimes they don't care for the errors, sometimes they want to handle the error by other means. > before. The way to have the engine not do the work, would be as simple > as fixing the code, which is the right solution here. What you still can't see if that this code is not broken, there's nothing to fix - this code is EXPLICITLY saying "do this, and if there's a error - ignore it". That's what the author wants. It's not an omission or mistake - it's intentional, it's part of the requirements. Only error_reporting now is broken so ignored errors waste excessive time. The right solution here is to fix error system so ignored errors not waste excessive time. > The production value we use according to php.ini-production is `E_ALL` & > ~E_DEPRECATED, which means `E_NOTICE` will not be hidden. Which conveniently ignores my argument about all other error types and bigger argument that THERE ARE such types and people ignore them on purpose, not because they are stupid. > Well, if you are going to hide them, the no one will ever debug them, > let alone know that they exist. How one would know they exist if reporting is disabled? By guessing it from the slowdown? I can't believe deliberately slowing down user applications in unknown places is really being recommended as a way of debugging. The fact is we have error_reporting and @, and however you may hate them, they exist and they are used. My patch only makes them do what they should be doing from the start, while keeping BC and allowing a bit more flexibility in advanced cases where you have special needs (like monitoring system that collects unreported errors, etc.). -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45355 — Ilia Alshanetsky — 2009-08-24T20:32:41+00:00 URL: https://externals.io/message/45348#45355 · In reply to: #45354 > > > […] > > If we discard emotional terms like "ridden" - there is such code in > almost every major app out there. Just search for @fopen or @include > or @xml or @eval - tons of examples. Ans the reason is simple - > people do not always want PHP to display/log errors for them. > Sometimes they don't care for the errors, sometimes they want to > handle the error by other means. Unless your code is "ridden" or if you prefer "filled" with @ and/or errors the speed improvement would be next to impossible to measure since you'd be literally saving microseconds. You'd need to have a substantial chunk of your code generating errors for your optimizations to make a reliably measurable difference. More over, if the user defines an error handler, which many applications, frameworks, etc... do then your improvement is next to invisible in between the overhead of executing PHP code to process the error. > > > […] > > What you still can't see if that this code is not broken, there's > nothing to fix - this code is EXPLICITLY saying "do this, and if > there's a error - ignore it". Vast majority of E_NOTICEs are not fixed not because people don't want to, but rather due to the fact that for ages PHP's default settings were set to ignore those errors. And E_NOTICES btw often could've let people know about security faults before they were abused, such as uninitialized variables and array keys that could be injected via register_globals, `extract()` function, etc... Stas, my biggest issue is that you are making this configurable value that makes this open to abuse, rather then using 0 as default and matching the error message creation to error reporting settings. ILia --- ## #45356 — Carl P. Corliss — 2009-08-24T20:33:23+00:00 URL: https://externals.io/message/45348#45356 · In reply to: #45348 Stanislav Malyshev wrote: > Hi! > > I've implemented a patch that allows to "mask" certain types of errors - > i.e. make PHP engine completely ignore them. Now even if the error is > not reported, it passes full cycle through message string creation, all > allocations on the way, etc. even though ultimately the result of it > will be thrown out. So I offer a way to make unwanted errors just > disappear. I know I'm a nobody here but, I think this is a really important change. I have worked in a couple of different companies where we have specifically turned turned down the error log noise in our production environment but kept them turned on fully (E_ALL | E_DEPRECATED) in our dev/staging environments. While our production environments were technically a bit quieter log-wise, there was definitely no apparent performance benefit aside from saving having to write to the logs. In fact, during our transition from 4.x -> 5.x, we noticed a marked slowdown related to generation of `E_STRICT` messages even when `E_STRICT` was disabled - which I only realized through stepping through the source via gdb was because it was still going through the motions of creating the messages even though they were discarded later. Again, I know I'm nobody here but, I really hope you find this patch worth committing - I know I would. Thanks for listening, -- Carl P. Corliss --- ## #45357 — Stanislav Malyshev — 2009-08-24T21:01:28+00:00 URL: https://externals.io/message/45348#45357 · In reply to: #45355 Hi! > Unless your code is "ridden" or if you prefer "filled" with @ and/or > errors the speed improvement would be next to impossible to measure > since you'd be literally saving microseconds. You'd need to have a Microsecond here, microsecond there - this stuff adds up. And 2-3 mallocs of substantial size (IIRC some messages are long enough so emalloc caches do not apply) plus whole printf ordeal - even twice in some cases - are not that small change, especially if it could happen repeatedly. Of course it alone won't make you 50% speedup, but even 0.5% here and 0.5% there add up. BTW, if you benchmark only this thing (yes, I know, it's not realistic :) the difference is very, very measurable - error reporting slows down operation with ignored error 3-4 times. I think if I proposed an improvement that would speed up certain set of opcodes 3 times that'd be worth having, not? > to make a reliably measurable difference. More over, if the user defines > an error handler, which many applications, frameworks, etc... do then > your improvement is next to invisible in between the overhead of > executing PHP code to process the error. The thing is you would be in control of which errors too feed to the error handler and which not to. That's exactly the point - giving you the tools to control it. Including tools to deal with "noisy" code the way you like - which may be your way, my way or any way in between. If you don't care - default changes nothing, but if you do, you have the means to make it run faster. > Vast majority of E_NOTICEs are not fixed not because people don't want You are still focused on one particular case of E_NOTICE. It's not (only) what's it about, it's bigger. > set to ignore those errors. And E_NOTICES btw often could've let people > know about security faults before they were abused, such as > uninitialized variables and array keys that could be injected via > register_globals, `extract()` function, etc... Yes, yes - if only they were actually *seen* by anybody. The case we discuss here is when they *are not* seen anyway. So I don't see how your argument applies. Your argument is about entirely different thing which I do not argue with you about - that some warning should not be disabled/hidden. I agree with you. But some OTHER warnings should be, and that's the case I want to fix. > Stas, my biggest issue is that you are making this configurable value > that makes this open to abuse, rather then using 0 as default and > matching the error message creation to error reporting settings. It can be done too, but not having other configuration will disable some scenarios when you don't want the error to go through usual reporting mechanisms but want debugger/monitoring system still see it. I understand that if we designed error_reporting from scratch maybe we'd just say "one switch for everybody", but since we already have people that are used to having current system, I made it as flexible as possible. If people around here think it's too flexible I could just make it on/off (i.e. same as -1/0 with current patch), it's trivial. I just wanted to start with max flexibility. -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45358 — Jani Taskinen — 2009-08-24T21:24:12+00:00 URL: https://externals.io/message/45348#45358 · In reply to: #45357 Quite boring to read this thread where two persons argue about something abstract. Stas, can you give a real life example where your patch is necessary..? --Jani Stanislav Malyshev wrote: > Hi! > > > […] > > Microsecond here, microsecond there - this stuff adds up. And 2-3 > mallocs of substantial size (IIRC some messages are long enough so > emalloc caches do not apply) plus whole printf ordeal - even twice in > some cases - are not that small change, especially if it could happen > repeatedly. Of course it alone won't make you 50% speedup, but even 0.5% > here and 0.5% there add up. > BTW, if you benchmark only this thing (yes, I know, it's not realistic > :) the difference is very, very measurable - error reporting slows down > operation with ignored error 3-4 times. I think if I proposed an > improvement that would speed up certain set of opcodes 3 times that'd be > worth having, not? > > > […] > > The thing is you would be in control of which errors too feed to the > error handler and which not to. That's exactly the point - giving you > the tools to control it. Including tools to deal with "noisy" code the > way you like - which may be your way, my way or any way in between. If > you don't care - default changes nothing, but if you do, you have the > means to make it run faster. > > > […] > > You are still focused on one particular case of E_NOTICE. It's not > (only) what's it about, it's bigger. > > > […] > > Yes, yes - if only they were actually *seen* by anybody. The case we > discuss here is when they *are not* seen anyway. So I don't see how your > argument applies. Your argument is about entirely different thing which > I do not argue with you about - that some warning should not be > disabled/hidden. I agree with you. But some OTHER warnings should be, > and that's the case I want to fix. > > > […] > > It can be done too, but not having other configuration will disable some > scenarios when you don't want the error to go through usual reporting > mechanisms but want debugger/monitoring system still see it. I > understand that if we designed error_reporting from scratch maybe we'd > just say "one switch for everybody", but since we already have people > that are used to having current system, I made it as flexible as > possible. If people around here think it's too flexible I could just > make it on/off (i.e. same as -1/0 with current patch), it's trivial. I > just wanted to start with max flexibility. --- ## #45359 — Rasmus Lerdorf — 2009-08-24T21:41:07+00:00 URL: https://externals.io/message/45348#45359 · In reply to: #45358 I think we should do something along the lines of what Stas is suggesting. The current approach of allocating and sprintf'ing all messages regardless of whether they will ever be used for anything is painful and a huge impediment to adding informative `E_NOTICE` and `E_STRICT` messages in the future. A good example of the benefit of this patch is upgrading a server running older PHP code that causes a lot of `E_STRICT` messages. The performance hit can be significant to the point where people may downgrade to the older version until they can get around to cleaning up the code. Having the ability to properly turn off `E_STRICT` such that not only are `E_STRICT` messages not shown, but they also don't eat up valuable cpu cycles is something we should have done long ago. It is non-intuitive that disabling an error type doesn't also remove the performance hit associated with generating that error message. -Rasmus Jani Taskinen wrote: > Quite boring to read this thread where two persons argue about something > abstract. Stas, can you give a real life example where your patch is > necessary..? > > --Jani > > Stanislav Malyshev wrote: > > […] --- ## #45360 — Stanislav Malyshev — 2009-08-24T21:42:46+00:00 URL: https://externals.io/message/45348#45360 · In reply to: #45358 Hi! > Quite boring to read this thread where two persons argue about something > abstract. Stas, can you give a real life example where your patch is > necessary..? Any code where you either use @ or error_reporting which is not -1 would benefit from it by not processing errors that go nowhere. I just looked at Zend Framework - with is pretty clean with regard to E_NOTICE/E_STRICT problems - and @ is used in dozens of classes around. The speedup would be probably not very big for whole RL application, but it's a 10-line patch, and little things help too. -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45362 — Lukas Kahwe Smith — 2009-08-24T21:51:01+00:00 URL: https://externals.io/message/45348#45362 · In reply to: #45360 > Hi! > > > […] > > Any code where you either use @ or error_reporting which is not -1 > would benefit from it by not processing errors that go nowhere. I > just looked at Zend Framework - with is pretty clean with regard to > E_NOTICE/E_STRICT problems - and @ is used in dozens of classes > around. The speedup would be probably not very big for whole RL > application, but it's a 10-line patch, and little things help too. well a few of those places would probably be fixable, by providing functions to check beforehand if calling the final function would cause an error. but that if course would add more overhead, but would still be the "cleaner" solution. overall i am not so convinced about the ignoring approach. as for `E_STRICT` .. that shouldnt become less of an issue now that we have `E_DEPRECATED` .. but i guess that just means that in the future people will complain about `E_DEPRECATED` .. anyways to me both `E_STRICT` and `E_DEPRECATED` are development tools that can be totally ignored in production. however `E_NOTICE` should not occur in production and we shouldnt encourage people to make them disappear entirely. regards, Lukas Kahwe Smith mls@pooteeweet.org --- ## #45363 — Rasmus Lerdorf — 2009-08-24T21:59:48+00:00 URL: https://externals.io/message/45348#45363 · In reply to: #45362 Lukas Kahwe Smith wrote: > > > > > […] > > > well a few of those places would probably be fixable, by providing > functions to check beforehand if calling the final function would cause > an error. but that if course would add more overhead, but would still be > the "cleaner" solution. > > overall i am not so convinced about the ignoring approach. as for > `E_STRICT` .. that shouldnt become less of an issue now that we have > `E_DEPRECATED` .. but i guess that just means that in the future people > will complain about `E_DEPRECATED` .. > > anyways to me both `E_STRICT` and `E_DEPRECATED` are development tools that > can be totally ignored in production. however `E_NOTICE` should not occur > in production and we shouldnt encourage people to make them disappear > entirely. Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED, E_NOTICE, whatever, all cause a performance hit even if the error_reporting level is such that they will never show up anywhere. That's what this patch is trying to address. To write optimal code, they have to be entirely clean of all messages including `E_DEPRECATED` and E_STRICT. -Rasmus --- ## #45365 — Pierre Joye — 2009-08-24T22:08:07+00:00 URL: https://externals.io/message/45348#45365 · In reply to: #45362 hi Lukas, > > > > > […] > > > well a few of those places would probably be fixable, by providing functions > to check beforehand if calling the final function would cause an error. but > that if course would add more overhead, but would still be the "cleaner" > solution. To call a function to avoid a noisy or resource expensive error is just as bad. There are many common operations in PHP where we over react (file ops for example). As I agree with Rasmus' comments, I do think that this patch is the wrong fix for the real problem (that does not mean I do not like to have it in). I would however prefer to have APIs without any kind of noisy errors, at least for the common operations or where the error is obviously expected (as IO can fail). Please note that I did not suggest to use exceptions everywhere. Cheers, -- Pierre http://blog.thepimp.net | http://www.libgd.org --- ## #45366 — Lukas Kahwe Smith — 2009-08-24T22:11:47+00:00 URL: https://externals.io/message/45348#45366 · In reply to: #45363 > Lukas Kahwe Smith wrote: > > […] > > Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED, > E_NOTICE, whatever, all cause a performance hit even if the > error_reporting level is such that they will never show up anywhere. > That's what this patch is trying to address. To write optimal code, > they have to be entirely clean of all messages including `E_DEPRECATED` > and E_STRICT. > right .. what i was trying to say was that i can see value in being able to hide `E_DEPRECATED` and E_STRICT, but the error stuff shouldnt be hidden that way. users should either display or log their E_NOTICES and fix them! regards, Lukas Kahwe Smith mls@pooteeweet.org --- ## #45367 — Stanislav Malyshev — 2009-08-24T22:12:36+00:00 URL: https://externals.io/message/45348#45367 · In reply to: #45363 Hi! > Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED, > E_NOTICE, whatever, all cause a performance hit even if the > error_reporting level is such that they will never show up anywhere. > That's what this patch is trying to address. To write optimal code, > they have to be entirely clean of all messages including `E_DEPRECATED` > and E_STRICT. BTW "cleaning the messages" won't help with performance problems in many cases. Let's suppose you have a code that tries to open some file and if it exists, do stuff (and if it doesn't it doesn't care): $fp = @fopen($filename, "r"); if($fp) { // do stuff } Now if you "clean" it, you do something like: if(is_readable($filename)) { $fp = fopen($filename, "r"); // do stuff } (and if you don't want race condition there, you still need the if()). Then is_readable and fopen can still can't be guaranteed to not produce warnings if there are open_basedir restrictions or streams involved or FS could change in the meanitime. But now instead of one FS access, you have two - not much of a speedup. -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45369 — Lukas Kahwe Smith — 2009-08-24T22:20:03+00:00 URL: https://externals.io/message/45348#45369 · In reply to: #45367 > Hi! > > > […] > > BTW "cleaning the messages" won't help with performance problems in > many cases. Let's suppose you have a code that tries to open some > file and if it exists, do stuff (and if it doesn't it doesn't care): > $fp = @fopen($filename, "r"); > if($fp) { > // do stuff > } > Now if you "clean" it, you do something like: > if(is_readable($filename)) { > $fp = fopen($filename, "r"); > // do stuff > } > (and if you don't want race condition there, you still need the > if()). Then is_readable and fopen can still can't be guaranteed to > not produce warnings if there are open_basedir restrictions or > streams involved or FS could change in the meanitime. But now > instead of one FS access, you have two - not much of a speedup. right .. but it should reduce the chances of such errors occurring. and like i said .. that is obviously not a speed up, but a slow down. but i generally think its wise to encourage people to properly avoid error conditions locally, because otherwise sooner rather than later you will also no longer see the error conditions which you did not try to avoid/handle locally. regards, Lukas Kahwe Smith mls@pooteeweet.org --- ## #45371 — Greg Beaver — 2009-08-24T22:54:20+00:00 URL: https://externals.io/message/45348#45371 · In reply to: #45348 Stanislav Malyshev wrote: > Hi! > > I've implemented a patch that allows to "mask" certain types of errors - > i.e. make PHP engine completely ignore them. Now even if the error is > not reported, it passes full cycle through message string creation, all > allocations on the way, etc. even though ultimately the result of it > will be thrown out. So I offer a way to make unwanted errors just > disappear. > The patch uses new directive - "error_mask", and reuses > orig_error_reporting global (which is not used anywhere in PHP source so > I wonder why it exists at all, but that's certainly convenient :) which > makes it binary-compatible and fit for 5.3. Of course, in HEAD we could > rename it. > If the value in not 0, that's the mask which would filter out all errors > that do not fit the mask (i.e. you could say in production that > everything but warnings and errors should be discarded), and if the > value is 0 then the mask is the same as error_reporting - i.e. it can be > more dynamic and account for @, etc. and all non-reported errors will be > discarded. Fatal errors will never be discarded. > Alternative approach may be to just discard all non-reported errors, but > that could be a problem with user error handlers and extension-supplied > error callbacks, so the proposed approach is more flexible as you could > control discarding and reporting separately. > > So, what do you think? I think it's a good idea. Your patch does not take into account a user error handler, however. If present, ZEND_CAN_REPORT should evaluate to 1 to match existing behavior. For those who are worried about losing errors, I would pose a couple of questions: 1) if a tree falls in the forest and there is no one there to hear it, do we still have to put up with the performance loss? 2) as long as the patch does not break any backwards compatibility (error logging still works as it always did independent of error_mask, user error handlers still get the same stuff), why would we care? There is a time and place for being academic about fixing things and it is called development, not production. Greg --- ## #45372 — Lukas Kahwe Smith — 2009-08-24T23:02:05+00:00 URL: https://externals.io/message/45348#45372 · In reply to: #45371 > 1) if a tree falls in the forest and there is no one there to hear it, > do we still have to put up with the performance loss? > 2) as long as the patch does not break any backwards compatibility > (error logging still works as it always did independent of error_mask, > user error handlers still get the same stuff), why would we care? > There > is a time and place for being academic about fixing things and it is > called development, not production. If you have bugs in production, you sure as hell want to know about .. and encouraging people to ignore them is a recipe for disaster. Stats "@fopen()" example is perfect here, so lets say we do add this feature and people simply turn of error's entirely so that they can instead write "fopen()", they feel all good about themselves, since they handle the error locally and get a magical speed boost (noticeable or not) .. all the while they are ignoring all sorts of E_NOTICES that would indicate them that they have some serious security issues. Again, I am all for being able to totally ignore E_STRICT/E_DEPRECATED in production .. but there is a time for fixing E_NOTICES .. and that time is always! regards, Lukas Kahwe Smith mls@pooteeweet.org --- ## #45373 — Rasmus Lerdorf — 2009-08-24T23:05:03+00:00 URL: https://externals.io/message/45348#45373 · In reply to: #45372 Lukas Kahwe Smith wrote: > > > > > […] > > > If you have bugs in production, you sure as hell want to know about .. > and encouraging people to ignore them is a recipe for disaster. > Stats "@fopen()" example is perfect here, so lets say we do add this > feature and people simply turn of error's entirely so that they can > instead write "fopen()", they feel all good about themselves, since they > handle the error locally and get a magical speed boost (noticeable or > not) .. all the while they are ignoring all sorts of E_NOTICES that > would indicate them that they have some serious security issues. > > Again, I am all for being able to totally ignore E_STRICT/E_DEPRECATED > in production .. but there is a time for fixing E_NOTICES .. and that > time is always! I don't see how this has anything to do with the patch in question. You seem to be arguing that we shouldn't let people turn off `E_NOTICE` in production. How is that related to the patch? -Rasmus --- ## #45375 — Ilia Alshanetsky — 2009-08-24T23:30:25+00:00 URL: https://externals.io/message/45348#45375 · In reply to: #45371 > > 2) as long as the patch does not break any backwards compatibility > (error logging still works as it always did independent of error_mask, > user error handlers still get the same stuff), why would we care? > There > is a time and place for being academic about fixing things and it is > called development, not production. You must not have any bugs in production environment, I am very envious. :-) --- ## #45377 — Stanislav Malyshev — 2009-08-25T00:05:14+00:00 URL: https://externals.io/message/45348#45377 · In reply to: #45372 Hi! > Stats "@fopen()" example is perfect here, so lets say we do add this > feature and people simply turn of error's entirely so that they can > instead write "fopen()", they feel all good about themselves, since they > handle the error locally and get a magical speed boost (noticeable or > not) .. all the while they are ignoring all sorts of E_NOTICES that > would indicate them that they have some serious security issues. Actually, my point was that even if you write that code "right", only thing you achieve is slow code. It would still not be robust, unless you create if() and custom error handler for every imaginable error, it's still not more secure (since all the security works on stream level and applies to both is_readable and fopen anyway), summarily it's not better in any way but by giving you this nice feeling you "invested into nicer code". Which is totally imaginary since your code has virtually no difference from the old one except being slower. And all this because of @-phobia :) > Again, I am all for being able to totally ignore E_STRICT/E_DEPRECATED > in production .. but there is a time for fixing E_NOTICES .. and that > time is always! If you think it is (and note it's not the law of the universe - I could think about dozens of use cases where `E_NOTICE` is useless and you *know* you don't want it) - go ahead and set you error mask to include E_NOTICE, there's nothing easier. It's not like anybody is advocating there should be only one error mask. On the contrary, I advocate flexible approach where everybody can choose the mask that fits his particular use case. -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #45378 — Tjerk Anne Meesters — 2009-08-25T00:39:53+00:00 URL: https://externals.io/message/45348#45378 · In reply to: #45377 Hi, Expanding on this. In a typical project I always try to make my own code very clean by letting it run on a staging environment with all error reporting enabled for a while before living it; so keeping the error_reporting enabled on the live server (and sending a daily log of caught errors) would actually be helpful to me. However, when it comes to external libraries, I hate to do clean-up work there because it makes it harder to upgrade etc. With that in mind, I'd like to be able to muffle any errors from those. Likelihood is that those would cause errors in the main code anyway. Not sure how it would work, technically, but I was thinking it'd be nice to extend require*() and include*(). ;-) > Hi! > > > […] > > Actually, my point was that even if you write that code "right", only > thing you achieve is slow code. It would still not be robust, unless you > create if() and custom error handler for every imaginable error, it's > still not more secure (since all the security works on stream level and > applies to both is_readable and fopen anyway), summarily it's not better > in any way but by giving you this nice feeling you "invested into nicer > code". Which is totally imaginary since your code has virtually no > difference from the old one except being slower. And all this because of > @-phobia :) > > > […] > > If you think it is (and note it's not the law of the universe - I could > think about dozens of use cases where `E_NOTICE` is useless and you *know* > you don't want it) - go ahead and set you error mask to include > E_NOTICE, there's nothing easier. It's not like anybody is advocating > there should be only one error mask. On the contrary, I advocate > flexible approach where everybody can choose the mask that fits his > particular use case. > -- > Stanislav Malyshev, Zend Software Architect > stas@zend.com http://www.zend.com/ > (408)253-8829 MSN: stas@zend.com > > -- > > > > -- -- Tjerk --- Next page: https://externals.io/message/45348.md?page=2