Re: [PHP-DEV] [patch] error masks
Rasmus Lerdorf wrote: There will always be a reason to not report certain types of errors. E_STRICT and E_DEPRECATED are informative development-level messages that have no place on a production system. They should be turned off. The actual real problem is that our error mechanism only allows us to turn off the final output stage, but not the actual generation of those errors. That is the real issue. I do agree that having another ini switch isn't ideal, but I have pondered this a few times and I don't see a solution that doesn't involve some sort of configuration like that because of custom error handlers and the php_errormsg issues. -Rasmus Hello, Then why not use a function instead ? Or extend error_reporting() to support Stas' proposal ? That way: - the end-user will be able to activate the mask on a per-script basis - it *will* take him/her time and effort to entirely suppress error handling on a 3rd party app - which might incite him/her to actually fix the errors instead - it will not affect other apps running on the same server (ever thought about shared hosting with the ini switch ?) - any decent programmer will find a clean way to activate the feature for his entire app in production if this is desirable In the documentation then, there'd be this big red warning box explaining the bad mojo stuff... On a personal note, I'm in favour of this patch: what's the sense of processing errors if they don't get printed out anywhere ? Just my 2¢ Steven -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Personally, I generally prefer log_errors and E_ALL in production. That said, there have been cases where it really wouldn't make much sense to log the errors on a web tier with many nodes. Or, at least, it didn't make sense to *just* log the errors. But I digress. I believe Stas has made a good pragamatic argument for a reasonable use case that improves performance in common enough usage by people who do know what they are doing, and want/need the performance gain. The detractors have, largely, not been cogent, to be frank. The most reasonable have been to be careful not to rush this through without considering its effects on cases like X. To which Stas says, Of course not. Don't use it in case X. I must say, however, that buried in Stas' use case, 3rd party, not-so-clean code, and realistic pragmatic installs thereof, is buried the rather large problem: Many people who do such a thing, do not really do a serious code-review of the 3rd party code. They toss it on there, it works, and they move on to the next task. I foresee a fair number of people who turn on error_mask, and then are defuddled by the 3rd party apps they never reviewed in the first place not behaving. Certainly, they are getting what they deserve, in some lights. But, to remain pragmatic, the PHP Dev Team has to be ready for an onslaught of issues and bug reports about 3rd party software in this state. As an example, before this feature rolls out, I'd have to suggest that the bug reporting page include it in things to turn OFF before a bug report is accepted, in that section about 3rd party extensions. This is just one example off the top of my head. I'm pretty sure there are more ramifications that go well beyond the code-based discussion so far. All that said: I am in favor of this patch, provided sufficient effort to be sure the community knows, in advance, loud and clear, it's not a cure-all and is meant only for code that can't/won't be fixed, rather than code that is in active development. In fact, I'd suggest that using error_mask should emit an E_STRICT, *not* suppressed by error_mask, before it kicks in. :-) -- Some people ask for gifts here. I just want you to buy an Indie CD for yourself: http://cdbaby.com/search/from/lynch -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 28.08.2009, at 22:47, Richard Lynch wrote: I must say, however, that buried in Stas' use case, 3rd party, not-so-clean code, and realistic pragmatic installs thereof, is buried the rather large problem: Many people who do such a thing, do not really do a serious code-review of the 3rd party code. Right and even so .. why should using that 3rd party code make it sensible to flip this switch on everything? I am not really suggesting the following because I havent thought it out .. just an example of out of the box thinking we need in order to try and really address the root causes. Maybe we rather need some way to define error handling on a per directory basis. This way you could just mark this 3rd party lib to use a different error reporting mode. Maybe this could be expanded even more to also cover the ever increasing number of extensions that optionally throw exceptions. This way we might even be able to reconcile the issue that if you run PDO and one lib wants exception mode and the other doesnt, you need two PDO instances. Another, again not fully thought out approach, would be to do something along the lines of exceptions. A magic object which can carry some more context but still returns true on a === false check. This object would of course have to be super lightweight and it should just contain the necessary bits in order to be able to generate the full error message if needed. I guess that would still probably not solve the performance issue that Stas is trying to solve, since its hard to imagine that such a solution would not come with so much overhead that it speed up anything. However it might provide for a better solution to deal with providing error context information without forcing everybody to deal with try/catch. Anyways, so yeah I am squarely in the camp that believes that adding more tools to just forget about error reporting is bad mojo. I also very much agree that many functions do not use the right reporting level and that several functions could probably be made to make it easier to avoid those reports to begin with, though yes there are cases where there is simply no way to avoid them. regards, Lukas Kahwe Smith m...@pooteeweet.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hi! I foresee a fair number of people who turn on error_mask, and then are defuddled by the 3rd party apps they never reviewed in the first place not behaving. /.../ All that said: I am in favor of this patch, provided sufficient effort to be sure the community knows, in advance, loud and clear, it's not a cure-all and is meant only for code that can't/won't be fixed, rather than code that is in active development. As I already noted, the masking - in most cases and definitely in recommended cases - would happen for errors that are NOT SEEN. Not reported. Not logged. Before the patch. Which means, whatever advantage you seek from looking at these errors, fixing them, reviewing the code, doing anything related to them at all, etc., etc. - you have ALREADY lost if when you decided not to report these errors. Without the patch. Before the patch. So all arguments about how wrong it is to disable errors when errors in fact might be useful are, again, irrelevant - they are already disabled without the patch. So far, the argument that made the most sense on this topic is that using this patch would taint you with bad mojo, I guess because when you sacrifice some performance to the Gods of Unreported Errors, it's all OK, but without that sacrifice, they could become enraged and revenge you by... I don't know, giving you more bad mojo? -- Stanislav Malyshev, Zend Software Architect s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 28.08.2009, at 23:09, Stanislav Malyshev wrote: As I already noted, the masking - in most cases and definitely in recommended cases - would happen for errors that are NOT SEEN. Not reported. Not logged. Before the patch. Which means, whatever advantage you seek from looking at these errors, fixing them, reviewing the code, doing anything related to them at all, etc., etc. - you have ALREADY lost if when you decided not to report these errors. Without the patch. Before the patch. So all arguments about how wrong it is to disable errors when errors in fact might be useful are, again, irrelevant - they are already disabled without the patch. So far, the argument that made the most sense on this topic is that using this patch would taint you with bad mojo, I guess because when you sacrifice some performance to the Gods of Unreported Errors, it's all OK, but without that sacrifice, they could become enraged and revenge you by... I don't know, giving you more bad mojo? Of course we are well aware that you can choose to ignore errors even today. However instead of adding yet another ini setting, some of us feel we should rather focus on solving the real issues. That certain errors in certain parts of your code are unavoidable and known and that certain pieces of code will raise errors to the global error handler even though you have all the code in place to handle the issue locally. regards, Lukas Kahwe Smith m...@pooteeweet.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Lukas Kahwe Smith wrote: On 28.08.2009, at 23:09, Stanislav Malyshev wrote: As I already noted, the masking - in most cases and definitely in recommended cases - would happen for errors that are NOT SEEN. Not reported. Not logged. Before the patch. Which means, whatever advantage you seek from looking at these errors, fixing them, reviewing the code, doing anything related to them at all, etc., etc. - you have ALREADY lost if when you decided not to report these errors. Without the patch. Before the patch. So all arguments about how wrong it is to disable errors when errors in fact might be useful are, again, irrelevant - they are already disabled without the patch. So far, the argument that made the most sense on this topic is that using this patch would taint you with bad mojo, I guess because when you sacrifice some performance to the Gods of Unreported Errors, it's all OK, but without that sacrifice, they could become enraged and revenge you by... I don't know, giving you more bad mojo? Of course we are well aware that you can choose to ignore errors even today. However instead of adding yet another ini setting, some of us feel we should rather focus on solving the real issues. That certain errors in certain parts of your code are unavoidable and known and that certain pieces of code will raise errors to the global error handler even though you have all the code in place to handle the issue locally. But that isn't actually the real issue. There will always be a reason to not report certain types of errors. E_STRICT and E_DEPRECATED are informative development-level messages that have no place on a production system. They should be turned off. The actual real problem is that our error mechanism only allows us to turn off the final output stage, but not the actual generation of those errors. That is the real issue. I do agree that having another ini switch isn't ideal, but I have pondered this a few times and I don't see a solution that doesn't involve some sort of configuration like that because of custom error handlers and the php_errormsg issues. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On Fri, August 28, 2009 4:09 pm, Stanislav Malyshev wrote: Hi! I foresee a fair number of people who turn on error_mask, and then are defuddled by the 3rd party apps they never reviewed in the first place not behaving. /.../ All that said: I am in favor of this patch, provided sufficient effort to be sure the community knows, in advance, loud and clear, it's not a cure-all and is meant only for code that can't/won't be fixed, rather than code that is in active development. As I already noted, the masking - in most cases and definitely in recommended cases - would happen for errors that are NOT SEEN. Not reported. Not logged. Before the patch. Which means, whatever advantage you seek from looking at these errors, fixing them, reviewing the code, doing anything related to them at all, etc., etc. - you have ALREADY lost if when you decided not to report these errors. Without the patch. Before the patch. So all arguments about how wrong it is to disable errors when errors in fact might be useful are, again, irrelevant - they are already disabled without the patch. I understand, really I do... And I know you understand, because you've repeated it. A lot. :-) I'm saying you have a big EDUCATION of community issue on your hands before releasing this feature, because if this many folks on @internals aren't getting it, and are making senseless knee-jerk arguments... Then you have millions of developers, and I use the term loosely, that will slaver over 300% performance boost and turn it on without having the faintest idea what it does... BREAKING the code that relies on custom error handlers, or that check $php_errormsg Those are the only sticking points for me. Let me give you a real-world pragmatic scenario: If I released good code with a custom error handler that gracefully and sensibly handled errors even if somebody was dumb enough to turn off error-reporting, and your patch breaks that, and then I get a zillion bug reports, I'm going to be a bit unhappy, and justifiably so. I think there more than a few developers who fit the above scenario, and the only way to avoid it is to be sure end users who install all kinds of crap next to my fabulous software [tongue firmly in cheek] and then use your new feature to shut up the bad software... I don't want to have to deal with a thousand bug reports because your patch encouraged the naive user to disable my error handling. Hope that clarifies the issue I'm forseeing... Again, I actually think you've made a good enough case for this. I just think your patch needs a lot of non-code changes. Better than average documentation. Changes to bug-reporting page. Comments in php.ini that clearly state that turning it on is BAD in general. . . . -- Some people ask for gifts here. I just want you to buy an Indie CD for yourself: http://cdbaby.com/search/from/lynch -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hi! Of course we are well aware that you can choose to ignore errors even today. However instead of adding yet another ini setting, some of us feel we should rather focus on solving the real issues. That certain This is a real issue. You have better solution for it? Go ahead, propose it. So far best I saw is useless discussion about somehow fixing all functions not to report errors and even more useless discussions about moving to exceptions, etc. And the bad mojo stuff, of course. -- Stanislav Malyshev, Zend Software Architect s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hi!
@ is often used to stop error/warning output to the browser on that line,
but the next couple lines of code are used to handle that error. For
example:
if (!($dom = @DOMDocument::load($file_name))) {
log_it('invalid XML: ' . $php_errormsg);
die('invalid XML');
}
So if error processing is totally turned off, $php_errormsg won't be
populated.
That's true. So, if you use code that uses $php_errormsg, of course you
can not use this optimization and should not enable it (at least for
error types and code parts that you use $php_errormsg with).
Also, if you use @ to stop warning output to the browser you should read
the manual about display_errors and part of the security guidelines when
it says never enable display_errors in production ;)
--
Stanislav Malyshev, Zend Software Architect
s...@zend.com http://www.zend.com/
(408)253-8829 MSN: s...@zend.com
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 08/25/2009 12:42 AM, Stanislav Malyshev wrote: 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. Just wondering why nobody hasn't suggested the obvious (?) alternative yet: Why not fix error_reporting to work like one expects. As in: If an error level isn't in it, then nothing happens. Adding an extra option to do that seems a bit overkill.. --Jani -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hi! Just wondering why nobody hasn't suggested the obvious (?) alternative yet: Why not fix error_reporting to work like one expects. As in: If an error level isn't in it, then nothing happens. Adding an extra option to do that seems a bit overkill.. Because it would break other funcions (namely, $php_errormsg, error handlers, etc.) which may be used by some. -- Stanislav Malyshev, Zend Software Architect s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 08/25/2009 10:35 AM, Stanislav Malyshev wrote: Hi! Just wondering why nobody hasn't suggested the obvious (?) alternative yet: Why not fix error_reporting to work like one expects. As in: If an error level isn't in it, then nothing happens. Adding an extra option to do that seems a bit overkill.. Because it would break other funcions (namely, $php_errormsg, error handlers, etc.) which may be used by some. Well, not necessarily. How doesn't your patch break them? Just do the same but without adding new option. --Jani -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
2009/8/24 Rasmus Lerdorf ras...@lerdorf.com: 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 As an outsider, and from what I've read here, surely the solution is to simply move the test to determine if an error type is reportable. If anything, as you _ARE_ all saying that there is a slowdown, I should maybe file a bug ... My @ridden code is really slow, even when I turn off error_reporting! So, as an outsider, I'm +1 for speeding up PHP by not running unnecessary code, but -1 for introducing what is surely quite obviously an unnecessary ini entry. speech style=iInspirationalEssentially, it's a bug folks! So let's fix it!/speech Regards, Richard. -- - Richard Quadling Standing on the shoulders of some very clever giants! EE : http://www.experts-exchange.com/M_248814.html Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498r=213474731 ZOPA : http://uk.zopa.com/member/RQuadling -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hi, I think the idea is good intended but will cause troubles of introduced in 5.3. I alone have a bunch of code with custom error handlers that expect to receive all errors at all times. If the feature allows runtime tweaking via ini_set/get, then one could add more plumbing code and workaround, but it still doesn't make it BC to the expected behavior for existing 5.x apps. However I fully agree E_NOTICE should: 1) be reserved for informational may-be-bad-but-usually-is-ok messages 2) not have performance impact when disabled (i.e. production) The problem? Right now important errors that could cause data damage if ignored are E_NOTICE instead of E_WARNING. I'm talking E_NOTICE when you using variables that don't exist, missing constants and so on. All of this should be E_WARNING so my production code can stop and notify me when it's encountered, then I wouldn't mind filtering E_NOTICE. Also Stas, consider why are you trying to treat the symptom of unwanted errors slowing things down. I would bet it's something like the stream APIs emitting errors on 404 response and such. Some API-s really throw notices and warnings where it makes no sense, so people mute them out of necessity (or use handlers). I'd rather fix the API-s instead of pretend it doesn't happen by masking the errors. All of the above would be a job for 6.0 IMHO, 5.x has pulled enough changes under our feet and it's really not fun anymore :P Regards, Stan Vassilev -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On Mon, 24 Aug 2009, Rasmus Lerdorf wrote: Lukas Kahwe Smith wrote: 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. And how exactly is that a problem? Sure, there are some cases where PHP functions are too noisy, but that should be addressed instead. regards, Derick -- http://derickrethans.nl | http://ezcomponents.org | http://xdebug.org twitter: @derickr -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Derick Rethans wrote: On Mon, 24 Aug 2009, Rasmus Lerdorf 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. And how exactly is that a problem? Sure, there are some cases where PHP functions are too noisy, but that should be addressed instead. You are making the assumption that everybody agrees on E_NOTICE, E_STRICT and E_DEPRECATED being things to be fixed in your code. That is not the case and punishing people for not following exactly the same coding style as you doesn't make sense when it could easily be avoided. Background info: We decided to patch PHP to not generate E_NOTICE for undefined variables or (worse) array offsets because we use this language feature a lot and cannot afford the overhead caused by E_NOTICEs even if error_reporting is set to ignore E_NOTICE. And you won't be able to convince us to change this policy because being able to do things like if (!$visited[$city]++) is more valuable (i.e. keeps the code uncluttered) to us than the E_NOTICE. We use http://cschneid.com/php/phplint.php (in a commit hook) instead to catch typos. It's not perfect but does the job for us. Summary: Don't punish different coding styles unless really necessary. Cheers, - Chris -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hey Stas: On Mon, Aug 24, 2009 at 11:43:22PM -0700, Stanislav Malyshev wrote: That's true. So, if you use code that uses $php_errormsg, of course you can not use this optimization and should not enable it (at least for error types and code parts that you use $php_errormsg with). Exactly. Totally killing E_STRICT on it's own seems like the biggest win (in the right circumstances). My main point is that we need to think this thing through carefully and document it well. Also, if you use @ to stop warning output to the browser you should read the manual about display_errors and part of the security guidelines when it says never enable display_errors in production ;) Of course. But folks don't want those same messages showing up in the error log, either. Thanks, --Dan -- T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y data intensive web and database programming http://www.AnalysisAndSolutions.com/ 4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hello Jani: On Tue, Aug 25, 2009 at 11:11:19AM +0300, Jani Taskinen wrote: On 08/25/2009 10:35 AM, Stanislav Malyshev wrote: Because it would break other funcions (namely, $php_errormsg, error handlers, etc.) which may be used by some. Well, not necessarily. How doesn't your patch break them? Just do the same but without adding new option. The patch allows setting reporting to E_ALL while using the mask to kill just E_STRICT. --Dan -- T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y data intensive web and database programming http://www.AnalysisAndSolutions.com/ 4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Daniel Convissor wrote: Hello Jani: On Tue, Aug 25, 2009 at 11:11:19AM +0300, Jani Taskinen wrote: On 08/25/2009 10:35 AM, Stanislav Malyshev wrote: Because it would break other funcions (namely, $php_errormsg, error handlers, etc.) which may be used by some. Well, not necessarily. How doesn't your patch break them? Just do the same but without adding new option. The patch allows setting reporting to E_ALL while using the mask to kill just E_STRICT. Sorry, I just don't get it. If the mask thing kills some level..what's the point in enabling it in the first place? And IIRC, E_STRICT is not part of E_ALL.. --Jani -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hi! Sorry, I just don't get it. If the mask thing kills some level..what's the point in enabling it in the first place? And IIRC, E_STRICT is not part of E_ALL.. There's no point in enabling it. The point is that now PHP generates full set of data even for DISABLED errors. And spends time/memory on that. -- Stanislav Malyshev, Zend Software Architect s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Stas,
While the patch is certainly interesting, I think masking errors is a
really really bad idea.
On 24-Aug-09, at 2:17 PM, 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?
--
Stanislav Malyshev, Zend Software Architect
s...@zend.com http://www.zend.com/
(408)253-8829 MSN: s...@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(params, 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 */
/*
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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. On 24-Aug-09, at 2:30 PM, Stanislav Malyshev wrote: 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 s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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 s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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 s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 24-Aug-09, at 3:30 PM, Stanislav Malyshev wrote: 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? Because it may force to fix the errors, rather then ignore them? 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 @ 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. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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 s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 24-Aug-09, at 3:59 PM, Stanislav Malyshev wrote: 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. 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. 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. 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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 s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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! 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. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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: 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. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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 s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 24.08.2009, at 23:42, Stanislav Malyshev wrote: 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. 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 m...@pooteeweet.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Lukas Kahwe Smith wrote: On 24.08.2009, at 23:42, Stanislav Malyshev wrote: 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. 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
hi Lukas, On Mon, Aug 24, 2009 at 11:51 PM, Lukas Kahwe Smithm...@pooteeweet.org wrote: On 24.08.2009, at 23:42, Stanislav Malyshev wrote: 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. 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 24.08.2009, at 23:59, Rasmus Lerdorf wrote: Lukas Kahwe Smith wrote: On 24.08.2009, at 23:42, Stanislav Malyshev wrote: 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. 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. 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 m...@pooteeweet.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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
s...@zend.com http://www.zend.com/
(408)253-8829 MSN: s...@zend.com
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
On 25.08.2009, at 00:12, Stanislav Malyshev wrote:
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.
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
m...@pooteeweet.org
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
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*(). ;-) On 8/25/09, Stanislav Malyshev s...@zend.com wrote: 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 s...@zend.com http://www.zend.com/ (408)253-8829 MSN: s...@zend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- -- Tjerk -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [patch] error masks
Hi Stas:
On Mon, Aug 24, 2009 at 12:59:55PM -0700, Stanislav Malyshev wrote:
Just search for @fopen or @include or
@xml or @eval - tons of examples.
True. But this patch could cause problems.
@ is often used to stop error/warning output to the browser on that line,
but the next couple lines of code are used to handle that error. For
example:
if (!($dom = @DOMDocument::load($file_name))) {
log_it('invalid XML: ' . $php_errormsg);
die('invalid XML');
}
So if error processing is totally turned off, $php_errormsg won't be
populated.
I like the general idea, but I'm encouraging folks to carefully consider
the ramifications of various implementation methods.
Thanks,
--Dan
--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
