Everything

php

[RFC] E_NOTICE for invalid array container

php.internals

David Walker

10 years ago

Hi All,

I'm attempting to get involved and hack away at some internals; I figured
the best method to do this would be to pick up a bug and do my best at it.
So I grabbed #37676.  The issue asks to implement an E_(NOTICE|WARNING)
when attempting to access scalar values as arrays.  cmb alerted me to a
long thread that came up last year (
http://marc.info/?t=143379796900001&r=1&w=2).  I've read over it and the
couple of other PR's that attempted to resolve this issue.

As I read, the couple of major complaints about adding an E_NOTICE were
that multiple-accesses in a set (i.e. $a=false;$a[1][2][3];) would produce
too many errors.  Matt
<http://marc.info/?l=php-internals&m=143384434418881&w=2> raised this
issue, in addition to it only being raised for non-null issues; having NULL
be an 'identity' of sort that any array access on a NULL is NULL.  I agree
with him in part, and oppose in others.  I think that original access to a
NULL should be treated as accessing any scalar value, however I do agree
that multiple notices would be bad.  Rowan
<http://marc.info/?l=php-internals&m=143385309121593&w=2> has also
presented his thoughts to the same, and this implementation takes into
account his solution where we don't raise much.

Christoph <http://marc.info/?l=php-internals&m=143385339521672&w=2> countered
that: ```$o->foo->bar``` would itself raise two notices rather than one.  I
would present that that behavior should be pruned itself. Object access on
a null should alert, but subsequent calls ought not to (I don't know how to
prevent that, but then again I haven't learned).


So I want to solicit for more discussion given previous talks.  My
first-attempt RFC for this is at:
https://wiki.php.net/rfc/notice-for-non-valid-array-container

--
Dave

ps: this is my first attempt here at an RFC solo, and preemptively
apologize if anything deviates from the norm too far, did my best to follow
all them how-to's.

Niklas Keller

10 years ago

2016-07-31 21:31 GMT+02:00 David Walker <dave@mudsite.com>:

> Hi All,
>
> I'm attempting to get involved and hack away at some internals; I figured
> the best method to do this would be to pick up a bug and do my best at it.
> So I grabbed #37676.  The issue asks to implement an E_(NOTICE|WARNING)
> when attempting to access scalar values as arrays.  cmb alerted me to a
> long thread that came up last year (
> http://marc.info/?t=143379796900001&r=1&w=2).  I've read over it and the
> couple of other PR's that attempted to resolve this issue.
>
> As I read, the couple of major complaints about adding an E_NOTICE were
> that multiple-accesses in a set (i.e. $a=false;$a[1][2][3];) would produce
> too many errors.  Matt
> <http://marc.info/?l=php-internals&m=143384434418881&w=2> raised this
> issue, in addition to it only being raised for non-null issues; having NULL
> be an 'identity' of sort that any array access on a NULL is NULL.  I agree
> with him in part, and oppose in others.  I think that original access to a
> NULL should be treated as accessing any scalar value, however I do agree
> that multiple notices would be bad.  Rowan
> <http://marc.info/?l=php-internals&m=143385309121593&w=2> has also
> presented his thoughts to the same, and this implementation takes into
> account his solution where we don't raise much.
>
> Christoph <http://marc.info/?l=php-internals&m=143385339521672&w=2>
> countered
> that: ```$o->foo->bar``` would itself raise two notices rather than one.  I
> would present that that behavior should be pruned itself. Object access on
> a null should alert, but subsequent calls ought not to (I don't know how to
> prevent that, but then again I haven't learned).
>
>
> So I want to solicit for more discussion given previous talks.  My
> first-attempt RFC for this is at:
> https://wiki.php.net/rfc/notice-for-non-valid-array-container
>
> --
> Dave
>
> ps: this is my first attempt here at an RFC solo, and preemptively
> apologize if anything deviates from the norm too far, did my best to follow
> all them how-to's.
>

Big +1. We came across that some days ago again.

Regards, Niklas

Dan Ackroyd

10 years ago

Hi David,

On 31 July 2016 at 20:31, David Walker <dave@mudsite.com> wrote:

> So I want to solicit for more discussion given previous talks.  My
> first-attempt RFC for this is at:
> https://wiki.php.net/rfc/notice-for-non-valid-array-container
>

I've very likely to support this RFC, however the section on 'Backward
Incompatible Changes' needs to be filled out, as this RFC would make
code that currently emits no notices, start emitting notices.

cheers
Dan

David Walker

10 years ago

Hi Dan,

I left it blank to go along with default production ini-settings where
notices would be excluded. However, you are probably correct that it should
be noted for development environments as something new. I'll toss it in
there in the morning

Thanks

--
Dave
On Mon, Aug 1, 2016 at 01:14 Dan Ackroyd <danack@basereality.com> wrote:

Dan Ackroyd

10 years ago

Hi David,

The updated section still doesn't really describe the BC break that well.

Some people will need to change their code to avoid having notices
raised frequently. How difficult that will be needs to be thought
about.

Additionally, although the default ini files may turn off notices in
production environment, I really don't believe that is best practice.
For my machines, in both production and dev all warnings and notices
(except deprecation notices in production) are converted into
exceptions. Anything else allows mistakes in code to go undetected.

cheers
Dan

David Walker

10 years ago

Dan,

You're right on both accounts; also, as I just notices it could easily have
other BC-breaks (although expecting errors in a certain format is odd)
through calls like error_get_last(), which would regardless of INI settings
include this NOTICE.  I will update the RFC with these changes.

--
Dave

On Mon, Aug 1, 2016 at 4:27 PM Dan Ackroyd <danack@basereality.com> wrote:

David Walker

10 years ago

All,

I updated the RFC, and made a couple updates to tests where this notice is
raising itself (fpm & mysqli).  I ended up catching a couple of oddities in
how mine (and previous proposed) changes would have impacted a
foreach(list() = each()), so it won't raise a warning on the last, null,
element of each.

Before I ask for voting, I do have a best-practices question I hope someone
would have an answer to.  My dev setup doesn't include all the extensions,
nor do I have a copy of windows for testing.  What would be the best way to
ensure all tests are run so I can find other locations where this warning
might be adversely impacting some of the tests?  I know Travis will run
tests when I create a PR, but I don't know if it is ensuring all tests are
fully run either.

Thanks

--
Dave

On Tue, Aug 2, 2016 at 12:33 PM David Walker <dave@mudsite.com> wrote: