[RFC] Improve language coherence for the behaviour of offsets and containers

3 months ago by Jakob Givoni — view source

unread

Hello internals,

I would like to formally open the discussion on an RFC I've been working
on for the past year:
https://wiki.php.net/rfc/container-offset-behaviour

Hi Gina,

100% support for this proposal from me!

What a gold mine of research to be found in this delightful RFC :-D

Best,
Jakob

3 months ago by Larry Garfield — view source

unread

Hello internals,

I would like to formally open the discussion on an RFC I've been
working on for the past year:
https://wiki.php.net/rfc/container-offset-behaviour

As DokuWiki is a bit of a faff at times, the Markdown sources are
available on GitHub:
https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

The implementation is basically done, other than some mysterious JIT
issues that I haven't been able to pinpoint yet.

Best regards,

Gina P. Banyard

I want to find time to go through some of the bits in finer detail to be sure, but you've gone through enough finer bits that I doubt I'd find much to object to. :-) Strongly supportive here.

--Larry Garfield

3 months ago by Nicolas Grekas — view source

unread

Hi Gina,

Impressive RFC!

Le jeu. 4 juil. 2024 à 15:55, Gina P. Banyard internals@gpb.moe a écrit :

Hello internals,

I would like to formally open the discussion on an RFC I've been working
on for the past year:
https://wiki.php.net/rfc/container-offset-behaviour

As DokuWiki is a bit of a faff at times, the Markdown sources are
available on GitHub:

https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

The implementation is basically done, other than some mysterious JIT
issues that I haven't been able to pinpoint yet.

I don't see any mention of Stringable objects in the RFC. Can you please
describe how they behave when used as string indexes, and if you're
proposing any changes for them?

Nicolas

3 months ago by Gina P. Banyard — view source

unread

I don't see any mention of Stringable objects in the RFC. Can you please describe how they behave when used as string indexes, and if you're proposing any changes for them

Hello Nicolas,

All objects behave the same for string offset, i.e. they are invalid and will throw an Error.
This is described in the "Invalid offsets" subsection of the "Strings" section. [1]
And I am not proposing any changes to them.

Best regards,
Gina P. Banyard

[1] https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md#invalid-offsets-1

3 months ago by Gina P. Banyard — view source

unread

Hello internals,

I would like to formally open the discussion on an RFC I've been working on for the past year:
https://wiki.php.net/rfc/container-offset-behaviour

As DokuWiki is a bit of a faff at times, the Markdown sources are available on GitHub:
https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

I have more or less finalized the implementation of the object handler part.
The JIT works, and my minimal opcache changes seem to not make everything crash.

I have added one tiny subsection to the RFC about the removal of the zend_class_arrayaccess_funcs struct:
https://wiki.php.net/rfc/container-offset-behaviour#removal_of_the_zend_class_arrayaccess_funcs_struct_and_ce_pointer

One related issue that I found while working on this RFC is related to ext/phar:
https://github.com/php/php-src/pull/14503

A PharFileInfo extends SplFileInfo and supports directories, which SplFileInfo does not.
But Phar also allows the user to set the file info class via the setInfoClass() method.
This method takes any SplFileInfo based class, such as SplFileObject.
Thus, there is currently a bug in the extension where isset() will return true about being able to access a string offset that points to a directory but SplFileObject will throw a LogicException that it cannot be used with a directory.

There are two-way to fix this issue, in my PR I went with the solution to return false for the has_dimension handler on directories if the info class is not based on PharFileInfo,
or we could remove support for setting info classes that are not based on PharFileInfo.

Let me know what you prefer, as my current implementation is based on the above PR to fix the Phar behavioural changes.

Moreover, I know the traffic on the list has been pretty high, but I do intend to have this RFC up for voting for inclusion in PHP 8.4, and I'm not exactly sure how I am meant to interpret the lack of responses.

Best regards,

Gina P. Banyard

3 months ago by Levi Morrison — view source

unread

Moreover, I know the traffic on the list has been pretty high, but I do intend to have this RFC up for voting for inclusion in PHP 8.4, and I'm not exactly sure how I am meant to interpret the lack of responses.

I am personally strongly in favor of the direction. As mentioned in
the PR, my main concern is honestly quite a small one: I think
Appendable::append ought to be renamed. Maybe Appendable and
FetchAppendable too.

The reason is that append is a common operation on a container type,
which is likely to want to implement these interfaces. I easily
identified a few such things with a quick GitHub search:

Given that I anticipate these methods to largely be called by
handlers, and not by names, I think an easy solution is to just name
this offsetAppend to match the other offset operations. For example,
I don't anticipate code doing:

$container->append($item);

I expect largely they will do:

$container[] = $item;

So it doesn't really matter if the name is append or offsetAppend
for the main use-case, and thereby we avoid some road bumps on
adoption. Any SPL containers with append, such as ArrayObject, can
make it an alias of offsetAppend, I think?

Anyway, this is a minor thing, and I will vote yes regardless of
whether it (and maybe the *Appendable interface names) are changed.
But I do think it would be prudent to change it. It would also match
the offset* convention of the other interfaces.

3 months ago by Larry Garfield — view source

unread

Moreover, I know the traffic on the list has been pretty high, but I do intend to have this RFC up for voting for inclusion in PHP 8.4, and I'm not exactly sure how I am meant to interpret the lack of responses.

I am personally strongly in favor of the direction. As mentioned in
the PR, my main concern is honestly quite a small one: I think
Appendable::append ought to be renamed. Maybe Appendable and
FetchAppendable too.

The reason is that append is a common operation on a container type,
which is likely to want to implement these interfaces. I easily
identified a few such things with a quick GitHub search:
1.
https://github.com/pmjones/php-styler/blob/5c7603f420e3a75a5750b3e54cc95dfdbef7d6e2/src/Line.php#L166
2.
https://github.com/ParvulaCMS/parvula/blob/dcb1876bef70caa14d09e212838a35cb29e23411/core/Models/Config.php#L46

Given that I anticipate these methods to largely be called by
handlers, and not by names, I think an easy solution is to just name
this offsetAppend to match the other offset operations. For example,
I don't anticipate code doing:
$container->append($item);
I expect largely they will do:
$container[] = $item;
So it doesn't really matter if the name is append or offsetAppend
for the main use-case, and thereby we avoid some road bumps on
adoption. Any SPL containers with append, such as ArrayObject, can
make it an alias of offsetAppend, I think?

Anyway, this is a minor thing, and I will vote yes regardless of
whether it (and maybe the *Appendable interface names) are changed.
But I do think it would be prudent to change it. It would also match
the offset* convention of the other interfaces.

Based on my research into collections with Derick, I agree that "append" is not a good name to claim for this interface; it would make it incompatible with standard collection method naming. offsetAppend() would neatly side-step that issue. +1 to what Levi said.

As to my limited response so far, it's mostly because I read through the proposal in detail a few months ago when it was first informally put forward and liked it then, and it seems there haven't been any serious changes since for me to comment on. I am very much in favor, though.

--Larry Garfield

3 months ago by Gina P. Banyard — view source

unread

Moreover, I know the traffic on the list has been pretty high, but I do intend to have this RFC up for voting for inclusion in PHP 8.4, and I'm not exactly sure how I am meant to interpret the lack of responses.

I am personally strongly in favor of the direction. As mentioned in
the PR, my main concern is honestly quite a small one: I think
Appendable::append ought to be renamed. Maybe Appendable and
FetchAppendable too.

The reason is that append is a common operation on a container type,
which is likely to want to implement these interfaces. I easily
identified a few such things with a quick GitHub search:

https://github.com/pmjones/php-styler/blob/5c7603f420e3a75a5750b3e54cc95dfdbef7d6e2/src/Line.php#L166

https://github.com/ParvulaCMS/parvula/blob/dcb1876bef70caa14d09e212838a35cb29e23411/core/Models/Config.php#L46

Given that I anticipate these methods to largely be called by
handlers, and not by names, I think an easy solution is to just name
this offsetAppend to match the other offset operations. For example,
I don't anticipate code doing:

$container->append($item);

I expect largely they will do:

$container[] = $item;

So it doesn't really matter if the name is append or offsetAppend
for the main use-case, and thereby we avoid some road bumps on
adoption. Any SPL containers with append, such as ArrayObject, can
make it an alias of offsetAppend, I think?

Anyway, this is a minor thing, and I will vote yes regardless of
whether it (and maybe the *Appendable interface names) are changed.
But I do think it would be prudent to change it. It would also match
the offset* convention of the other interfaces.

Part of the RFC is to deprecate aliases to the dimension handlers, e.g. SplObjectStorage,
because those classes are not final, and you can extend the aliased method and make it behave in whatever way you want.
Which causes unintuitive bugs, because if you overwrite SplObjectStorage::contains(), which is currently considered the "canonical" method,
you don't actually overwrite the behaviour of isset($obj[$index])
The documentation even indicates that offsetExists is an alias of contains() [1]

Considering, one part of the RFC is to get rid of this aliasing, introducing it for append() doesn't make sense to me.

