- πΊπΈUnited States smustgrave
Seems to have 8.2 failures. Should that be hold up or no?
- Status changed to Needs review
almost 2 years ago 7:44am 16 February 2023 - πΊπ¦Ukraine voleger Ukraine, Rivne
No, these were unrelated issues during the test run. The patch contains new additions only.
-
+++ b/core/lib/Drupal/Component/Utility/ArrayObject.php @@ -0,0 +1,100 @@ + public function set(mixed $key, mixed $value): void { ... + public function offsetSet(mixed $offset, mixed $value): void {
I suggest using union type argument declarations:
`?string|?int` for `$key` argument -
+++ b/core/lib/Drupal/Component/Utility/ArrayObject.php @@ -0,0 +1,100 @@ + public function offsetExists(mixed $offset): bool { ... + public function offsetGet(mixed $offset): mixed { ... + public function offsetUnset(mixed $offset): void {
I suggest using a union type argument declaration:
`string|int`
Also, why not introduce the method chaining support for setter methods?
-
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Thanks for kicking this issue along!
`?string|?int`
is not valid as you can't mix union types with nullable types. Addedstring|int $key
for get and set methods instead.- We cannot change the method signatures for methods implementing the
\ArrayAccess
interface.
Added a fluid setter and bumped to 10.1.x
- Status changed to RTBC
almost 2 years ago 2:23pm 17 February 2023 - πΊπΈUnited States smustgrave
Change looks like a good consistentcy addition
- Status changed to Needs work
almost 2 years ago 2:32pm 17 February 2023 - π¬π§United Kingdom catch
Are any of the issues that reference this one planning to use it? There's no indication as such in the issue summary. Without a concrete use-case, it won't help with conversions. For example the builder pattern issue provides a class that creates a render array, it doesn't object-ify the render array itself, so it won't help there.
Additionally, if we added something like this, I don't think we should call it ArrayObject because that already exists in SPL.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
I came here from #2024043-53: Add Module, Theme, Profile, and Extension value objects β where @larowlan suggested this. I think we need a base object that can support the existing arrays that are currently a dumping ground.
Maybe we can call this
BaseArrayObject
to make that more clear? - Issue was unassigned.
- last update
over 1 year ago 29,407 pass - π©πͺGermany donquixote
I am skeptical about the use of ArrayAccess objects.
I also don't like these grand base classes, which tend to give their children more capabilities than they need.
E.g. would the extension info classes really need getIterator()?I would want to see examples how this can improve DX elsewhere - see also my comment in π Add Module, Theme, Profile, and Extension value objects Needs work .
We can create proof-of-concept branches that show how this would be used.
(test cases alone are not sufficient for this)
So +1 to @catch in #28.I did recently post a proposal involving ArrayAccess in slack, but this was for a very niche and controversial case to allow
$cache[$cid] ??= ...
, very different from what we do here.Btw, plain php arrays _do_ have some benefits over general-purpose ArrayAccess objects:
A php array is passed by value, and is therefore in a way "immutable": If you pass it around to other functions, your own copy of the array won't be manipulated. The same cannot be said about mutable ArrayAccess objects.
Also there are operations and functions for arrays that don't work with ArrayAccess objects.The DX with arrays can be improved with phpstan/psalm advanced types.
I do like value objects, but these work best with a known and limited list of properties.
Or, if we do want to support custom properties in addition to known properties, for the custom ones we can simply have ->get($key) instead of [$key].
This way, when reading the code, we know it is an object.The only other reason for ArrayAccess might be for BC support, as a smooth replacement for arrays.
But this is already limited if the code we want to support uses array type hints, or things like preg_grep() etc.