PHP 5.3.8 Released!

13 years ago by johannes@php.net — view source

unread

The PHP development team would like to announce the immediate
availability of PHP 5.3.8. This release fixes two issues introduced in
the PHP 5.3.7 release:

* Fixed bug #55439 (crypt() returns only the salt for MD5)
* Reverted a change in timeout handling restoring PHP 5.3.6
  behavior, which caused mysqlnd SSL connections to hang (Bug
  #55283).

All PHP users should note that the PHP 5.2 series is NOT supported
anymore. All users are strongly encouraged to upgrade to PHP 5.3.8.

For source downloads please visit our downloads page at
http://php.net/downloads.php, Windows binaries can be found on
http://windows.php.net/download/.

Johannes Schlüter
PHP 5.3 Release Master

13 years ago by alan@akbkhome.com — view source

unread

It might have been better to have waited for the is_a() fix to get
sorted out.. - It's a very annoying BC break (changes the documented
behaviour), and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

Regards
Alan

The PHP development team would like to announce the immediate
availability of PHP 5.3.8. This release fixes two issues introduced in
the PHP 5.3.7 release:
 * Fixed bug #55439 (crypt() returns only the salt for MD5)
 * Reverted a change in timeout handling restoring PHP 5.3.6
   behavior, which caused mysqlnd SSL connections to hang (Bug
   #55283).
All PHP users should note that the PHP 5.2 series is NOT supported
anymore. All users are strongly encouraged to upgrade to PHP 5.3.8.

For source downloads please visit our downloads page at
http://php.net/downloads.php, Windows binaries can be found on
http://windows.php.net/download/.

Johannes Schlüter
PHP 5.3 Release Master

13 years ago by Ferenc Kovacs — view source

unread

It might have been better to have waited for the is_a() fix to get sorted
out.. - It's a very annoying BC break (changes the documented behaviour),
and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current behavior
is correct, the old was a bug:

as Stas pointed out:
"Not a bug. $var is interpreted as a class name. To know if one class
extends another, the class in question (first one) should be loaded.
There's no need to load the second one since if it's unknown nothing
can be instance of it and existing classes can not extend it."
if you used this previously

from Dmitry:
"Before the patch, is_a() didn't accept string as the first argument
at all, so it always returned "false" and never triggered
__autoload(). The proposed patch didn't revert to original behavior.
It just disables autoloading and may lead to wrong result.

class a {}
class b extends a {}
var_dump(is_a("b", "a")); // it was false before 5.3.7, now it's true

I would say that the old behaviour was wrong, especially because
"instanceof" and is_subclass_of() already implemented support for
string arguments."

so your example was bogus, as passing a non-object as a first
parameter wasn't supported (see http://php.net/is_a) so your code
example depends on an undefined behavior and results in a bogus result
(is_a() alwas returned false if you passed a non-object)

so in a way it is really a BC, but I think that this change is really a bugfix.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Derick Rethans — view source

unread

It might have been better to have waited for the is_a() fix to get sorted
out.. - It's a very annoying BC break (changes the documented behaviour),
and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current behavior
is correct, the old was a bug:

It is also a BC break! Yes, it should be fixed, but only in 5.4. This
kind of little changes is what makes people look at PHP as a joke!

Derick

--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug

13 years ago by Ferenc Kovacs — view source

unread

It might have been better to have waited for the is_a() fix to get sorted
out.. - It's a very annoying BC break (changes the documented behaviour),
and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current behavior
is correct, the old was a bug:

It is also a BC break! Yes, it should be fixed, but only in 5.4. This
kind of little changes is what makes people look at PHP as a joke!

agree with keeping the BC as much as possible, but from the responses
from the others (Dmitry, Stas, Zeev) and the nature of the change
(changing the undefined behavior of the function) and the fact that it
is already out there (and changing this back and releasing a new
version now or later would be even more wrong) my guess is that we
will keep the new behavior.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Pierre Joye — view source

unread

agreed.

It might have been better to have waited for the is_a() fix to get sorted
out.. - It's a very annoying BC break (changes the documented behaviour),
and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current behavior
is correct, the old was a bug:

It is also a BC break! Yes, it should be fixed, but only in 5.4. This
kind of little changes is what makes people look at PHP as a joke!

Derick

--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug

--

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Ferenc Kovacs — view source

unread

agreed.

about this specific change:
the real problem is that nobody spotted this change until we rolled
out the stable release, after that we can only chose from bad options.
we could have spotted this via two ways:

those who participated in fixing
https://bugs.php.net/bug.php?id=53727 could have spotted this
our tests should have start failing after the change

AFAIK non of that happened, and fixing the first is hard, so I would
propose fixing the second (I know, it is already agreed and worked
toward)

another problem is that the current(and before the change also) proto
was not forced by the function, so if is_a() would raise warnings when
non-object is passed to the first argument, this BC break would have
much less impact.

so I got the idea that it would be usefull, if somebody could write a
script which fetches the proto for the functions/methods and does some
fuzzing which checks if the protos are enforced.
after that, we would have a list, and probably we could fix those
(changing the proto and the docs to the real behaviour, or vica
versa), and we could see whether the fix would impy BC and for those
cases we could just keep it as is for the stable branch and create
tests which would guarantee that changing the current behavior in the
stable branch wouldn't go unnoticed.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Pierre Joye — view source

unread

the problem is to change existing tests to match a "possible fix",
that defeats the whole purpose of testing possible BC breaks using our
test suites.

We should definitively run x.y.0 tests during the whole x.y.0 life,
and if something has to be changed, then it should be 1st be
discussed, or RFC for big changes. And then it should indeed be
documented as well, in the UPGRADING guide (should be done anyway for
x.y+1 versions too).

agreed.

about this specific change:
the real problem is that nobody spotted this change until we rolled
out the stable release, after that we can only chose from bad options.
we could have spotted this via two ways:

those who participated in fixing
https://bugs.php.net/bug.php?id=53727 could have spotted this

our tests should have start failing after the change

AFAIK non of that happened, and fixing the first is hard, so I would
propose fixing the second (I know, it is already agreed and worked
toward)

another problem is that the current(and before the change also) proto
was not forced by the function, so if is_a() would raise warnings when
non-object is passed to the first argument, this BC break would have
much less impact.

so I got the idea that it would be usefull, if somebody could write a
script which fetches the proto for the functions/methods and does some
fuzzing which checks if the protos are enforced.
after that, we would have a list, and probably we could fix those
(changing the proto and the docs to the real behaviour, or vica
versa), and we could see whether the fix would impy BC and for those
cases we could just keep it as is for the stable branch and create
tests which would guarantee that changing the current behavior in the
stable branch wouldn't go unnoticed.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by johannes@schlueters.de — view source

unread

we could have spotted this via two ways:

those who participated in fixing
https://bugs.php.net/bug.php?id=53727 could have spotted this

our tests should have start failing after the change

Third option:

RC testers might have spotted and reported it.

I have the impression that very few people actually test these. When
creating an RC we inform the "primary testers" as well as qa and
internals list members. From there I get one or two responses in
general.

When I approach PHP users I often get answers like installing PHP
without breaking their setup would be complicated (which is not the case
but maybe needs education?) and they won't have time. I try to use a
hypothetical case, like we have here in reality, to explain them why it
is beneficial for their business if it is detected early as then we can
fix it, fixing something after a release is hard. We also can try to
improve our tests but we will never be able to test each and every way
PHP is being used out there in the wild.

So how can we motivate people to test new versions during RC not the day
after it is being released?

We don't push them out as news on the php.net frontpage and we don't
send it out to the announce list for reasons like not confusing users.
Should we change that? Other ideas?

johannes

13 years ago by Dirk Haun — view source

unread

Quoting Johannes Schlüter johannes@schlueters.de:

I have the impression that very few people actually test these. When
creating an RC we inform the "primary testers" as well as qa and
internals list members. From there I get one or two responses in
general.

What happens to the reports that "make test" sends back to php.net?
There are always a few tests failing (I don't think I ever installed a
PHP version - final or not - that didn't have at least one or two
failing tests).

Where do those reports end up and is anyone actually looking at them?

bye, Dirk

13 years ago by Ferenc Kovacs — view source

unread

Quoting Johannes Schlüter johannes@schlueters.de:

I have the impression that very few people actually test these. When
creating an RC we inform the "primary testers" as well as qa and
internals list members. From there I get one or two responses in
general.

What happens to the reports that "make test" sends back to php.net? There
are always a few tests failing (I don't think I ever installed a PHP version

final or not - that didn't have at least one or two failing tests).

Where do those reports end up and is anyone actually looking at them?

they are sent to the qa reports mailing list: http://news.php.net/php.qa.reports
the aggregated report summaries are available at
http://qa.php.net/reports/ thanks to Oliver Doucet who make this
happen.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Ryan McCue — view source

unread

Ferenc Kovacs wrote:

the aggregated report summaries are available at
http://qa.php.net/reports/ thanks to Oliver Doucet who make this
happen.

Which, incidentally, doesn't appear to be working at this point in time.

Ryan McCue
<http://ryanmccue.info/

13 years ago by Ferenc Kovacs — view source

unread

Ferenc Kovacs wrote:

the aggregated report summaries are available at
http://qa.php.net/reports/ thanks to Oliver Doucet who make this
happen.

Which, incidentally, doesn't appear to be working at this point in time.

it was working when I sent the mailt.
it would be a weird coincidence...

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Dirk Haun — view source

unread

Quoting Ferenc Kovacs tyra3l@gmail.com:

Ferenc Kovacs wrote:

the aggregated report summaries are available at
http://qa.php.net/reports/ thanks to Oliver Doucet who make this
happen.

Which, incidentally, doesn't appear to be working at this point in time.

it was working when I sent the mailt.
it would be a weird coincidence...

Works for me.

However, it's still not clear (to me) what's happening to these
reports once they're up there. Is anyone looking at them?

bye, Dirk

13 years ago by Ferenc Kovacs — view source

unread

Quoting Ferenc Kovacs tyra3l@gmail.com:

Ferenc Kovacs wrote:

the aggregated report summaries are available at
http://qa.php.net/reports/ thanks to Oliver Doucet who make this
happen.

Which, incidentally, doesn't appear to be working at this point in time.

it was working when I sent the mailt.
it would be a weird coincidence...

Works for me.

However, it's still not clear (to me) what's happening to these reports once
they're up there. Is anyone looking at them?

it should be.
AFAIK the core devs mostly check our own build reports
(http://gcov.php.net/ and Pierres buildbot results at
http://windows.php.net/downloads/snaps/php-trunk ) only.
the QA team isn't really active as a group, hovewer half of the
members are active as developers (see the http://php.net/credits.php
Quality Assurance Team)

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Ferenc Kovacs — view source

unread

2011/8/24 Johannes Schlüter johannes@schlueters.de:

we could have spotted this via two ways:

those who participated in fixing
https://bugs.php.net/bug.php?id=53727 could have spotted this

our tests should have start failing after the change

Third option:

RC testers might have spotted and reported it.

I have the impression that very few people actually test these. When
creating an RC we inform the "primary testers" as well as qa and
internals list members. From there I get one or two responses in
general.

When I approach PHP users I often get answers like installing PHP
without breaking their setup would be complicated (which is not the case
but maybe needs education?) and they won't have time. I try to use a
hypothetical case, like we have here in reality, to explain them why it
is beneficial for their business if it is detected early as then we can
fix it, fixing something after a release is hard. We also can try to
improve our tests but we will never be able to test each and every way
PHP is being used out there in the wild.

So how can we motivate people to test new versions during RC not the day
after it is being released?

We don't push them out as news on the php.net frontpage and we don't
send it out to the announce list for reasons like not confusing users.
Should we change that? Other ideas?

johannes

agree, should have mentioned that.
I think that currently testing the RCs have a very high barrier.
usually they are going unnoticed for most people and compiling your
own version (with all the extensions that you need) can be really
cumbersome.

we need to get out the word to the masses (the current php.net site
simply lacks this, maybe the http://prototype.php.net/ will be better
in this regard), for which we also need to lower the barriers to
entry:
better documentation about how to build your own php version would
be a must, maybe phpfarm can be also useful for this
we should cooperate with the major php projects out there to run
their testsuites against the new releases or maybe even trunk, if I
remember correctly somebody mentioned that we already do this for some
projects (maybe Pierre mentioned this). this would be an easy way to
boost our test coverage and make the BC breaks more obvious.
having pre-packaged versions of php available would also help,
testing out the latest mysql versions are much more easy for example,
as I can just grab the Linux - Generic archive, extract it, and voila,
I can test it.
projects like http://apt.damz.org/ also help

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Alexey Shein — view source

unread

2011/8/24 Ferenc Kovacs tyra3l@gmail.com:

2011/8/24 Johannes Schlüter johannes@schlueters.de:

we could have spotted this via two ways:

those who participated in fixing
https://bugs.php.net/bug.php?id=53727 could have spotted this

our tests should have start failing after the change

Third option:

RC testers might have spotted and reported it.

I have the impression that very few people actually test these. When
creating an RC we inform the "primary testers" as well as qa and
internals list members. From there I get one or two responses in
general.

When I approach PHP users I often get answers like installing PHP
without breaking their setup would be complicated (which is not the case
but maybe needs education?) and they won't have time. I try to use a
hypothetical case, like we have here in reality, to explain them why it
is beneficial for their business if it is detected early as then we can
fix it, fixing something after a release is hard. We also can try to
improve our tests but we will never be able to test each and every way
PHP is being used out there in the wild.

So how can we motivate people to test new versions during RC not the day
after it is being released?

We don't push them out as news on the php.net frontpage and we don't
send it out to the announce list for reasons like not confusing users.
Should we change that? Other ideas?

johannes

agree, should have mentioned that.
I think that currently testing the RCs have a very high barrier.
usually they are going unnoticed for most people and compiling your
own version (with all the extensions that you need) can be really
cumbersome.

we need to get out the word to the masses (the current php.net site
simply lacks this, maybe the http://prototype.php.net/ will be better
in this regard), for which we also need to lower the barriers to
entry:

better documentation about how to build your own php version would
be a must, maybe phpfarm can be also useful for this

I would really glad to have a script that will be eager to enable as
much modules as it can find on my machine (plus enable debug flags as
--enable-zts-maintainer and such) and even offer to install some
missing dependencies from OS packages (I'm on ubuntu so it's easy to
do some apt-get/yum install if you know what are you for). For now you
have to read quite a long list of configure options and then install
their dependencies which are quite non-obvious to find out.

we should cooperate with the major php projects out there to run
their testsuites against the new releases or maybe even trunk, if I
remember correctly somebody mentioned that we already do this for some
projects (maybe Pierre mentioned this). this would be an easy way to
boost our test coverage and make the BC breaks more obvious.

having pre-packaged versions of php available would also help,
testing out the latest mysql versions are much more easy for example,
as I can just grab the Linux - Generic archive, extract it, and voila,
I can test it.

projects like http://apt.damz.org/ also help

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

--

--
Regards,
Shein Alexey

13 years ago by Christopher Jones — view source

unread

We don't push them out as news on the php.net frontpage and we don't
send it out to the announce list for reasons like not confusing users.
Should we change that?

Announcements of RC X, Alpha X etc should be on the frontpage.

Chris

--
Email: christopher.jones@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/

13 years ago by johannes@schlueters.de — view source

unread

We don't push them out as news on the php.net frontpage and we don't
send it out to the announce list for reasons like not confusing users.
Should we change that?

Announcements of RC X, Alpha X etc should be on the frontpage.

Provocative: Would that help in any way? I know that PEAR people knew
about the RCs but still there were 3 RCs or so not properly tested with
PEAR it seems.

A hope there is that more visibility == more testing == more feedback.

johannes

ps. I'm not blaming PEAR or any individual. But well it is a php.net
project and having an issue in there discovered roughly a week after
release is late and is an indication of a fundamental problem ...

Chris

--
Email: christopher.jones@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/

13 years ago by alan@akbkhome.com — view source

unread

Since when has changing documented behaviour and breaking a large number
of applications been acceptable? (Well, happens occasionally by accident ..)

This was a 'feature change' to is_a(), it was documented as only
returning 'TRUE' when an object was passed as the first object and it
was an instance of that...

I did read through the previous posts, (just caught up the other day),
it looks like if anybody really need to do what this new feature
provides, (which is probably very rare), then

$left == $right || is_subclass_of($left,$right)

should work fine without breaking any code.

Regards
Alan

It might have been better to have waited for the is_a() fix to get sorted
out.. - It's a very annoying BC break (changes the documented behaviour),
and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current behavior
is correct, the old was a bug:

as Stas pointed out:
"Not a bug. $var is interpreted as a class name. To know if one class
extends another, the class in question (first one) should be loaded.
There's no need to load the second one since if it's unknown nothing
can be instance of it and existing classes can not extend it."
if you used this previously

from Dmitry:
"Before the patch, is_a() didn't accept string as the first argument
at all, so it always returned "false" and never triggered
__autoload(). The proposed patch didn't revert to original behavior.
It just disables autoloading and may lead to wrong result.

class a {}
class b extends a {}
var_dump(is_a("b", "a")); // it was false before 5.3.7, now it's true

I would say that the old behaviour was wrong, especially because
"instanceof" and is_subclass_of() already implemented support for
string arguments."

so your example was bogus, as passing a non-object as a first
parameter wasn't supported (see http://php.net/is_a) so your code
example depends on an undefined behavior and results in a bogus result
(is_a() alwas returned false if you passed a non-object)

so in a way it is really a BC, but I think that this change is really a bugfix.

13 years ago by Zeev Suraski — view source

unread

I think there are two ways to look at it:

As a new feature. If I understand you correctly, the fact that beforehand is_a() was documented to only return TRUE in case the first argument was an instance of the second argument, means that if we do anything beyond that - e.g. support strings as the first argument - this means we break compatibility (please correct me if I misunderstood). IMHO that's not a realistic way to look at retaining compatibility, and I'm saying that as someone who's almost religious about maintain compatibility. With that view of the world, almost every bug fix and literally every feature we add - breaks compatibility.
As a bug fix. Strings were supported even before this change, but they weren't working properly or consistently with the way classes work everywhere else in PHP. That was fixed. Just so that we feel good about ourselves, we also changed undocumented behavior. In case it's not clear, a situation where is_a("bar", "foo") returns FALSE, even though bar extends foo, but bar simply doesn't happen to be loaded in memory at the time of the call - can lead to very real world bugs.

Zeev

-----Original Message-----
From: alan@akbkhome.com [mailto:alan@akbkhome.com]
Sent: Wednesday, August 24, 2011 12:37 PM
To: internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

Since when has changing documented behaviour and breaking a large
number of applications been acceptable? (Well, happens occasionally by
accident ..)

This was a 'feature change' to is_a(), it was documented as only returning
'TRUE' when an object was passed as the first object and it was an instance of
that...

I did read through the previous posts, (just caught up the other day), it looks
like if anybody really need to do what this new feature provides, (which is
probably very rare), then

$left == $right || is_subclass_of($left,$right)

should work fine without breaking any code.

Regards
Alan

On Wed, Aug 24, 2011 at 3:57 AM,
alan@akbkhome.com alan@akbkhome.com wrote:

It might have been better to have waited for the is_a() fix to get
sorted out.. - It's a very annoying BC break (changes the documented
behaviour), and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current behavior
is correct, the old was a bug:

as Stas pointed out:
"Not a bug. $var is interpreted as a class name. To know if one class
extends another, the class in question (first one) should be loaded.
There's no need to load the second one since if it's unknown nothing
can be instance of it and existing classes can not extend it."
if you used this previously

from Dmitry:
"Before the patch, is_a() didn't accept string as the first argument
at all, so it always returned "false" and never triggered
__autoload(). The proposed patch didn't revert to original behavior.
It just disables autoloading and may lead to wrong result.

class a {}
class b extends a {}
var_dump(is_a("b", "a")); // it was false before 5.3.7, now it's true

I would say that the old behaviour was wrong, especially because
"instanceof" and is_subclass_of() already implemented support for
string arguments."

so your example was bogus, as passing a non-object as a first
parameter wasn't supported (see http://php.net/is_a) so your code
example depends on an undefined behavior and results in a bogus result
(is_a() alwas returned false if you passed a non-object)

so in a way it is really a BC, but I think that this change is really a bugfix.

--
To unsubscribe, visit:
http://www.php.net/unsub.php

13 years ago by Pierre Joye — view source

unread

I think there are two ways to look at it:

As a new feature. If I understand you correctly, the fact that beforehand is_a() was documented to only return TRUE in case the first argument was an instance of the second argument, means that if we do anything beyond that - e.g. support strings as the first argument - this means we break compatibility (please correct me if I misunderstood). IMHO that's not a realistic way to look at retaining compatibility, and I'm saying that as someone who's almost religious about maintain compatibility. With that view of the world, almost every bug fix and literally every feature we add - breaks compatibility.

As a bug fix. Strings were supported even before this change, but they weren't working properly or consistently with the way classes work everywhere else in PHP. That was fixed. Just so that we feel good about ourselves, we also changed undocumented behavior. In case it's not clear, a situation where is_a("bar", "foo") returns FALSE, even though bar extends foo, but bar simply doesn't happen to be loaded in memory at the time of the call - can lead to very real world bugs.

No matter what it is or how it is defined by us, it breaks existing
code and that should be avoided in bug fixes releases like 5.3.7/8.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Zeev Suraski — view source

unread

No matter what it is or how it is defined by us, it breaks existing code and that
should be avoided in bug fixes releases like 5.3.7/8.

Pierre,

This wholesale statement doesn't get us anywhere. Every bug fix can result in breaking existing code. If due to a logic error, under some circumstances - file_exists() returned false for a file that actually exists, are we barred from fixing that in a maintenance release? Obviously not. What about bug # 54459? What if some piece of code out there relied on this behavior? What about # 55082 - what if someone already relied on this and wrote a layer to alter the output accordingly?

Since clearly the definition of never breaking existing code, no matter how far-fetched it may be, means we can't do just about anything in bug fix releases - we need to set slightly more realistic definitions. The fix for is_a() falls squarely within the realm of stuff we should be doing in bug fix releases, IMHO. It is a bug fix, bug fixes by definition change behavior - sometimes to the degree of breaking certain (broken) pieces of code.

Zeev

13 years ago by Pierre Joye — view source

unread

No matter what it is or how it is defined by us, it breaks existing code and that
should be avoided in bug fixes releases like 5.3.7/8.

Pierre,

This wholesale statement doesn't get us anywhere.

It does, we underestimate the situation and this
fix/improvement/consistency change breaks apps and codes out there.
And I do not consider it as acceptable at this stage in 5.3.x. Let do
it only in 5.4.

Every bug fix can result in breaking existing code.

Not every, and here it is easily fixable.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Zeev Suraski — view source

unread

This wholesale statement doesn't get us anywhere.

It does, we underestimate the situation and this fix/improvement/consistency
change breaks apps and codes out there.
And I do not consider it as acceptable at this stage in 5.3.x. Let do it only in
5.4.

How is it different from any of the other non-crash-fixing bugs we fixed, that could break apps that relied on the old behavior? Gave you two examples from the last release (well, 5.3.7), and another imaginary one. They all fall in the category of potentially breaking code out there, and yet - they should all be fixed.

Every bug fix can result in breaking existing code.

Not every, and here it is easily fixable.

I'll refrain from word fighting you on the first part, but on the second - how is it easily fixable?

This:

is_a("bar", "foo"); // false
$obj = new bar;
is_a($obj, "foo"); // true, since now that bar's been loaded we know it extends foo

Is a language bug, and a pretty bad one too.

Zeev

13 years ago by Pierre Joye — view source

unread

This wholesale statement doesn't get us anywhere.

It does, we underestimate the situation and this fix/improvement/consistency
change breaks apps and codes out there.
And I do not consider it as acceptable at this stage in 5.3.x. Let do it only in
5.4.

How is it different from any of the other non-crash-fixing bugs we fixed, that could break apps that relied on the old behavior? Gave you two examples from the last release (well, 5.3.7), and another imaginary one. They all fall in the category of potentially breaking code out there, and yet - they should all be fixed.

Zeev, the whole point is that changes like this one must be discussed
on a case by case basis. It is now very well known that when a fix
requires a test change, then it often leads to bc breaks or similar
issues.

I do not think we have to argue about the semantics or general cases
but how to avoid such things and be sure that we break as less code as
possible in patches release.

It is also obvious, as you stated, that there will have edge cases
where we have to break existing codes, for security or critical
reasons. This is definitively not the case here.

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Zeev Suraski — view source

unread

by case basis. It is now very well known that when a fix requires a test change,
then it often leads to bc breaks or similar issues.

I do not think we have to argue about the semantics or general cases but how
to avoid such things and be sure that we break as less code as possible in
patches release.

In order to discuss how to avoid 'such things', we need to talk about different types of cases.

It is also obvious, as you stated, that there will have edge cases where we have
to break existing codes, for security or critical reasons. This is definitively not
the case here.

It has nothing to do with security (criticality is subjective so I'm leaving it aside). The 3 bugs I mentioned (2 from 5.3.7 and one imaginary) have nothing to do with security, and yet we fixed them (or would have fixed them), despite the potential for people out there relying on the old behavior. It boils down to evaluating the severity of the bug and the likelihood that it'll break code. If it's a clear bug, which IMHO this is_a() issue was - then unless we're looking at code breakage at massive scale, it should be fixed.

Again, I'm almost religious about retaining compatibility (even across major versions), but if we had a piece of code that was returning clearly the wrong value, we can't ignore it because some (my guess very few but who knows) relied on this behavior.

Zeev

13 years ago by alan@akbkhome.com — view source

unread

" If it's a clear bug, which IMHO this is_a() issue was - then unless
we're looking at code breakage at massive scale, it should be fixed. "

mmh.. how much breakage did you want.

PEAR::isError is basically is_a($input, 'PEAR_Error'); it's been like
that for > 8 years....

google search for PEAR::isError shows 16,600 matches..

http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php&type=cs
http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php&type=cs

for is_a you get 149K. and this is only public code...

It's big... Luckily quite a few people are on holiday this month and
will not upgrade too soon.

Regards
Alan

It has nothing to do with security (criticality is subjective so I'm leaving it aside). The 3 bugs I mentioned (2 from 5.3.7 and one imaginary) have nothing to do with security, and yet we fixed them (or would have fixed them), despite the potential for people out there relying on the old behavior. It boils down to evaluating the severity of the bug and the likelihood that it'll break code. If it's a clear bug, which IMHO this is_a() issue was - then unless we're looking at code breakage at massive scale, it should be fixed.

Again, I'm almost religious about retaining compatibility (even across major versions), but if we had a piece of code that was returning clearly the wrong value, we can't ignore it because some (my guess very few but who knows) relied on this behavior.

Zeev

13 years ago by Pierre Joye — view source

unread

Hi Alan,

the breakage is about is_a with a string as 1st argument, not is_a as
a whole. So yes, it breaks is_a alone is used as validation.

" If it's a clear bug, which IMHO this is_a() issue was - then unless we're
looking at code breakage at massive scale, it should be fixed. "

mmh.. how much breakage did you want.

PEAR::isError is basically is_a($input, 'PEAR_Error'); it's been like that
for > 8 years....

google search for PEAR::isError shows 16,600 matches..

http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php&type=cs
http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php&type=cs

for is_a you get 149K. and this is only public code...

It's big... Luckily quite a few people are on holiday this month and will
not upgrade too soon.

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Jonathan Bond-Caron — view source

unread

Hi Alan,

the breakage is about is_a with a string as 1st argument, not is_a as
a whole. So yes, it breaks is_a alone is used as validation.

I've been digging more into this:
http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/Zend/zend_builtin_fun
ctions.c?r1=307522&r2=312904&pathrev=312904

From what I understand, this patch is only place where is_a() all of sudden
starts accepting a string.
Btw the documentation has never been updated:
http://php.net/manual/en/function.is-a.php

It seems unintentional, the patch tries to fix a bug but introduces a new
'feature'. Should it be reverted?

13 years ago by Etienne Kneuss — view source

unread

Hi,

Hi Alan,

the breakage is about is_a with a string as 1st argument, not is_a as
a whole. So yes, it breaks is_a alone is used as validation.

I've been digging more into this:

http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/Zend/zend_builtin_fun
ctions.c?r1=307522&r2=312904&pathrev=312904

From what I understand, this patch is only place where is_a() all of sudden
starts accepting a string.
Btw the documentation has never been updated:
http://php.net/manual/en/function.is-a.php

It seems unintentional, the patch tries to fix a bug but introduces a new
'feature'. Should it be reverted?

We already discussed that in length the past couple of weeks, the patch
was in fact intentional and we decided not to revert it.

Best,

--

--
Etienne Kneuss
http://www.colder.ch

13 years ago by Nicolas Grekas — view source

unread

We already discussed that in length the past couple of weeks, the patch
was in fact intentional and we decided not to revert it.

I'm ok with the decision,
but what about adding a third argument [, bool $autoload = true ] to is_a()
and is_subclass_of(),
in order to be consistent with class_parents() and class_implements() ?

Nicolas

13 years ago by Zeev Suraski — view source

unread

Well, I have to admit this is mighty convincing :) Wasn't aware of this use-case. Falls under the category of mass breakage I guess.

Zeev

-----Original Message-----
From: alan@akbkhome.com [mailto:alan@akbkhome.com]
Sent: Wednesday, August 24, 2011 3:39 PM
To: Zeev Suraski; internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

" If it's a clear bug, which IMHO this is_a() issue was - then unless we're
looking at code breakage at massive scale, it should be fixed. "

mmh.. how much breakage did you want.

PEAR::isError is basically is_a($input, 'PEAR_Error'); it's been like that for > 8
years....

google search for PEAR::isError shows 16,600 matches..

http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php&;
type=cs
<http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php
&type=cs>

for is_a you get 149K. and this is only public code...

It's big... Luckily quite a few people are on holiday this month and will not
upgrade too soon.

Regards
Alan

It has nothing to do with security (criticality is subjective so I'm leaving it
aside). The 3 bugs I mentioned (2 from 5.3.7 and one imaginary) have nothing
to do with security, and yet we fixed them (or would have fixed them), despite
the potential for people out there relying on the old behavior. It boils down to
evaluating the severity of the bug and the likelihood that it'll break code. If it's
a clear bug, which IMHO this is_a() issue was - then unless we're looking at
code breakage at massive scale, it should be fixed.

Again, I'm almost religious about retaining compatibility (even across major
versions), but if we had a piece of code that was returning clearly the wrong
value, we can't ignore it because some (my guess very few but who knows)
relied on this behavior.

Zeev

13 years ago by jawed@php.net — view source

unread

I think it's too late to do much of anything other than document the
change in a way that makes it easier for people to recognize. I spoke
to Philip about this offline and I think the two options along these
lines are:

Add a WARNING box which specifies the change for the function, that
a true type check for strings no longer yields FALSE
Update the first argument documentation from object -> mixed,
mention it has started accepting mixed as of 5.3.7, and highlight the
fact this function no longer returns FALSE when the type check (for
strings) fails.

fwiw - the pre-5.3.7 behavior was sub-optimal/broken but this is a BC
break, if you consider modifying an existing function signature a BC
break (which I'd hope most people do).

I realize the release RFC isn't live till 5.4 but I am concerned that
the continuing debate around what is and isn't a BC break, which has
taken a large chunk of this thread, will hinder the relrfc moving
forward.

Well, I have to admit this is mighty convincing :) Wasn't aware of this use-case. Falls under the category of mass breakage I guess.

Zeev

-----Original Message-----
From: alan@akbkhome.com [mailto:alan@akbkhome.com]
Sent: Wednesday, August 24, 2011 3:39 PM
To: Zeev Suraski; internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

" If it's a clear bug, which IMHO this is_a() issue was - then unless we're
looking at code breakage at massive scale, it should be fixed. "

mmh.. how much breakage did you want.

PEAR::isError is basically is_a($input, 'PEAR_Error'); it's been like that for > 8
years....

google search for PEAR::isError shows 16,600 matches..

http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php&;
type=cs
<http://www.google.com/codesearch#search/&q=PEAR::isError%20lang:php
&type=cs>

for is_a you get 149K. and this is only public code...

It's big... Luckily quite a few people are on holiday this month and will not
upgrade too soon.

Regards
Alan

It has nothing to do with security (criticality is subjective so I'm leaving it
aside). The 3 bugs I mentioned (2 from 5.3.7 and one imaginary) have nothing
to do with security, and yet we fixed them (or would have fixed them), despite
the potential for people out there relying on the old behavior. It boils down to
evaluating the severity of the bug and the likelihood that it'll break code. If it's
a clear bug, which IMHO this is_a() issue was - then unless we're looking at
code breakage at massive scale, it should be fixed.

Again, I'm almost religious about retaining compatibility (even across major
versions), but if we had a piece of code that was returning clearly the wrong
value, we can't ignore it because some (my guess very few but who knows)
relied on this behavior.

Zeev

13 years ago by Stas Malyshev — view source

unread

Hi!

" If it's a clear bug, which IMHO this is_a() issue was - then unless
we're looking at code breakage at massive scale, it should be fixed. "

mmh.. how much breakage did you want.

PEAR::isError is basically is_a($input, 'PEAR_Error'); it's been like
that for> 8 years....

So, PEAR has a bug, if it passes non-object argument to is_a, as
previously is_a was only documented (and actually still is) as accepting
objects. The fact that this bug remained for 8 years is very sad, but it
doesn't cease to be a bug.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Lester Caine — view source

unread

Stas Malyshev wrote:

mmh.. how much breakage did you want.

PEAR::isError is basically is_a($input, 'PEAR_Error'); it's been like
that for> 8 years....

So, PEAR has a bug, if it passes non-object argument to is_a, as previously is_a
was only documented (and actually still is) as accepting objects. The fact that
this bug remained for 8 years is very sad, but it doesn't cease to be a bug.

If I am reading things right, is_a was only designed to handle an object, so
feeding it a mixed parameter in isError was always wrong? As far as I can see,
on the whole, the PEAR code only ever feeds an object and feeding it a string
would have to be a real error? So there are a number of actions here all of
which are potentially wrong, and PEAR should return an error if $input is not a
valid object rather than relying on undocumented actions simply to fail?

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk//
Firebird - http://www.firebirdsql.org/index.php

13 years ago by Stas Malyshev — view source

unread

Hi!

If I am reading things right, is_a was only designed to handle an object, so
feeding it a mixed parameter in isError was always wrong? As far as I can see,
on the whole, the PEAR code only ever feeds an object and feeding it a string
would have to be a real error? So there are a number of actions here all of
which are potentially wrong, and PEAR should return an error if $input is not a
valid object rather than relying on undocumented actions simply to fail?

If they just wanted to see if it's an object of class PEAR_Error,
instanceof should be used.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Ferenc Kovacs — view source

unread

If I am reading things right, is_a was only designed to handle an object, so
feeding it a mixed parameter in isError was always wrong? As far as I can
see, on the whole, the PEAR code only ever feeds an object and feeding it a
string would have to be a real error? So there are a number of actions here
all of which are potentially wrong, and PEAR should return an error if
$input is not a valid object rather than relying on undocumented actions
simply to fail?

for the record, this is what happened:
1, https://bugs.php.net/bug.php?id=53727 got reported
2, dimitry commited http://svn.php.net/viewvc/?view=revision&revision=312904
3, that commit broke Pear, as is_a() started throwing a warning, if
non-object is passed as first argument
https://pear.php.net/bugs/bug.php?id=18656
4, Johannes brought that issue up on the mailing list:
internals@lists.php.net/msg51868.html" rel="nofollow" target="_blank">http://www.mail-archive.com/internals@lists.php.net/msg51868.html and
in the end the the accepted solution was to remove the warning.
5, Helgi fixed Pear in the meanwhile
http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313081&r2=313083&pathrev=313340
6, Stas removes the warning
http://svn.php.net/viewvc?view=revision&revision=313162 Dmitry
adds some tests
http://svn.php.net/viewvc/?view=revision&revision=313271
7, Helgi reverts the Pear fix
http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313340&r2=313339&pathrev=313340
8, nobody notices the meaning of this change:
http://svn.php.net/viewvc/php/php-src/branches/PHP_5_4/Zend/zend_builtin_functions.c?r1=312904&r2=312903&pathrev=312904#l848
which AFAIK means that the zend_lookup_class (and hence autoloading)
will always be called if the first argument is a string for is_a.
previously it would only happen for is_subclass_of()

Tyrael

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Stas Malyshev — view source

unread

Hi!

Thanks for providing the timeline.

5, Helgi fixed Pear in the meanwhile
http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313081&r2=313083&pathrev=313340

This fix doesn't look good - it doesn't do what is was meant to do.

7, Helgi reverts the Pear fix
http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313340&r2=313339&pathrev=313340

And this should be using instanceof instead.

8, nobody notices the meaning of this change:
http://svn.php.net/viewvc/php/php-src/branches/PHP_5_4/Zend/zend_builtin_functions.c?r1=312904&r2=312903&pathrev=312904#l848
which AFAIK means that the zend_lookup_class (and hence autoloading)
will always be called if the first argument is a string for is_a.
previously it would only happen for is_subclass_of()

Well, it is obvious to me that `is_a()` and `is_subclass_of()` should work
the same and both autoload the first argument if it's a string. However,
the docs have `is_subclass_of()` documented as accepting string while
`is_a()` is not and it worked as always returning false given non-object.
I think we could easily keep this behavior for 5.3 even though I think
relying on this is wrong (and you SHOULD fix it anywhere your code
relies on it, including PEAR).

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Ferenc Kovacs — view source

unread

Hi!

Thanks for providing the timeline.

5, Helgi fixed Pear in the meanwhile

http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313081&r2=313083&pathrev=313340

This fix doesn't look good - it doesn't do what is was meant to do.

/o, I didn't noticed it until you mentioned...

7, Helgi reverts the Pear fix

http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313340&r2=313339&pathrev=313340

And this should be using instanceof instead.

yep, I think the main reason that is_a was used in the first place is
the fact that instanceof was added with php 5...
obviously there is no point in watching out for php4 compatibility anymore.
or at least it shouldn't be.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Alan Knowles — view source

unread

Some real history for the young ones ;)

If you go all the way back to when is_a() was introduced, it appears
that it was done to simplify the code in PEAR::isError, which basically
did stuff like

if is_object() and is subclass() or get_classname() == ...

So the previous behavior was very likely the 'designed' behavior. Not an
accidental side effect, or bug.

It's use case is reasonably common when doing tests on mixed returns
(method returns PEAR:error, object or normal value.)

So we had a situation where a reasonable number of people (eg. anyone
who had used PEAR), had seen and expected the previous behavior.

Where as now, we have not had a single direct bug report saying this
behavior is bad (AFAIK), yet we are going to change it to fix an unusual
use case on is_subclass_of as they use the same backend code.

If this had been reported as a bug, or anybody on this list had actually
been using it and had a gotcha moment, fine. but I've not heard anybody
say that, just they think perhaps it should work a different way.

Please do not fix something that is not broken, and breaks real working
code, just for the hell of it, even in 5.4.

Regards
Alan

Hi!

Thanks for providing the timeline.

5, Helgi fixed Pear in the meanwhile
http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313081&r2=313083&pathrev=313340

This fix doesn't look good - it doesn't do what is was meant to do.

7, Helgi reverts the Pear fix
http://svn.php.net/viewvc/pear/pear-core/tags/PEAR-1.9.5/PEAR.php?r1=313340&r2=313339&pathrev=313340

And this should be using instanceof instead.

8, nobody notices the meaning of this change:
http://svn.php.net/viewvc/php/php-src/branches/PHP_5_4/Zend/zend_builtin_functions.c?r1=312904&r2=312903&pathrev=312904#l848

which AFAIK means that the zend_lookup_class (and hence autoloading)
will always be called if the first argument is a string for is_a.
previously it would only happen for is_subclass_of()

Well, it is obvious to me that is_a() and is_subclass_of() should work
the same and both autoload the first argument if it's a string.
However, the docs have is_subclass_of() documented as accepting string
while is_a() is not and it worked as always returning false given
non-object. I think we could easily keep this behavior for 5.3 even
though I think relying on this is wrong (and you SHOULD fix it
anywhere your code relies on it, including PEAR).

13 years ago by Stas Malyshev — view source

unread

Hi!

Some real history for the young ones ;)

I wonder who you are meaning... :)

So the previous behavior was very likely the 'designed' behavior. Not an
accidental side effect, or bug.

Bugs can be very well intentional, but if they use the language wrong
way they are bugs.

It's use case is reasonably common when doing tests on mixed returns
(method returns PEAR:error, object or normal value.)

That's when you use instanceof.

So we had a situation where a reasonable number of people (eg. anyone
who had used PEAR), had seen and expected the previous behavior.