It should be noted, we have other internal classes that have an append() method that is compatible with the current interface design.
One such example is AppendIterator, currently it doesn't implement ArrayAccess because... that doesn't make sense, but it would be quite useful for it to implement the new Appendable interface (or whatever preferred name).
Changing the name of the method on the interface would require introducing more aliases, something that I don't think is wise.
(another class that could benefit from this is the Dom\Element class)

Side note: we had issues in the past with SplFileObject having different methods names for the same thing being aliased, and just thinking about it again makes my brain hurt.

Moreover, the name 'offsetAppend' is somewhat nonsensical, one appends to a container, the naming here implies to me that I am appending to an offset, which to me makes this a bad name.

Finally, the whole point of ArrayAccess is to make container types feel more like a normal array, so it seems perfectly normal for me to have append be the name of it.
Yes there will be some classes that are incompatible with this interface, but I would guess the majority of implementations of an append() method just take a single value, or a variadic number of values to append to the custom container.
Both such custom container type implementations would be able to use the interface without any major changes. [2]
From a quick glance, I can see various classes of Symfony and Laravel that could add an implementation of Appendable, and just widen the parameter type (truly sad the lack of generics) to allow people to use the $container[] = $value syntax with said class.
Requiring a lot of projects to duplicate the implementations of their methods just to support this, and possibly hitting the problem we face with ArrayObject at a larger scale, seems ill-advised to me.

For container types where appending means something more complex, then yes they cannot use the interface, but regardless of name they would not be able to.
And I don't see how them continuing to use "append" as a method name causes problems.

Thus, from my PoV, the only adoption issue is for classes that pass the value by reference to the append() method, which seems to indicate that they need to implement fetchAppend() to grab a reference and then assign the value.

Everything considered, I still do not think changing the name of the append() method is a good idea, will it hurt adoption in the short term for some classes, yes, but I prefer this than having a large part of the ecosystem needing to create an alias just for some weird cases.

Best regards,

Gina P. Banyard

[1] https://www.php.net/manual/en/splobjectstorage.offsetexists.php
[2] https://3v4l.org/E8BcK

3 months ago by Levi Morrison — view source

unread

Everything considered, I still do not think changing the name of the append() method is a good idea, will it hurt adoption in the short term for some classes, yes, but I prefer this than having a large part of the ecosystem needing to create an alias just for some weird cases.

I don't fully understand the issue with aliases. Could you elaborate
on that? Would calling the method instead of direct aliasing avoid
whatever the issue is?

3 months ago by Gina P. Banyard — view source

unread

Everything considered, I still do not think changing the name of the append() method is a good idea, will it hurt adoption in the short term for some classes, yes, but I prefer this than having a large part of the ecosystem needing to create an alias just for some weird cases.

I don't fully understand the issue with aliases. Could you elaborate
on that? Would calling the method instead of direct aliasing avoid
whatever the issue is?

I don't think this would solve the issue, as it is not required for a child class that extends said method to call the parent.

Let's use a class that extends SplObjectStorage as an example: https://3v4l.org/l913b
Our custom class overwrites the behaviour of the contains() method without calling the parent method.
As such, the behaviour of isset() becomes incorrect.

And even if we make offsetExists() call the contains() method, nothing actually prevents the child class from overwriting the offsetExists() method instead of the contains() one.
In which case, the contains() method would still be the original SplObjectStorage one.

One could also just extend both methods and have them differ in implementations.

I really believe there isn't a good way to solve this problem other than not having any aliases within class methods.

Best regards,

Gina P. Banyard

3 months ago by Juliette Reinders Folmer — view source

unread

Moreover, I know the traffic on the list has been pretty high, but I do intend to have this RFC up for voting for inclusion in PHP 8.4, and I'm not exactly sure how I am meant to interpret the lack of responses.

Gina, just as a heads-up: I really want to spend some time to study this
RFC (and some others) in detail and am struggling to keep up (yes, the
traffic has been pretty high over the last month or so).

If I'm honest, I'd say that posting a complex and long RFC like this
with only a little more than a month left before feature freeze may be a
little late in the cycle.
I fear there is not enough time for detailed discussion and that it will
either fail due to the lateness in the cycle or pass because people
trust you, rather than based on the merit of the RFC.

As an aside - not directly related to this RFC - I'm noticing more and
more RFCs which need correction or a follow-up RFC after the vote has
taken place. In my perception, this was much rarer in previous cycles.
I'm not going to hypothesize why this is, but it does make me wonder ...

Smile,
Juliette

3 months ago by Gina P. Banyard — view source

unread

Hello internals,

I would like to formally open the discussion on an RFC I've been working on for the past year:
https://wiki.php.net/rfc/container-offset-behaviour

Hello internals,

Ilija did a review of the RFC and pointed out some good questions and remarks.

I've clarified, and amended the implementation, that ArrayAccess extends DimensionReadable, DimensioWriteable, and DimensionUnsettable.

I've added to the RFC the deprecation of using integer offsets when using ArrayObject with a backing object, as it turns out supporting this adds a lot of complexity in the engine that would be great to remove.

One of the new warnings the RFC introduces is when using a bogus value as a container in isset()/empty() other than null which shortcuts.
This shouldn't cause many issues, except possibly if the container is false, as it may be the result from an internals function call which returns false on failure instead of null.
The question is if false should have the same exemption as null?

The behaviour around the increment and decrement operators is slightly unfortunate.
Incrementing an array offset or object property is treated more like a read-write operation, however, due to the lack of VM opcodes, it is treated like a fetch for object offsets.
After discussion with Ilija it seems wise to make this an Error for now so that we can support it properly in the future, and have the increment observable via the offsetSet() method.

There are still a couple of questions around the behaviour of ArrayObject with property hooks that I need to look into, especially around the current ArrayObject behaviour with ignoring the __set() and __get() magic methods.

The final question, which is somewhat tricky, is how to deal with auto-vivification?
The gist can be explained with the following code:

$obj = new Vector();
$ref = &$obj[0];
var_dump($ref);

$obj[1][] = 'append';
$obj[2][50] = 'set-to-offset';

For arrays, $ref would be NULL
And when nesting them, the "null" auto-magically becomes an array.
However, for objects it is not clear what to do, if the fetch() method creates an object,
then the initial $ref would already have a auto-vivified container.
However, if NULL is returned, then the nested append and offset write would not auto-vivify a nested Vec container, but a standard PHP array.

I currently have a crude prototype where, in the nested dimension case, I instantiate a new object of the same type,
but this doesn't call the constructor (which I could do), but passing arguments to it is impossible.
Another solution is to add another interface, e.g. Autovivificapable, which would define an autovivify() method which would be called in those instances.
Does anyone have any opinions about this?

I have also slightly amended the RFC and added a BC break recap section.
Reminder, the RFC on the Wiki is located here:
https://wiki.php.net/rfc/container-offset-behaviour
And on GitHub here:
https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

Best regards,

Gina P. Banyard

2 months ago by Claude Pache — view source

unread

Le 4 juil. 2024 à 15:52, Gina P. Banyard internals@gpb.moe a écrit :

Hello internals,

I would like to formally open the discussion on an RFC I've been working on for the past year:
https://wiki.php.net/rfc/container-offset-behaviour

As DokuWiki is a bit of a faff at times, the Markdown sources are available on GitHub:
https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

The implementation is basically done, other than some mysterious JIT issues that I haven't been able to pinpoint yet.

Best regards,

Gina P. Banyard

Hi,

Thanks for the RFC. Some small remarks:

In § Add granular interfaces: DimensionUnsetable : I would have written “unsettable” with 2 t’s (like “settable”, “unforgettable”, “regrettable”, and like “unsetting”).
You propose to emit warnings in 8.4 for $array[null], $array[true] and $array[false]. I argue that it should be deprecation notices instead. The reason is that currently no notice is emitted, and that there may be valid code that (implicitly or not) rely on the current behaviour. For example:

function foo(?int $x): string {
    static $cache = [ ];
    return $cache[$x] ??= heavyCalculationForFoo($x);
}

and:

$total_by_type = [ ];
foreach ($list as $row) {
    assert($row['type'] === null || is_string($row['type']) && $row['type'] !== '');
    $total_by_type[$row['type']] ??= 0;
    $total_by_type[$row['type']] += $row['amount'];
}

The case of $array[null] is similar to strlen(null), deprecated in: https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg

Given the new semantics, array_key_exists() should be amended in order to support objects implementing DimensionReadable.

—Claude

2 months ago by Rob Landers — view source

unread

Hello internals,

I would like to formally open the discussion on an RFC I've been working on for the past year:
https://wiki.php.net/rfc/container-offset-behaviour

As DokuWiki is a bit of a faff at times, the Markdown sources are available on GitHub:
https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

The implementation is basically done, other than some mysterious JIT issues that I haven't been able to pinpoint yet.

Best regards,

Gina P. Banyard

I apparently was sitting on a draft in this thread:

Hey Gina,

Is there potentially a 12th type of operation? It is implied multiple times, but not spelled out: "offset exists" (i.e., "array_key_exists()" for arrays). The recent discussion on pattern matching makes it seem like there is potentially a huge difference between "an offset that exists" vs. "an offset with no value"; whether or not that is the case, I believe it is an operation on a container? Maybe. I believe you are the subject-matter-expert on this, by this point, for sure; so you would know. In any case, why or why wouldn't it be considered an operation?

— Rob