illuminate/collections still coupled to illuminate/support

[drbyte]

• illuminate/collections Version: 9.0-dev = dev-master = commit hash d17728b
• PHP Version: 8.0.9

Description:

According to #32478, #32481, #32506 the intent of creating illuminate/collections was to remove the dependency on illuminate/support.
While this appears to be operational in v8.54.0, it is broken in 9.0-dev.

While exploring converting Valet from using tightenco/collect to illuminate/collections with 9.0-dev, it reveals that illuminate/collections 9.0-dev is still highly coupled to illuminate/support

More specifically, the illuminate/collections 9.0-dev currently package depends on (requires) the illuminate/support package to provide the Conditionable trait and its associated Enumerable implementations.

Fatal error: Trait "Illuminate\Support\Traits\Conditionable" not found in .../valet/vendor/illuminate/collections/Traits/EnumeratesValues.php on line 47

Attempting to bypass use Conditionable; exposes more of the dependency details:

PHP Fatal error:  Class Illuminate\Support\Collection contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (Illuminate\Support\Enumerable::when, Illuminate\Support\Enumerable::unless) in .../valet/vendor/illuminate/collections/Collection.php on line 13

Steps To Reproduce:

• git clone git@github.com:laravel/valet.git
• add "minimum-stability": "dev", to composer.json
• composer remove tightenco/collect
• composer require illuminate/collections:^9.0 illuminate/container:^9.0 --with-all-dependencies
• run either ./valet or phpunit

Aside: Changing the ^9.0 to ^8.0 avoids the reported errors, so it seems to be isolated to the 9.0-dev branch.

/cc @mattstauffer
/ref tighten/collect#217

[GrahamCampbell]

Looks like the conditional trait needs to be moved into the collections package?

[driesvints]

Thanks for reporting @drbyte. Seems like #37632 is the culprit here. I left a comment on the PR.

[driesvints]

@GrahamCampbell I'm not sure if moving the trait to the collections package is the right call here. I feel it belongs more in the support package.

[inxilpro]

I can see a couple paths. I think option 1 is probably the cleanest solution, and option 3 is probably the simplest solution. Thoughts?

1. New Package

We could extract the functionality into a illuminate/conditionable package that has the Conditionable trait and the HigherOrderWhenProxy proxy object, and then make this a dependency of the support and collections packages. This is probably the "cleanest" solution, but I don't love introducing a whole new package for such a small bit of functionality.

2. Duplicate Code

We could just duplicate the functionality in both illuminate/support and illuminate/collections. We'd probably leave Conditionable in the support package for use throughout the framework, and just copy-and-paste the code into collections to keep that package separate.

We could reduce the pain of this is a couple of ways:

1. Introduce a contract that is implemented in both packages, and then typehint the contract where possible
2. We may be able to implement a "copied in two places" option thru symlinks, although I'm not sure. The collections package is already a little weird because of the namespace issues, so this might be viable, but I only want to pursue it if that's something y'all would consider.
3. There's maybe a way to implement the code in both places with a class_exists() check so that both packages can hold a copy of the code and whichever is autoloaded first takes precedence. I don't love this, but it would probably work.

3. Move to the collections package

Since illuminate/collections is a dependency of illuminate/support, we could just move the Conditionable trait to the collections package. It doesn't exactly make sense, but it would get the job done.

[JosephSilber]

I'm not sure if moving the trait to the collections package is the right call here. I feel it belongs more in the support package.

@driesvints How's that any different than the HigherOrderWhenProxy?

[driesvints]

@JosephSilber no idea tbh

[inxilpro]

Realistically, Conditionable and HigherOrderWhenProxy don't really belong in the collections package, but unless we want to extract them into their own package like macroable, that's probably the best solution.

@driesvints thoughts?

[inxilpro]

In the end I decided to open up #38457 — it just feels like a cleaner solution. That said, I'm happy to go the other direction if that doesn't end up making sense.

[driesvints]

We split up the trait into its own package: https://github.com/illuminate/conditionable. Thanks all and thanks @inxilpro.