Allow Hash::remove() to remove from ArrayAccess objects #17569
Conversation
ADmad
force-pushed
the
hash-remove-object
branch
from
18c87fd
to
f6143a1
Compare
|
@othercorey Didn't my update get this working as expected? |
|
Oh, maybe? I didn't notice you pushed. I made similar changes and kept running into static analysis errors due to remove() accepting ArrayAccess as a parameter. |
|
My bad, I should have left a comment after updating. I'll look into the static analysis errors. |
ADmad
force-pushed
the
hash-remove-object
branch
3 times, most recently
from
5b3bfe3
to
12afbea
Compare
src/Utility/Hash.php
Outdated
| * @param list<string> $path The path to work on. | ||
| * @param mixed $values The values to insert when doing inserts. | ||
| * @return \ArrayAccess|array data. | ||
| * @psalm-return (T is array ? array : \ArrayAccess) |
There was a problem hiding this comment.
I think this is what I was missing. I was expecting T to evaluate to a specific type in the above union.
src/Utility/Hash.php
Outdated
| @@ -415,6 +420,10 @@ public static function remove(ArrayAccess|array $data, string $path): ArrayAcces | |||
|
|
|||
| [$token, $conditions] = self::_splitConditions($token); | |||
|
|
|||
| if (!is_iterable($data)) { | |||
| throw new CakeException('Cannot use path tokens of type `{}` or `[]` for non-iterable objects.'); | |||
| } | |||
There was a problem hiding this comment.
Ok, this is why I closed the PR originally, I wasn't sure if we had to add ArrayAccess-only iteration and logic.
|
I think this change requires changing insert() to match. |
othercorey
force-pushed
the
hash-remove-object
branch
2 times, most recently
from
8492d73
to
3ed3aed
Compare
|
@ADmad Added support to insert() |
|
LGTM. The psalm errors can be ignored, the iterable check seems to confuse it. |
othercorey
force-pushed
the
hash-remove-object
branch
from
3ed3aed
to
05cd92e
Compare
othercorey
force-pushed
the
hash-remove-object
branch
from
05cd92e
to
7129958
Compare
closes #17542