Everything

php

Fwd: Re: #25494 [Com]: array_merge allowing "false" as argument (silent when non-array is passed)

php.internals

Jan Schneider

22 years ago

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.

On Tue, 23 Sep 2003, jan at horde dot org wrote:

> 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

J Smith

22 years ago

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:

Wez Furlong

22 years ago

Hi Jay,

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

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() or array_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.

J Smith

22 years ago

Wez Furlong wrote:

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

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

Ilia A.

22 years ago

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

Jan Schneider

22 years ago

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

J Smith

22 years ago

Jan Schneider wrote:

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

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

Jan Schneider

22 years ago

Zitat von Jay Smith <jay@php.net>:

> Jan Schneider wrote:
>
> >
> > 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.
> >
>
> 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

22 years ago

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

J Smith

22 years ago

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

Mike Robinson

22 years ago

Jay Smith wrote:

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

Notwithstanding its very unsafe to make such an assumption, the minority
of setups affected by this change could still be in the thousands, tens
or even hundreds of thousands.

IMHO, if the function behaviour hasn't changed, there should be no notice
warning about possible future changes. Thats what documentation and
announcements
are for.



Regards

Mike Robinson
http://fiddy8.com/

Brad Fisher

22 years ago

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

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

Stefan Walk

22 years ago

On Tue, Sep 23, 2003 at 11:44:29AM -0400, Jay Smith wrote:

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

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() or array_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>