Seeing wrong code somewhere doesn't mean it's not wrong. There's a
reason we have the manual.

Please do not fix something that is not broken, and breaks real working
code, just for the hell of it, even in 5.4.

`is_a()` was broken - it was returning different results from essentially
the same function `is_subclass_of()` for no reason at all (no, somebody
writing buggy code in PEAR using undocumented parameter types is not a
reason). The reason why we kept `is_a()` and not killed it in favor of
instanceof was to have it work with string arguments, since instanceof
does not. Thus, string arguments should be handled properly. I can see
how it can be argued that 5.3 is mature enough so such changes shouldn't
be there, however correct in theory. For 5.4, I see no base for argument
here.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by alan@akbkhome.com — view source

unread

I'm not sure it's possible to get agreement on if this is a bug or not,
you might see a bug, I just see this as a pointless change for
consistency that pretty much nobody will ever need or use.

I think I'll leave it as
a) no bug was ever reported on the previous behaviour.

b) intended "design" of is_subclass_of was originally to return false
on non-object - andrei (1999)
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=17245&r2=17272
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=17245&r2=17272

c) is_a() was introduced by sebastian (2002) and kept this intended
behaviour
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=67102&r2=69234
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=67102&r2=69234

d) when andrey (2004) proposed the change to accepts strings on
is_subclass_of, he deliberately maintained the existing behaviour for
is_a()
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=170604&r2=171349
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=170604&r2=171349

e) is_a() has a valid use case , and is currently reasonably commonly used.

d) the new behaviour is an uncommon use case, and can be done very
easily in other ways.

BTW. we could really do with a searchable archive of php.dev +
internals... - It's pretty difficult to find out if this was ever
discussed before..

Regards
Alan

Hi!

Some real history for the young ones ;)

I wonder who you are meaning... :)

So the previous behavior was very likely the 'designed' behavior. Not an
accidental side effect, or bug.

Bugs can be very well intentional, but if they use the language wrong
way they are bugs.

It's use case is reasonably common when doing tests on mixed returns
(method returns PEAR:error, object or normal value.)

That's when you use instanceof.

So we had a situation where a reasonable number of people (eg. anyone
who had used PEAR), had seen and expected the previous behavior.

Seeing wrong code somewhere doesn't mean it's not wrong. There's a
reason we have the manual.

Please do not fix something that is not broken, and breaks real working
code, just for the hell of it, even in 5.4.

is_a() was broken - it was returning different results from
essentially the same function is_subclass_of() for no reason at all
(no, somebody writing buggy code in PEAR using undocumented parameter
types is not a reason). The reason why we kept is_a() and not killed
it in favor of instanceof was to have it work with string arguments,
since instanceof does not. Thus, string arguments should be handled
properly. I can see how it can be argued that 5.3 is mature enough so
such changes shouldn't be there, however correct in theory. For 5.4, I
see no base for argument here.

13 years ago by Ronald Chmara — view source

unread

BTW. we could really do with a searchable archive of php.dev + internals...

It's pretty difficult to find out if this was ever discussed before..

http://marc.info/?l=php-internals

marc has a ton of the PHP lists. (Is this not what you are looking for?)

-Ronabop

13 years ago by alan@akbkhome.com — view source

unread

It did not like my search for is_a ;) - I guess it's too short.

BTW. we could really do with a searchable archive of php.dev + internals...

It's pretty difficult to find out if this was ever discussed before..
http://marc.info/?l=php-internals

marc has a ton of the PHP lists. (Is this not what you are looking for?)

-Ronabop

13 years ago by Aleksandr Galkin — view source

unread

For the record, I'd like to point out I do need the new behaviour.
In 5.3.6 you need reflection to check if a class implements an interface.
You also need to check is_subclass_of AND compare the lowercased classes.
All that is about 5 times slower than is_a in 5.3.8.

Probably it should be class_is_a() instead of altering is_a() behaviour,
but the need to match class names against each other is pretty much real.

Sincerely yours, Aleksandr.

I'm not sure it's possible to get agreement on if this is a bug or not, you might see a bug, I just see this as a pointless change for consistency that pretty much nobody will ever need or use.

I think I'll leave it as
a) no bug was ever reported on the previous behaviour.

b) intended "design" of is_subclass_of was originally to return false on non-object - andrei (1999)
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=17245&r2=17272 http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=17245&r2=17272

c) is_a() was introduced by sebastian (2002) and kept this intended behaviour
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=67102&r2=69234 http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=67102&r2=69234

d) when andrey (2004) proposed the change to accepts strings on is_subclass_of, he deliberately maintained the existing behaviour for is_a()
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=170604&r2=171349 http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=170604&r2=171349

e) is_a() has a valid use case , and is currently reasonably commonly used.

d) the new behaviour is an uncommon use case, and can be done very easily in other ways.

BTW. we could really do with a searchable archive of php.dev + internals... - It's pretty difficult to find out if this was ever discussed before..

Regards
Alan

13 years ago by Pierre Joye — view source

unread

BTW. we could really do with a searchable archive of php.dev + internals...

It's pretty difficult to find out if this was ever discussed before..

Again, it was discussed already and the argument of using instanceof
was used to deprecate is_a (along other arguments). I see no point to
argue again about that. It was a mistake to change it again in 5.3,
Zeev realized the use in this exact case, let move on now. Revert that
in 5.3 and do it only in 5.4.

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Zeev Suraski — view source

