Hello,
I've followed the war, sorry, discussion about exceptions.
Now, let me introduce some problems I've found in Tidy.
Look at the code:
<?
//doesn't echo any error, but should!
//should generate a E_WARNING because it can't find the file
$tidy = tidy_parse_string('sdgdsg', 'BogusConfig.file');
/*********************************************************/
//throw exception instead of E_WARNING
try {
$tidy = tidy_parse_string('sdgdsg', array('bogusconf' => 'bogusvalue'));
//because of throwing the exception, this function is never executed
//thus making it complitely unusefull.
echo tidy_config_count($tidy);
}
catch (tidy_exception $e) {
echo $e;
}
/*********************************************************/
//an exception here. why? a BUG?!?!?!
$tidy = new tidy();
$tidy->ParseString('test');
?>
These are just some examples. I don't hate exceptions, but whe they are
misused...
John said that Tidy should only generate exceptions with OO code, but as you
can see in the above code, it generate exceptions with non-OO code.
Nuno
Attached is a patch which I hope will keep people happy when it comes to
specifically the Tidy extension. I'd like some feedback on this before I
commit it / throw it away:
Changes:
- All errors were re-evaluated, and those (such as a bogus config
option) were demoted toE_NOTICEor promoted toE_ERRORas
necessary - Those errors which are truly
E_WARNINGwill behave as such when
called from a procedural context. If called from an object oriented
context, they will be represented as exceptions.
I also looked at the bugs you reported, Nuno but I couldn't reproduce
some of them. In either case, the ones i could reproduced should be
fixed.
Feedback welcome.
John
Hello,
I've followed the war, sorry, discussion about exceptions.
Now, let me introduce some problems I've found in Tidy.Look at the code:
<?//doesn't echo any error, but should!
//should generate aE_WARNINGbecause it can't find the file
$tidy = tidy_parse_string('sdgdsg', 'BogusConfig.file');/*********************************************************/
//throw exception instead of
E_WARNING
try {
$tidy = tidy_parse_string('sdgdsg', array('bogusconf' => 'bogusvalue'));//because of throwing the exception, this function is never executed
//thus making it complitely unusefull.
echo tidy_config_count($tidy);
}
catch (tidy_exception $e) {
echo $e;
}/*********************************************************/
//an exception here. why? a BUG?!?!?!
$tidy = new tidy();
$tidy->ParseString('test');?>
These are just some examples. I don't hate exceptions, but whe they are
misused...
John said that Tidy should only generate exceptions with OO code, but as you
can see in the above code, it generate exceptions with non-OO code.Nuno
--
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-
John Coggeshall http://www.coggeshall.org/
The PHP Developer's Handbook http://www.php-handbook.com/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=
Attached is a patch which I hope will keep people happy when it comes
to
specifically the Tidy extension. I'd like some feedback on this before
I
commit it / throw it away:Changes:
- All errors were re-evaluated, and those (such as a bogus config
option) were demoted toE_NOTICEor promoted toE_ERRORas
necessary- Those errors which are truly
E_WARNINGwill behave as such when
called from a procedural context. If called from an object oriented
context, they will be represented as exceptions.I also looked at the bugs you reported, Nuno but I couldn't reproduce
some of them. In either case, the ones i could reproduced should be
fixed.
-
TIDY_THROW("Could not retrieve key from option array");
-
zend_error(E_ERROR, "Could not retrieve key from option array");
Also wrong. You never through an E_ERROR for this sort of thing.
-sterling
Also wrong. You never through an
E_ERRORfor this sort of thing.
Ilia pointed that out, its been corrected -- all zend_error() references
were also changed to php_error_docref() (except in RINIT)
John
--
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-
John Coggeshall http://www.coggeshall.org/
The PHP Developer's Handbook http://www.php-handbook.com/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=
- All errors were re-evaluated, and those (such as a bogus config
option) were demoted toE_NOTICEor promoted toE_ERRORas
necessary- Those errors which are truly
E_WARNINGwill behave as such when
called from a procedural context. If called from an object oriented
context, they will be represented as exceptions.
Do you mean E_ERRORS become exceptions or also E_WARNINGS? E_WARNINGS
should never become exceptions as it's a non-fatal error.
Derick
Do you mean E_ERRORS become exceptions or also E_WARNINGS? E_WARNINGS
should never become exceptions as it's a non-fatal error.
E_WARNINGs are exceptions. If you look at the code with the patch
applied, I've downgraded all truly minor error conditions to E_NOTICE
and made E_WARNINGs errors which are actually problems (i.e. I couldn't
find this file), but recoverable. Alan pointed out that it'd be nice if
exceptions didn't necessary stop the script if it wasn't caught -- but
considering that isn't available at this time this is the best solution.
In 5.1, if we have exceptions which are not terminating if uncaught,
I'll upgrade a few of the thrown exceptions in Tidy to them.
I'll commit my changes later today with the suggestions made by Ilia and
others barring any strong arguments against the solution.
John
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-
John Coggeshall http://www.coggeshall.org/
The PHP Developer's Handbook http://www.php-handbook.com/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=
John Coggeshall wrote:
E_WARNINGs are exceptions.
I think this is wrong.
For E_WARNING the program flow continues unchanged whether you handle
the return value of e.g. fopen or not.
For exceptions you have to handle it, otherwise your program aborts.
Two different things.
- Chris
I think this is wrong.
ForE_WARNINGthe program flow continues unchanged whether you handle
the return value of e.g. fopen or not.
For exceptions you have to handle it, otherwise your program aborts.
Two different things.
I agree, however ZE2 does not provide non-fatal exceptions so I consider
this an engine limitation rather than a implementation error. This is
the best compromise I can reach without discarding exceptions entirely,
which I believe is even more wrong for OO code.
John
--
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-
John Coggeshall http://www.coggeshall.org/
The PHP Developer's Handbook http://www.php-handbook.com/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=
John Coggeshall wrote:
the best compromise I can reach without discarding exceptions entirely,
which I believe is even more wrong for OO code.
I disagree. I lost track over the last couple of days, what is everyone
else's view on this?
- Chris
John Coggeshall wrote:
the best compromise I can reach without discarding exceptions entirely,
which I believe is even more wrong for OO code.I disagree. I lost track over the last couple of days, what is everyone
else's view on this?
I disgree with this behavior too. E_WARNINGs were never supposed to
abort a script, that's what we have E_ERRORs for. (Making E_ERROR an
exception in an OO context is fine, as when it's unhandled it should
abort the script, just like in PHP 4 and all other non-oo extensions).
Derick
I disgree with this behavior too. E_WARNINGs were never supposed to
abort a script, that's what we have E_ERRORs for. (MakingE_ERRORan
exception in an OO context is fine, as when it's unhandled it should
abort the script, just like in PHP 4 and all other non-oo extensions).
The problem here is that the errors in question truly are warnings, not
show-stopping errors. Unless the engine can support a non-fatal
exception, i see no reasonable solution short of completely removing
Exceptions from use. I'm not prepared to do that, a failure to open a
file is an exception in OO-world and that's what should be there.
John
--
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-
John Coggeshall http://www.coggeshall.org/
The PHP Developer's Handbook http://www.php-handbook.com/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=
John Coggeshall wrote:
Exceptions from use. I'm not prepared to do that, a failure to open a
file is an exception in OO-world and that's what should be there.
No! OO and exceptions are two completely different things. One is about
data encapsulation and one is about error handling. OO can happily live
without exceptions and exceptions can exist without OO!
They are not an integral part.
- Chris
Tidy's current error handling scheme is totally messed up - every
single thing in the extension should be an E_WARNING by PHP standards.
Its RC2, and this stuff has worked for a long time, breaking it now is
counterproductive and annoying.
John, if you insist on messing up the error handling in the tidy
extension, which is a serious thing, why not move it to pecl where you
have full control over such things without bothering the rest of the
release..?
-Sterling
John Coggeshall wrote:
the best compromise I can reach without discarding exceptions
entirely,
which I believe is even more wrong for OO code.I disagree. I lost track over the last couple of days, what is
everyone
else's view on this?I disgree with this behavior too. E_WARNINGs were never supposed to
abort a script, that's what we have E_ERRORs for. (MakingE_ERRORan
exception in an OO context is fine, as when it's unhandled it should
abort the script, just like in PHP 4 and all other non-oo extensions).Derick
Tidy's current error handling scheme is totally messed up - every
single thing in the extension should be anE_WARNINGby PHP standards.
Its RC2, and this stuff has worked for a long time, breaking it now is
counterproductive and annoying.John, if you insist on messing up the error handling in the tidy
extension, which is a serious thing, why not move it to pecl where you
have full control over such things without bothering the rest of the
release..?
I have to agree with this. Although it might be useful (to some people),
messing with it all the time warrants a nice place in pecl.
Derick