On Monday, 11 March 2024 at 16:11, Larry Garfield <la...@garfieldtech.com> wrote:
> > Woof. That's my kind of RFC. :-) The extensive background helps a lot, thank > you. > > I am generally in favor of this, and have wanted more fine-grained > ArrayAccess interfaces for a long time. > > Thoughts in no particular order: > > * "Dimension" is clearly based on the existing engine hooks, but as a > user-space dev, it's a very confusing term. Possibly documentable if there's > an obvious logic for it we could present, but something more self-evident is > probably better. I am open to naming changes, but "dimension" is very much the standard and ubiquitous term for this, see multidimensional arrays, n-dimensional array, etc. > > * I am not sure combining get and exists into a single interface is right. > I'm not certain it's wrong, either, though. What is the argument for > combining them? > > * Do we have some guidance for what offsetGet() should do when offsetExists() > is false? I know there isn't really any now, but it seems like the sort of > thing to think about here. I will answer both points together. The reason that checking an offset exists is combined with being able to read it is that it just makes sense to be together. If not and only the "read" operation is supported, how do you know that an offset actually exists before accessing it? You just access it and get an Exception that you need to catch? It is also required for the null coalesce operator to function properly. What offsetGet() does if the offset doesn't exist is up to the implementation, you could return null as a default value or throw an exception. The only expectation is that *if* the offset exists, then reading the value should be possible. If you can only write to a container, then being able to check something exists is somewhat meaningless. > > * You sort of skirt around but don't directly address the impact of this > change on the long-standing desire to make array functions accept arrayified > objects. What if any impact would this have there? Eg, could some array > functions be made to accept array|DimensionRead without breaking the world? > > * How, if at all, does this relate to iterable? I think it has no impact, but > since it's in the same problem space it's worth confirming. The former is actually also more related to iterable, as most array functions need to be able to traverse the container to be able to do anything meaningful with it. Or there would need to be a way to provide a "manifest" of what offsets are set for things like array_search or array_flip. > > * You mention at one point applying shim code ArrayAccess to make it work > like the new interfaces. Please expand on that. Do you mean the engine would > automatically insert some shims, like how `__toString()` now magically > implies `implements Stringable`? Or some trait that objects could use (either > a user-space trait or an engine trait) that would provide such shims in an > overridable fashion? I don't fully follow here. > > * If I read correctly, an internal object that implements one of the > dimension handlers will automagically appear to user-land as having the > corresponding interface, much like `__toString()`/`Stringable`. Is that > correct? It seemed implied but not fully stated. If so, a brief code example > would help to make it clear in such a long RFC. Move around a later point to respond to them together. Internal objects don't actually magically implement Stringable, this is something internal objects must do themselves. Moreover, internals objects can support string casts without implementing __toString(), see the GMP object which is the only example of this in php-src (and I should fix this or if someone else wants an easy PR to do feel free). To understand how the shim works, I first need to explain how interfaces being implemented in a class work. Internal interfaces can have a special handler called interface_gets_implemented which gets called during compilation of classes. This is the mechanism used by the Throwable and the DateTimeInterface interfaces to prevent userland classes from implementing them. The ArrayAccess interface has (and all the other Dimension ones actually have) such a handler, and the "shim" is to set the append, fetch, and fetch-append dimension handlers on the CE to magically support those operations on the class for the time being. No methods are created on the class, the new interfaces are not implemented. To override the behaviour of append/fetch/fetch-append the relevant new interface should be implemented. This is conceived as a temporary measure to ease migration for userland classes that support those operations already. > > * Big +1 to removing the magic semi-silent casting when using weird key types. > > * I feel like some of the sections could benefit from more short code > examples. Eg, What the heck does fetch-append on a null even look like? That > would help illustrate why the current behavior is weird, or why some things > need to stay non-obvious because they're used in odd cases. (Like how > $a[1][2] is a by-ref fetch internally, something most people don't think > about.) > I find having too many examples makes RFCs difficult to read and parse, and I prefer to use them sparingly for when they are really needed. Just for clarity but $a[1][2] is only a by-ref fetch for write operations, if it is a read operation those are performed in sequence. Fetch-append on null is not really weird, it just appends the given value (/null if no assignments happens) and provides a reference to it. See https://3v4l.org/UPg3P > * What would the changes to ArrayObject mean for a backing object that uses > hooks on some properties? It says __get/__set would be bypassed. Would hooks > also be bypassed? (I have no clue what the preferred logic here is, honestly. > Just thinking aloud and hoping hooks pass so that we have to think about > this. :-) ) The current behaviour by-passes __set() and __get() because it works with the property HashTable directly. My current PR to fix all those weird behaviours is to use the write_property object handler with ZEND_GUARD_PROPERTY_SET guard set. I don't know how hooks are implemented so Ilija should be able to determine how this works exactly, but my guess is that it should "just work" with set hooks being called. > > Again, thank you for your work on this, and I hope it passes easily. > > --Larry Garfield Hope I've answered the questions you had. Best regards, Gina P. Banyard