unread

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Thursday, August 25, 2011 11:31 AM
To: alan@akbkhome.com
Cc: Stas Malyshev; internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

On Thu, Aug 25, 2011 at 3:51 AM, alan@akbkhome.com
alan@akbkhome.com wrote:

BTW. we could really do with a searchable archive of php.dev + internals...

It's pretty difficult to find out if this was ever discussed before..

Again, it was discussed already and the argument of using instanceof was used
to deprecate is_a (along other arguments). I see no point to argue again about
that. It was a mistake to change it again in 5.3, Zeev realized the use in this
exact case, let move on now. Revert that in 5.3 and do it only in 5.4.

Just so that my position is clear on the matter:

I still think that is_a() is working properly the way it does now, after the change.
I think the code in isError() is wrong, and should be fixed.
Had we (the collective we) known that fixing is_a() would result in such breakage, it would have probably been wise not to fix it in 5.3.x, and wait for 5.4 for this purpose.
Given that we've already done it - I wouldn't revert it. Fix it in PEAR. That's the only way to create something that works across all versions of 5.3.x.

Zeev

13 years ago by Pierre Joye — view source

unread

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Thursday, August 25, 2011 11:31 AM
To: alan@akbkhome.com
Cc: Stas Malyshev; internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

On Thu, Aug 25, 2011 at 3:51 AM, alan@akbkhome.com
alan@akbkhome.com wrote:

BTW. we could really do with a searchable archive of php.dev + internals...

It's pretty difficult to find out if this was ever discussed before..

Again, it was discussed already and the argument of using instanceof was used
to deprecate is_a (along other arguments). I see no point to argue again about
that. It was a mistake to change it again in 5.3, Zeev realized the use in this
exact case, let move on now. Revert that in 5.3 and do it only in 5.4.

Just so that my position is clear on the matter:

I still think that is_a() is working properly the way it does now, after the change.

I think the code in isError() is wrong, and should be fixed.

Had we (the collective we) known that fixing is_a() would result in such breakage, it would have probably been wise not to fix it in 5.3.x, and wait for 5.4 for this purpose.

Given that we've already done it - I wouldn't revert it. Fix it in PEAR. That's the only way to create something that works across all versions of 5.3.x.

I won't battle endlessly to get that fixed back again (you will always
find smtg else to keep that thing in anyway ;-) but it does confirm
what I said earlier about changing behaviors in patch releases. This
is something we must deal with much more carefully. And using x.y.0
tests as base is a good start, also the BC breakages are now covered
by the RFC, let stick to that for 5.4.x.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by johannes@schlueters.de — view source

unread

Given that we've already done it - I wouldn't revert it. Fix it in
PEAR. That's the only way to create something that works across all
versions of 5.3.x.

Unfortunately this is the case.

johannes

13 years ago by Matthew Weier O'Phinney — view source

unread

I'm not sure it's possible to get agreement on if this is a bug or
not, you might see a bug, I just see this as a pointless change for
consistency that pretty much nobody will ever need or use.

Please don't generalize based on your own opinions and use cases. I am a
long time PEAR user (and contributor), and I actually agree with the
change.

The reporter of the issue that started this all is a colleague of mine,
Ralph Schindler, and we discussed it in June along with David Zuilke,
who had run into similar issues we had (as well as discussed it with
others in other projects). It's not an isolated request; there are many
who find the current behavior of is_a() (pre-5.3.7) incoherent for
modern PHP usage.

Basically, in our case, we were building a DI container. To keep the DI
container light-weight, you create definitions that utilize string class
names. In order to determine what injections need to be made, you need
to introspect the class a little -- and this is where is_a() vs
is_subclass_of() vs instanceof comes into play. The latter two require
an object instance -- which we may not yet be ready to provide (we may
be trying to determine what to inject into the constructor). is_a() does
not require an object instance... but prior to 5.3.7 was unable to
test against inherited behavior. As such, the only fallback is the much
more expensive Reflection API.

Having is_a() work properly with string class names and checking the
inheritance hierarchy is a huge improvement, keeps it semantically
consistent with the rest of the language, and provides capabilities
is_subclass_of() cannot (as it cannot check against strings).

I think I'll leave it as
a) no bug was ever reported on the previous behaviour.

False -- others in this thread have pointed it out, and I alluded to the
report Ralph issued earlier.

b) intended "design" of is_subclass_of was originally to return false
on non-object - andrei (1999)
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=17245&r2=17272
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=17245&r2=17272

c) is_a() was introduced by sebastian (2002) and kept this intended
behaviour
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=67102&r2=69234
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=67102&r2=69234

d) when andrey (2004) proposed the change to accepts strings on
is_subclass_of, he deliberately maintained the existing behaviour for
is_a()
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=170604&r2=171349
http://svn.php.net/viewvc/php/php-src/trunk/Zend/zend_builtin_functions.c?r1=170604&r2=171349

e) is_a() has a valid use case , and is currently reasonably commonly used.

d) the new behaviour is an uncommon use case, and can be done very
easily in other ways.

BTW. we could really do with a searchable archive of php.dev +
internals... - It's pretty difficult to find out if this was ever
discussed before..

Regards
Alan

Hi!

Some real history for the young ones ;)

I wonder who you are meaning... :)

So the previous behavior was very likely the 'designed' behavior. Not an
accidental side effect, or bug.

Bugs can be very well intentional, but if they use the language wrong
way they are bugs.

It's use case is reasonably common when doing tests on mixed returns
(method returns PEAR:error, object or normal value.)

That's when you use instanceof.

So we had a situation where a reasonable number of people (eg. anyone
who had used PEAR), had seen and expected the previous behavior.

Seeing wrong code somewhere doesn't mean it's not wrong. There's a
reason we have the manual.

Please do not fix something that is not broken, and breaks real working
code, just for the hell of it, even in 5.4.

is_a() was broken - it was returning different results from
essentially the same function is_subclass_of() for no reason at all
(no, somebody writing buggy code in PEAR using undocumented parameter
types is not a reason). The reason why we kept is_a() and not killed
it in favor of instanceof was to have it work with string arguments,
since instanceof does not. Thus, string arguments should be handled
properly. I can see how it can be argued that 5.3 is mature enough so
such changes shouldn't be there, however correct in theory. For 5.4, I
see no base for argument here.

--
Matthew Weier O'Phinney
Project Lead | matthew@zend.com
Zend Framework | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc

13 years ago by alan@akbkhome.com — view source

unread

It's not really a bug fix, as the document for is_a() never said it
would support 'mixed' first argument. (is_subclass_of is different, it
currently says mixed as the first argument). If it needed fixing, then
support for strings should have been removed, rather than added.

The problem I have is that this 'feature' is not really that usefull, to
anyone AFAIKS. (why would you realistically want to do that kind of
check?) and if you really did need to do such a test, there are other
trivial ways to do it already (which work across versions).

I know it's reusing code at the backend, but it's a trivial change to
remove the BC break, and it does not break anything else. (nobody would
have used it that way yet..).

Regards
Alan

I think there are two ways to look at it:

As a new feature. If I understand you correctly, the fact that beforehand is_a() was documented to only return TRUE in case the first argument was an instance of the second argument, means that if we do anything beyond that - e.g. support strings as the first argument - this means we break compatibility (please correct me if I misunderstood). IMHO that's not a realistic way to look at retaining compatibility, and I'm saying that as someone who's almost religious about maintain compatibility. With that view of the world, almost every bug fix and literally every feature we add - breaks compatibility.

As a bug fix. Strings were supported even before this change, but they weren't working properly or consistently with the way classes work everywhere else in PHP. That was fixed. Just so that we feel good about ourselves, we also changed undocumented behavior. In case it's not clear, a situation where is_a("bar", "foo") returns FALSE, even though bar extends foo, but bar simply doesn't happen to be loaded in memory at the time of the call - can lead to very real world bugs.

