Everything

php

Exceptions and a real example: Tidy

php.internals

Nuno Lopes

22 years ago

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

John Coggeshall

22 years ago

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 to E_NOTICE or promoted to E_ERROR as
necessary
- Those errors which are truly E_WARNING will 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


On Thu, 2004-04-15 at 08:00, Nuno Lopes wrote:

> 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

--
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-
John Coggeshall                   http://www.coggeshall.org/
The PHP Developer's Handbook    http://www.php-handbook.com/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-

Sterling Hughes

22 years ago

On Apr 15, 2004, at 4:12 PM, John Coggeshall wrote:

> 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 to E_NOTICE or promoted to E_ERROR as
>     necessary
>   - Those errors which are truly E_WARNING will 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

John Coggeshall

22 years ago

On Thu, 2004-04-15 at 20:07, Sterling Hughes wrote:

> Also wrong.  You never through an E_ERROR for 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/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-

Derick Rethans

22 years ago

On Thu, 15 Apr 2004, John Coggeshall wrote:

>   - All errors were re-evaluated, and those (such as a bogus config
>     option) were demoted to E_NOTICE or promoted to E_ERROR as
>     necessary
>   - Those errors which are truly E_WARNING will 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

John Coggeshall

22 years ago

On Fri, 2004-04-16 at 03:32, Derick Rethans wrote:

> 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/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-

Christian Schneider

22 years ago

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

John Coggeshall

22 years ago

On Fri, 2004-04-16 at 12:01, Christian Schneider wrote:

> 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.

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/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-

Christian Schneider

22 years ago

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

Derick Rethans

22 years ago

On Fri, 16 Apr 2004, Christian Schneider wrote:

> 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

John Coggeshall

22 years ago

On Fri, 2004-04-16 at 12:44, Derick Rethans wrote:

> 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).

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/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-

Christian Schneider

22 years ago

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

Sterling Hughes

22 years ago

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


On Apr 16, 2004, at 9:44 AM, Derick Rethans wrote:

Derick Rethans

22 years ago

On Fri, 16 Apr 2004, Sterling Hughes wrote:

> 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..?

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

Andi Gutmans

22 years ago

John,

I agree that currently it's better to change these back to real E_WARNINGS.
I agree that exceptions should be thrown for things which we'd usually make
E_ERRORs (if recoverable).
As most people here prefer not to go the route of exceptions, and I think
many (not all) of their reasoning makes sense we should continue in the
current direction of PHP (except for special cases such as
SoapServer::handle() for example).
I think the ultimate solution will be to think of a way (possibly wrappers)
to give the users a choice. For certain projects I do see great value in
exceptions, but I don't think the average PHP user should be forced to work
with them (especially as it's not an easy subject to implement correctly).

Andi

At 03:20 PM 4/16/2004 -0700, Sterling Hughes wrote:

John Coggeshall

22 years ago

I'll take care of changing everything to E_WARNING tomorrow.

John

On Fri, 2004-04-16 at 18:27, Andi Gutmans wrote:

> John,
>
> I agree that currently it's better to change these back to real E_WARNINGS.
> I agree that exceptions should be thrown for things which we'd usually make
> E_ERRORs (if recoverable).
> As most people here prefer not to go the route of exceptions, and I think
> many (not all) of their reasoning makes sense we should continue in the
> current direction of PHP (except for special cases such as
> SoapServer::handle() for example).
> I think the ultimate solution will be to think of a way (possibly wrappers)
> to give the users a choice. For certain projects I do see great value in
> exceptions, but I don't think the average PHP user should be forced to work
> with them (especially as it's not an easy subject to implement correctly).
>
> Andi
>
> At 03:20 PM 4/16/2004 -0700, Sterling Hughes wrote:
> >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
> >
> >
> >On Apr 16, 2004, at 9:44 AM, Derick Rethans wrote:
> >
> >>On Fri, 16 Apr 2004, Christian Schneider wrote:
> >>
> >>>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
> >>
> >>--
> >>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

--
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-
John Coggeshall                   http://www.coggeshall.org/
The PHP Developer's Handbook    http://www.php-handbook.com/
-=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=--=~=-