Hi,
the recent change to array_merge that now checks for IS_ARRAY breaks BC IMO.
At least I know get a lot of E_NOTICEs everywhere.
ID: 25494
Comment by: jan at horde dot org
Reported By: enygma at phpdeveloper dot org
Status: Bogus
Bug Type: Arrays related
Operating System: Any
PHP Version: 4.3.2
New Comment:This fix also breaks BC in the way that previously allowed (and
working) NULLs being passed as arguments, now produce an unnecessary
E_NOTICE.
Jan.
--
http://www.horde.org - The Horde Project
http://www.ammma.de - discover your knowledge
http://www.tip4all.de - Deine private Tippgemeinschaft
Is this fix really causing this much grief? Throwing an E_NOTICE isn't a BC
break. It still works as before, it just throws the E_NOTICEs now. This was
meant to be a bridge to the behaviour used in PHP 5, which, like other
array_*() functions, doesn't work on non-arrays at all. (Although that fix
for PHP 5 is debatable, I suppose.)
The function is called array_merge(), not null_merge() or string_merge().
The change was to make it act more like other array functions, like
array_intersect() or array_sum(), which also check parameters for arrays.
What's the consensus? Keep the change or revert? Personally, I think it's
more consistent if it acts like the other array functions, but if it's
causing a lot of headaches...
Just a thought -- perhaps keep it in for the first 4.3.4 RC and gauge the
reaction and go from there?
J
Jan Schneider wrote:
Hi,
the recent change to array_merge that now checks for IS_ARRAY breaks BC
IMO. At least I know get a lot of E_NOTICEs everywhere.
Hi Jay,
Is this fix really causing this much grief? Throwing an
E_NOTICEisn't a
BC
break. It still works as before, it just throws the E_NOTICEs now.
yes and yes :)
Smart people (like Jan) write code that doesn't cause E_NOTICEs.
Now they are faced with code that worked flawlessly before but raises all
kinds of errors now.
Before you say that "errors shouldn't be turned on in production", don't
forget that smart people do turn off the errors, and have them go into
an error log instead.
Some people do magic and get mailed or paged when errors happen.
This was
meant to be a bridge to the behaviour used in PHP 5, which, like other
array_*() functions, doesn't work on non-arrays at all. (Although that fix
for PHP 5 is debatable, I suppose.)
IF someone decides to upgrade to PHP 5, let them worry about PHP 5
behaviour
then.
The function is called
array_merge(), not null_merge() or string_merge().
The change was to make it act more like other array functions, like
array_intersect()orarray_sum(), which also check parameters for arrays.
PHP is a loosely typed language.
You've suddenly made it more strict in a minor release.
Behaviour changing patches are not suitable for the stable branch.
What's the consensus? Keep the change or revert? Personally, I think it's
more consistent if it acts like the other array functions, but if it's
causing a lot of headaches...
Revert - it will cause hundreds more people lots more headaches than you
were having on your own :)
Just a thought -- perhaps keep it in for the first 4.3.4 RC and gauge the
reaction and go from there?
No :)
RC means release candidate, which means "this is what we are going to
release
if no one finds bugs".
Actually, this is Ilia's decision, but I think we should not raise any
notices
for NULL values, for the sake of BC. It really does suck to break code that
was working with no problems in a minor release.
If the change had been made for 4.3, or 4.4 or 5, it wouldn't matter quite
so much.
But to change it for 4.3.4 !?
--Wez.
Wez Furlong wrote:
Actually, this is Ilia's decision, but I think we should not raise any
notices forNULLvalues, for the sake of BC. It really does suck to break
code that was working with no problems in a minor release.If the change had been made for 4.3, or 4.4 or 5, it wouldn't matter quite
so much.
But to change it for 4.3.4 !?--Wez.
Point taken. I've got no great attachment to the "fix", although I do prefer
the consistency with the other array functions.
Would checking for IS_ARRAY and IS_NULL be too much of a kludge?
J
In my opinion the change is fine, given the current state of affairs a
transitional release between 4.3 & 5.0 does not seem likely. Therefor it
would only seem logical to give people a fair warning (E_NOTICE) that the
(wrong) behavior they are relying upon is not going work/last forever.
Otherwise, without a word of warning working code will suddenly become broken
code once they upgrade.
Most people have their error reporting set to E_ALL ^ E_NOTICE (do not display
notices) so it won't affect them anyway. Even then, the behavior itself
implies a failure in some code, since an non-array value is passed to a
function expecting an array. I venture it would help a number of people find
errors in their code that before were nigh impossible to find and/or track
down.
And yes PHP is typeless, but all of the code using new argument parsing
functions will most definitely reject strings passed to functions expecting
arrays and vice versa. I see this situations as not being any different.
Ilia
Zitat von Ilia Alshanetsky ilia@prohost.org:
In my opinion the change is fine, given the current state of affairs a
transitional release between 4.3 & 5.0 does not seem likely. Therefor it
would only seem logical to give people a fair warning (E_NOTICE) that the
(wrong) behavior they are relying upon is not going work/last forever.
Otherwise, without a word of warning working code will suddenly become
broken
code once they upgrade.
But they would expect their code to (potentially) break if they upgrade to
PHP 5.0. And it actually will, despite the efforts to make 5.0 as BC as
possible.
Most people have their error reporting set to
E_ALL^E_NOTICE(do not
display
notices) so it won't affect them anyway. Even then, the behavior itself
implies a failure in some code, since an non-array value is passed to a
function expecting an array. I venture it would help a number of people
find
errors in their code that before were nigh impossible to find and/or
track
down.
I generally agree (this is the purpose of E_NOTICE after all), but there is
a subtle difference between what has been fixed and what is broken now.
Passing NULLs to array_merge didn't lead to the borked arrays that have
been "fixed" by this patch.
And yes PHP is typeless, but all of the code using new argument parsing
functions will most definitely reject strings passed to functions
expecting
arrays and vice versa. I see this situations as not being any different.
Does this apply to NULL "values" too?
Jan.
--
http://www.horde.org - The Horde Project
http://www.ammma.de - discover your knowledge
http://www.tip4all.de - Deine private Tippgemeinschaft
Jan Schneider wrote:
I generally agree (this is the purpose of
E_NOTICEafter all), but there
is a subtle difference between what has been fixed and what is broken now.
Passing NULLs to array_merge didn't lead to the borked arrays that have
been "fixed" by this patch.
How are the arrays borked? The patch doesn't touch, skip or otherwise do
anything to any of the parameters, it just does a type check. Am I missing
something here? (Which is quite possible...)
Or are you talking about 5.0?
J
Zitat von Jay Smith jay@php.net:
Jan Schneider wrote:
I generally agree (this is the purpose of
E_NOTICEafter all), but
there
is a subtle difference between what has been fixed and what is broken
now.
Passing NULLs to array_merge didn't lead to the borked arrays that have
been "fixed" by this patch.How are the arrays borked? The patch doesn't touch, skip or otherwise do
anything to any of the parameters, it just does a type check. Am I
missing
something here? (Which is quite possible...)
Ah, well, I misread the original bug report. With "borked" I meant that
array_merge(false, array("foo" => "bar")) resulted in array(0 => false,
"foo" => "bar"). I though this was changed by the patch.
Anyway, array_merge(array("foo" => "bar"), null) was never producing such an
array and should thus not result in an E_NOTICE.
Jan.
--
http://www.horde.org - The Horde Project
http://www.ammma.de - discover your knowledge
http://www.tip4all.de - Deine private Tippgemeinschaft
I love it when threads silently die. ;-)
Will this problem actually adressed by anyone or will we again have to
release new versions of our software just because a minor PHP came out or
deal with a huge amount of user complaints?
Zitat von Jan Schneider jan@horde.org:
Zitat von Jay Smith jay@php.net:
Jan Schneider wrote:
I generally agree (this is the purpose of
E_NOTICEafter all), but
there
is a subtle difference between what has been fixed and what is broken
now.
Passing NULLs to array_merge didn't lead to the borked arrays that
have
been "fixed" by this patch.How are the arrays borked? The patch doesn't touch, skip or otherwise
do
anything to any of the parameters, it just does a type check. Am I
missing
something here? (Which is quite possible...)Ah, well, I misread the original bug report. With "borked" I meant that
array_merge(false, array("foo" => "bar")) resulted in array(0 => false,
"foo" => "bar"). I though this was changed by the patch.Anyway, array_merge(array("foo" => "bar"), null) was never producing such
an
array and should thus not result in an E_NOTICE.
Jan.
--
http://www.horde.org - The Horde Project
http://www.ammma.de - discover your knowledge
http://www.tip4all.de - Deine private Tippgemeinschaft
Jan Schneider wrote:
I love it when threads silently die. ;-)
Will this problem actually adressed by anyone or will we again have to
release new versions of our software just because a minor PHP came out or
deal with a huge amount of user complaints?
Yeah, it kind of just trailed off at then end there, didn't it...
As near as I can tell, leaving it in seems to be the way to go at the
moment. Being as they're E_NOTICEs and not full-on E_WARNINGs and the fact
that the actual behaviour of the function hasn't changed (except, of
course, the E_NOTICEs), it seems to be a good reminder that the
undocumented behaviour being relied upon is subject to change. (And will
change in 5, for that matter.)
I'd imagine that most setups ignore E_NOTICEs, so most people won't even
notice. (No pun intended, of course.)
If there's anybody else who wants to take up the issue they're more than
welcome to. I prefer leaving it in, but maybe that's just me. (Well, it's
not just me, actually, but you know what I mean.)
J
Jay Smith wrote:
The function is called
array_merge(), not null_merge() or string_merge().
The change was to make it act more like other array functions, like
array_intersect()orarray_sum(), which also check parameters for arrays.What's the consensus? Keep the change or revert? Personally, I think it's
more consistent if it acts like the other array functions, but if it's
causing a lot of headaches...
I see no problem with the array_{merge|intersect|diff|flip|etc.} functions
taking non-array parameters, since they all return the resulting array. As
long as they don't promote a non-array variable passed as a parameter into an
array as a side-effect, I don't see the problem. I'd say revert the change and
keep it as it was.
-Brad
Is this fix really causing this much grief? Throwing an
E_NOTICEisn't a BC
break. It still works as before, it just throws the E_NOTICEs now. This was
meant to be a bridge to the behaviour used in PHP 5, which, like other
array_*() functions, doesn't work on non-arrays at all. (Although that fix
for PHP 5 is debatable, I suppose.)
Well, array_splice() accepts non-array arguments in the same manner that
array_merge did before, so it's as inconsistent as it was before the
'fix'.
The function is called
array_merge(), not null_merge() or string_merge().
So why does str_repeat(10,10) work? It's not called number_repeat.
PHP is a dynamically typed language, if a function gets an argument that
does not "fit" i expect it to cast, if it does make sense in any way...
and merging a single element into an existing array, or merging two
elements to form a new array does make sense IMHO.
Of course, if a function expects a resource, a cast doesn't make much
sense - except i could think of a few cases where it could fopen() and
fclose() a resource on the fly. Same with objects.
Btw, the default (string)$somearray output "Array" is not satisfactory
IMO, i think a join('',$array) or something like that would be more
useful - that would have the benefit of actually being useful ;)
The change was to make it act more like other array functions, like
array_intersect()orarray_sum(), which also check parameters for arrays.
Like i said, array_splice() acts like the old array_merge()...
And this behaviour is documented.
I think adding strict checks is the wrong way, because it breaks BC (at
least it fills error logs). If a consistent behaviour is desired,
typecasts should be added to the functions which don't support it.
What's the consensus? Keep the change or revert? Personally, I think it's
more consistent if it acts like the other array functions, but if it's
causing a lot of headaches...
As you might have guessed, i'm in favour of reverting it ;)
--
Regards,
Stefan Walk
<swalk@prp.physik.tu-darmstadt.de