Zeev

-----Original Message-----
From: alan@akbkhome.com [mailto:alan@akbkhome.com]
Sent: Wednesday, August 24, 2011 12:37 PM
To: internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

Since when has changing documented behaviour and breaking a large
number of applications been acceptable? (Well, happens occasionally by
accident ..)

This was a 'feature change' to is_a(), it was documented as only returning
'TRUE' when an object was passed as the first object and it was an instance of
that...

I did read through the previous posts, (just caught up the other day), it looks
like if anybody really need to do what this new feature provides, (which is
probably very rare), then

$left == $right || is_subclass_of($left,$right)

should work fine without breaking any code.

Regards
Alan

On Wed, Aug 24, 2011 at 3:57 AM,
alan@akbkhome.com alan@akbkhome.com wrote:

It might have been better to have waited for the is_a() fix to get
sorted out.. - It's a very annoying BC break (changes the documented
behaviour), and now it means we need 4.3.9 in a few more days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current behavior
is correct, the old was a bug:

as Stas pointed out:
"Not a bug. $var is interpreted as a class name. To know if one class
extends another, the class in question (first one) should be loaded.
There's no need to load the second one since if it's unknown nothing
can be instance of it and existing classes can not extend it."
if you used this previously

from Dmitry:
"Before the patch, is_a() didn't accept string as the first argument
at all, so it always returned "false" and never triggered
__autoload(). The proposed patch didn't revert to original behavior.
It just disables autoloading and may lead to wrong result.

class a {}
class b extends a {}
var_dump(is_a("b", "a")); // it was false before 5.3.7, now it's true

I would say that the old behaviour was wrong, especially because
"instanceof" and is_subclass_of() already implemented support for
string arguments."

so your example was bogus, as passing a non-object as a first
parameter wasn't supported (see http://php.net/is_a) so your code
example depends on an undefined behavior and results in a bogus result
(is_a() alwas returned false if you passed a non-object)

so in a way it is really a BC, but I think that this change is really a bugfix.

--
To unsubscribe, visit:
http://www.php.net/unsub.php

13 years ago by Zeev Suraski — view source

unread

Alan,

I think this feature can certainly be useful, but either way, but again, unless I'm misunderstanding you - you're defining new functionality (support for new types of a first argument) as a BC break. I don't think it's realistic to look at it this way.

And I do think that it's useful, assuming you don't have typos and ask about non-existent classes, or have a broken autoloader - this will work as expected and return true/false properly.

Zeev

-----Original Message-----
From: alan@akbkhome.com [mailto:alan@akbkhome.com]
Sent: Wednesday, August 24, 2011 2:35 PM
To: Zeev Suraski
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

It's not really a bug fix, as the document for is_a() never said it would support
'mixed' first argument. (is_subclass_of is different, it currently says mixed as
the first argument). If it needed fixing, then support for strings should have
been removed, rather than added.

The problem I have is that this 'feature' is not really that usefull, to anyone
AFAIKS. (why would you realistically want to do that kind of
check?) and if you really did need to do such a test, there are other trivial
ways to do it already (which work across versions).

I know it's reusing code at the backend, but it's a trivial change to remove the
BC break, and it does not break anything else. (nobody would have used it that
way yet..).

Regards
Alan

I think there are two ways to look at it:

As a new feature. If I understand you correctly, the fact that beforehand
is_a() was documented to only return TRUE in case the first argument was an
instance of the second argument, means that if we do anything beyond that -
e.g. support strings as the first argument - this means we break compatibility
(please correct me if I misunderstood). IMHO that's not a realistic way to look
at retaining compatibility, and I'm saying that as someone who's almost
religious about maintain compatibility. With that view of the world, almost
every bug fix and literally every feature we add - breaks compatibility.

As a bug fix. Strings were supported even before this change, but they
weren't working properly or consistently with the way classes work
everywhere else in PHP. That was fixed. Just so that we feel good about
ourselves, we also changed undocumented behavior. In case it's not clear, a
situation where is_a("bar", "foo") returns FALSE, even though bar extends foo,
but bar simply doesn't happen to be loaded in memory at the time of the call -
can lead to very real world bugs.

Zeev

-----Original Message-----
From: alan@akbkhome.com [mailto:alan@akbkhome.com]
Sent: Wednesday, August 24, 2011 12:37 PM
To: internals@lists.php.net
Subject: Re: [PHP-DEV] PHP 5.3.8 Released!

Since when has changing documented behaviour and breaking a large
number of applications been acceptable? (Well, happens occasionally
by accident ..)

This was a 'feature change' to is_a(), it was documented as only
returning 'TRUE' when an object was passed as the first object and it
was an instance of that...

I did read through the previous posts, (just caught up the other
day), it looks like if anybody really need to do what this new
feature provides, (which is probably very rare), then

$left == $right || is_subclass_of($left,$right)

should work fine without breaking any code.

Regards
Alan

On Wed, Aug 24, 2011 at 3:57 AM,
alan@akbkhome.com alan@akbkhome.com wrote:

It might have been better to have waited for the is_a() fix to get
sorted out.. - It's a very annoying BC break (changes the
documented behaviour), and now it means we need 4.3.9 in a few more
days.

Let me know if you need help testing / applying etc..

from what I understand, this won't be changed, as the current
behavior is correct, the old was a bug:

as Stas pointed out:
"Not a bug. $var is interpreted as a class name. To know if one
class extends another, the class in question (first one) should be loaded.
There's no need to load the second one since if it's unknown nothing
can be instance of it and existing classes can not extend it."
if you used this previously

from Dmitry:
"Before the patch, is_a() didn't accept string as the first argument
at all, so it always returned "false" and never triggered
__autoload(). The proposed patch didn't revert to original behavior.
It just disables autoloading and may lead to wrong result.

class a {}
class b extends a {}
var_dump(is_a("b", "a")); // it was false before 5.3.7, now it's
true

I would say that the old behaviour was wrong, especially because
"instanceof" and is_subclass_of() already implemented support for
string arguments."

so your example was bogus, as passing a non-object as a first
parameter wasn't supported (see http://php.net/is_a) so your code
example depends on an undefined behavior and results in a bogus
result
(is_a() alwas returned false if you passed a non-object)

so in a way it is really a BC, but I think that this change is really a bugfix.

--
To unsubscribe,
visit:
http://www.php.net/unsub.php

13 years ago by Sebastian Bergmann — view source

unread

It might have been better to have waited for the is_a() fix to get sorted
out.

No, it would have been better if the only difference between PHP 5.3.7
and PHP 5.3.8 would have been the fix for the crypt() issue.

--
Sebastian Bergmann Co-Founder and Principal Consultant
http://sebastian-bergmann.de/ http://thePHP.cc/

13 years ago by Reindl Harald — view source

unread

Am 24.08.2011 10:06, schrieb Sebastian Bergmann:

It might have been better to have waited for the is_a() fix to get sorted
out.

No, it would have been better if the only difference between PHP 5.3.7
and PHP 5.3.8 would have been the fix for the crypt() issue

it was and additionally the break of mysqlnd/mysql-over-ssl was reverted
which killed ALL our setups and forced me to play around with ssh-tunnels
between servers what is nothing you like to do for some months because
you are killing 127.0.0.1-restrictions on the target-machine

Fixed bug #55439 (crypt() returns only the salt for MD5)
Reverted a change in timeout handling restoring PHP 5.3.6 behavior,
which caused mysqlnd SSL connections to hang (Bug #55283).

PHP 5.3.8 Released!

Which, incidentally, doesn't appear to be working at this point in time.

Cheers,

Cheers,

-- Lester Caine - G8HFL

If they just wanted to see if it's an object of class PEAR_Error, instanceof should be used.

Cheers,

--
Lester Caine - G8HFL

If they just wanted to see if it's an object of class PEAR_Error,
instanceof should be used.