Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify Collections' interaction with Traversible<object, mixed>, such as WeakMap #49089

Closed
Vectorial1024 opened this issue Nov 22, 2023 · 4 comments · Fixed by #49095
Closed
Assignees
Labels

Comments

@Vectorial1024
Copy link

Laravel Version

10.33

PHP Version

8.1.5

Database Driver & Version

N/A

Description

Laravel has collect() to produce a Collection; currently, the Collection accepts many types, one of which is PHP's Traversible.

It looks fine, but a problem: WeakMap implements Traversible. From the constructor definition/hinting, WeakMap instances are valid Collection "items". But as soon as such Collections are instantiated, a fatal error is always thrown:

TypeError: Illegal offset type.

Clearly, the underlying data structure of the Collection is an array, which obviously cannot accept objects as keys.

This is quite undocumented in the official Laravel docs in the Laravel website. However, the existing Collection constructor did not really check against WeakMaps.

I do know a bit about the covariance-contravariance thing that makes it a bit difficult to specifically reject WeakMap instances during typehinting.

A clarification is very much appreciated:

  • If Collections should not accept WeakMap, then perhaps the docs should be updated with this info; or, a more specific error message could be printed.
  • If Collections are supposed to accept WeakMap, then perhaps the Collection class needs some refactoring.

One very jarring experience with Laravel, is that e.g. I can do filter() on "real array" Collections, but I cannot do filter() on WeakMap instances, even when in native PHP, both can be traversed with something like

function filter(array|WeakMap $original, callable|Closure $predicate)
{
    $results = is_array($original) ? [] : new WeakMap();
    foreach ($original as $key => $value) {
        if ($predicate($value)) {
            $results[$key] = $value;
        }
    }
    return $results;
}

Steps To Reproduce

Using php artisan tinker [file]:

File contents:

<?php

$testMap = new WeakMap();
$testInstance = new stdClass();
$testInstance->haha = 'yes';
$testMap[$testInstance] = 3;

// should the following line be allowed or rejected?
$testCollection = collect($testMap);
// TypeError: Illegal offset type

// unreachable code: unhandled exception thrown
$testCollection->each(fn($x) => var_dump($x));
crynobone added a commit that referenced this issue Nov 23, 2023
…akMap`

fixes #49089

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone
Copy link
Member

Submitted a PR to throw exception when trying to use $testCollection = collect($testMap);

@crynobone crynobone self-assigned this Nov 23, 2023
@driesvints driesvints added the bug label Nov 23, 2023
@Vectorial1024
Copy link
Author

Slow down! The problem is not that trivial.

I did some more research after submitting this, and realized WeakMap is not the only class affected by this clarification. For example, SplObjectStorage is also affected by this, because it instanceof Traversible, but obviously its keys are all objects, and then it cannot be sent into a Collection.

Basically, any class that is Traversible is a potential incompatible class of Collection. To throw more accurate exceptions, we will need to check whether the keys of Traversible is of the pseudotype array-key (basically int|string).

We will never know how many such incompatible classes out there that extends Traversible but have their keys in object type.

Therefore, the question remains:

  • refactor Collections to allow object keys?
  • improve detection of Traversible<object, mixed>?

@Vectorial1024 Vectorial1024 changed the title Clarify Collections' interaction with WeakMap Clarify Collections' interaction with Traversible<object, mixed>, such as WeakMap Nov 23, 2023
@Vectorial1024
Copy link
Author

I noticed this flag in iterator_to_array:

  • preserve_keys

Will setting it to false help with the situation? I see mentions of doing so to give out items inside SplObjectStorage instances.

@crynobone
Copy link
Member

You can't convert WeakMap to an array and we not planning to add support for WeakMap using Collection at the moment.

taylorotwell added a commit that referenced this issue Nov 23, 2023
…akMap` (#49095)

* [10.x] Throw exception when trying to initiate `Collection` using `WeakMap`

fixes #49089

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

* Apply fixes from StyleCI

* wip

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

* Update SupportCollectionTest.php

* Update EnumeratesValues.php

---------

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Co-authored-by: StyleCI Bot <bot@styleci.io>
Co-authored-by: Taylor Otwell <taylor@laravel.com>
taylorotwell added a commit to illuminate/collections that referenced this issue Nov 23, 2023
…akMap` (#49095)

* [10.x] Throw exception when trying to initiate `Collection` using `WeakMap`

fixes laravel/framework#49089

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

* Apply fixes from StyleCI

* wip

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

* Update SupportCollectionTest.php

* Update EnumeratesValues.php

---------

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Co-authored-by: StyleCI Bot <bot@styleci.io>
Co-authored-by: Taylor Otwell <taylor@laravel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants