PHP-5.5 unpack change broke pecl/pear

12 years ago by Igor Wiedler — view source — reply

unread

Hi Internals,

When test driving PHP-5.5 I ran into issues with a change of unpack behaviour. Archive_Tar which is used by pecl and pear (pecl install) uses unpack with the "a" format character. On 5.4 it strips trailing NUL bytes, on 5.5 it does not. There is a new "Z" format character that can be used to get the old behaviour. The point is, this change broke pecl, and will likely break many other things in existence.

Anthony has asked me to post to internals, so that you can re-visit the issue and perhaps consider not breaking BC.

If you decide that you want to keep the BC break, I will gladly submit a patch for Archive_Tar.

Thanks,

Igor

12 years ago by Sherif Ramadan — view source — reply

unread

This change does not break BC.

The "a" format in pack/unpack functions has not changed at all in 5.5. It
still does the same thing and it was not replaced by the new "Z" format
option, which I added. They do different things.

The "a" format strips null bytes from the string.

The new "Z" format in 5.5 truncates the string at the first null-byte,
returning everything prior to it.

pack/unpack functions should still be backwards-compatible with PHP < 5.5.

12 years ago by Remi Collet — view source — reply

unread

Le 24/12/2012 04:16, Igor Wiedler a écrit :

Hi Internals,

When test driving PHP-5.5 I ran into issues with a change of unpack behaviour. Archive_Tar which is used by pecl and pear (pecl install) uses unpack with the "a" format character. On 5.4 it strips trailing NUL bytes, on 5.5 it does not. There is a new "Z" format character that can be used to get the old behaviour. The point is, this change broke pecl, and will likely break many other things in existence.

Anthony has asked me to post to internals, so that you can re-visit the issue and perhaps consider not breaking BC.

If you decide that you want to keep the BC break, I will gladly submit a patch for Archive_Tar.

See http://pear.php.net/bugs/bug.php?id=19746

Remi.

Thanks,

Igor

12 years ago by Christopher Jones — view source — reply

unread

Le 24/12/2012 04:16, Igor Wiedler a écrit :

Hi Internals,

When test driving PHP-5.5 I ran into issues with a change of unpack behaviour. Archive_Tar which is used by pecl and pear (pecl install) uses unpack with the "a" format character. On 5.4 it strips trailing NUL bytes, on 5.5 it does not. There is a new "Z" format character that can be used to get the old behaviour. The point is, this change broke pecl, and will likely break many other things in existence.

Anthony has asked me to post to internals, so that you can re-visit the issue and perhaps consider not breaking BC.

If you decide that you want to keep the BC break, I will gladly submit a patch for Archive_Tar.

See http://pear.php.net/bugs/bug.php?id=19746

Remi.

Thanks,

Igor

Sherif,

Are you "owning" this issue?
I assigned bug https://bugs.php.net/bug.php?id=63073 to you in case you are.

Chris

--
christopher.jones@oracle.com http://twitter.com/ghrd
Newly updated, free PHP & Oracle book:
http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html

12 years ago by Sherif Ramadan — view source — reply

unread

On Sun, Jan 13, 2013 at 11:35 PM, Christopher Jones <
christopher.jones@oracle.com> wrote:

Le 24/12/2012 04:16, Igor Wiedler a écrit :

Hi Internals,

When test driving PHP-5.5 I ran into issues with a change of unpack
behaviour. Archive_Tar which is used by pecl and pear (pecl install) uses
unpack with the "a" format character. On 5.4 it strips trailing NUL bytes,
on 5.5 it does not. There is a new "Z" format character that can be used to
get the old behaviour. The point is, this change broke pecl, and will
likely break many other things in existence.

Anthony has asked me to post to internals, so that you can re-visit the
issue and perhaps consider not breaking BC.

If you decide that you want to keep the BC break, I will gladly submit a
patch for Archive_Tar.

See http://pear.php.net/bugs/bug.**php?id=19746 http://pear.php.net/bugs/bug.php?id=19746

Remi.

Thanks,

Igor

Sherif,

Are you "owning" this issue?
I assigned bug https://bugs.php.net/bug.php?**id=63073 https://bugs.php.net/bug.php?id=63073to you in case you are.

It was discussed on IRC that this change was applied in order to bring
PHP's implementation of pack() to be more inline with Perl's implementation
(see https://bugs.php.net/61038 for details) and thus why the change was
introduced in PHP 5.5 instead of 5.4, which was already final at the time.
The BC was taken into consideration, but the manual never specified that
this was the defined behavior. So we had made the decision at the time that
this BC would be worth taking in 5.5 for bringing the implementations
inline.

The BC concerns seem to be fiarly limited in in immediate scope to
Archive_Tar, which seems to be a trivial patch (see
http://pear.php.net/bugs/bug.php?id=19746&edit=12&patch=archive_tar_php55.patch&revision=1355241213also)
and Igor
Wiedler seems willing to apply it. Obviously there can more BC out there
with people that rely on this behavior in their existing code, but I can
not gauge this if I just base it on the number of open bugs in the bug
tracker that have to do with pack and PHP 5.5.

So in light of these events the plan is to ask that Archive_Tar be patched
to resolve the make install issue and remain consistent with the Perl
implementation, which also seems consistent with Ruby's implementation of
pack. The spec itself seems a bit ambiguous as noted earlier in this
thread, but I think PHP will benefit more from being consistent with the
other implementations.

Please let me know if there are other BC concerns that have not yet come up.

Chris

--
christopher.jones@oracle.com http://twitter.com/ghrd
Newly updated, free PHP & Oracle book:
http://www.oracle.com/technetwork/topics/php/
underground-php-oracle-manual-**098250.html<http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html

12 years ago by Remi Collet — view source — reply

unread

Le 14/01/2013 06:57, Sherif Ramadan a écrit :

Please let me know if there are other BC concerns that have not yet come up.

Please don't forget that php tree comes with
pear/install-pear-nozlib.phar

This old archive contains Archive_Tar 1.3.7, so should be updated.
(I don't know who still use it, Fedora don't use it for a while)

Remi.

12 years ago by Sherif Ramadan — view source — reply

unread

In that case I was mistaken regarding the new "Z" char. But "a" has been
changed. It is noted in the UPGRADING file.

You're right. It does break BC, but this was a bug fix.
https://bugs.php.net/bug.php?id=61038

The behavior is incorrect in that regard.

12 years ago by Anthony Ferrara — view source — reply

unread

Sherif,

You're right. It does break BC, but this was a bug fix.

https://bugs.php.net/bug.php?id=61038

The behavior is incorrect in that regard.

However in this case that bug does have fairly significant BC
implications.

In fact, how I read the specification is that the original binary format is
NULL padded, not the resulting string. Therefore, you could make the
argument that the new behavior is wrong.

In any case, I'd suggest reverting the "bug fix" for "a", and then adding Z
which keeps the raw bytes. So that way the "bug" would be fixed by the "Z"
parameter, but "a" would still behave as it always has (allowing things
like Archive_Tar to still work properly).

Otherwise it's a BC break that has weird implications (having different
format specifications for 5.4 and 5.5, or requiring odd things like running
trim on the output...

My $0.02...

Anthony

12 years ago by Gustavo Lopes — view source — reply

unread

On Mon, 24 Dec 2012 12:13:35 -0000, Anthony Ferrara ircmaxell@gmail.com
wrote:

You're right. It does break BC, but this was a bug fix.

https://bugs.php.net/bug.php?id=61038

The behavior is incorrect in that regard.

However in this case that bug does have fairly significant BC
implications.

In fact, how I read the specification is that the original binary format
is NULL padded, not the resulting string. Therefore, you could make the
argument that the new behavior is wrong.

The Perl docs say:

´A´ strips trailing whitespace and nulls, ´Z´ strips everything after
the first null, and ´a´ returns data with no stripping at all.

PHP docs are silent on this matter.

Yes, PHP does not have to follow Perl's specification, but

PHP's pack/unpack was clearly modeled after Perl
is desirable to have close semantics to Perl's implementation as it
benefits those who use both languages like the submitter of #61038

Given 1), perl's docs and implementation and the absence of evidence to
the contrary (like PHP tests and documentation), it's clear to me that
pre-5.5 behavior is a bug (no quotes).

Now you may think that the cost of fixing this bug does not cover its
benefits, possibly given alternative solutions.

What I can say about that is that I think that the alternative solution
that you propose (switching the behavior of "a" and "Z") would be trading
a relatively minor short term inconvenience (this will affect several
orders of magnitude less people than the htmlentities() change in 5.4) for
one that will exist indefinitely and I consider that a poor choice.

--
Gustavo Lopes

12 years ago by Sherif Ramadan — view source — reply

unread

On Mon, Dec 24, 2012 at 7:13 AM, Anthony Ferrara ircmaxell@gmail.comwrote:

Sherif,

You're right. It does break BC, but this was a bug fix.

https://bugs.php.net/bug.php?id=61038

The behavior is incorrect in that regard.

However in this case that bug does have fairly significant BC
implications.

In fact, how I read the specification is that the original binary format
is NULL padded, not the resulting string. Therefore, you could make the
argument that the new behavior is wrong.

In any case, I'd suggest reverting the "bug fix" for "a", and then adding
Z which keeps the raw bytes. So that way the "bug" would be fixed by the
"Z" parameter, but "a" would still behave as it always has (allowing things
like Archive_Tar to still work properly).

Otherwise it's a BC break that has weird implications (having different
format specifications for 5.4 and 5.5, or requiring odd things like running
trim on the output...

My $0.02...

Anthony

Hmm, the intent behind the fix was to get the behavior more inline with
Perl's pack/unpack as in PHP they weren't behaving the same way. Apparently
this has had some implications in the pecl/pear installer that I was not
aware of. The BC break can be reverted of course, but the idea was to only
put this patch in 5.5 because of the BC concern. Now that I recall the
situation I'm not sure what the best play is here, but I'll review this
again and see if this is indeed inline with the spec or not.

12 years ago by Daniel Convissor — view source — reply

unread

Hi:

In that case I was mistaken regarding the new "Z" char. But "a" has been
changed. It is noted in the UPGRADING file.

You're right. It does break BC, but this was a bug fix.
https://bugs.php.net/bug.php?id=61038

And the fix created another bug:
https://bugs.php.net/bug.php?id=63073

--Dan

--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335