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

[5.6.8] Not able to use tightenco/collect #23420

Closed
mpociot opened this issue Mar 7, 2018 · 9 comments
Closed

[5.6.8] Not able to use tightenco/collect #23420

mpociot opened this issue Mar 7, 2018 · 9 comments

Comments

@mpociot
Copy link
Contributor

mpociot commented Mar 7, 2018

  • Laravel Version: 5.6.8

Description:

Because we now treat tightenco/collect as a conflict (PR #23379) composer installs both packages and adds them to the autoloader.

The problem is that the autoloader sequence is wrong and puts tightenco/collect in front, therefore loading the tightenco/collect helper methods instead of Laravel's helpers.

'fe62ba7e10580d903cc46d808b5961a4' => __DIR__ . '/..' . '/tightenco/collect/src/Collect/Support/helpers.php',
'caf31cc6ec7cf2241cb6f12c226c3846' => __DIR__ . '/..' . '/tightenco/collect/src/Collect/Support/alias.php',
'f0906e6318348a765ffb6eb24e0d0938' => __DIR__ . '/..' . '/laravel/framework/src/Illuminate/Foundation/helpers.php',
'58571171fd5812e6e447dce228f52f4d' => __DIR__ . '/..' . '/laravel/framework/src/Illuminate/Support/helpers.php',

Because of this, the wrong collection classes will be passed internally:

Type error: Argument 1 passed to Illuminate\Routing\Router::sortMiddleware() must be an instance of Illuminate\Support\Collection, instance of Tightenco\Collect\Support\Collection given, called in /Users/marcel/Code/chatwidget/vendor/laravel/framework/src/Illuminate/Routing/Router.php on line 676

Steps To Reproduce:

laravel new testapp
cd testapp
composer require spatie/crawler
open http://testapp.test

See error:

Type error: Argument 1 passed to Illuminate\Routing\Router::sortMiddleware() must be an instance of Illuminate\Support\Collection, instance of Tightenco\Collect\Support\Collection given, called in /Users/marcel/Code/chatwidget/vendor/laravel/framework/src/Illuminate/Routing/Router.php on line 676
@SjorsO
Copy link
Contributor

SjorsO commented Mar 7, 2018

Could someone help me understand why tightenco/collect is used at all in the frameworks composer.json?

@sisve
Copy link
Contributor

sisve commented Mar 7, 2018

We're declaring that we're incompatible with the older (<5.5.33) version of tightenco/collect since that version was using identical namespaces and class names as Laravel provided. It's not being pulled into your project by the root composer.json file, but the spatie/crawler project.

So, the tightenco/collect package declare helpers that are not compatible with Laravel. I believe the current stance is that this is someone else's fault. They shouldn't use our function names. Booo them! Booo! #20370 laravel/ideas#709

On a more serious note, the only way around this is to namespace/prefix all our helpers with something relevant. We cannot control what third party code is declaring, but we can reduce the area of conflict.

Another problem is that we're conditionally defining the helper functions. This means that the code doesn't fail in helpers.php, but instead later when someone discovers that the function they called wasn't the one they intended to call.

@sisve
Copy link
Contributor

sisve commented Mar 7, 2018

The clarify, the problem is not that we're declaring a conflict in our composer.json file, that doesn't pull down the tightenco/collect package. You get it because you're installing spatie/crawler with depends on it.

The problem is that you've got a package that has conflicting helpers. That's not allowed. Boo!

@mattstauffer
Copy link
Member

FWIW, we consider this an issue that collect and the collect team are are taking the responsibility to fix. If it requires a change at the framework level we will attempt to PR it but I believe it won’t.

@ghost
Copy link

ghost commented Mar 14, 2018

I installed a captcha package for Laravel and got the exact same problem.

@repat
Copy link

repat commented Mar 19, 2018

@mattstauffer any updates/ETA on this, we'd really like to use spaties mixed-content-scanner ;-)

@tomcoonen
Copy link

@repat Please see tighten/collect#92 , a fix is released.

@tomcoonen
Copy link

@mpociot this issue can be closed i guess :)

@mpociot
Copy link
Contributor Author

mpociot commented Mar 20, 2018

Yeah - this is now fixed in 5.6.12 of tightenco/collect - https://github.com/tightenco/collect/releases/tag/v5.6.12

@mpociot mpociot closed this as completed Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants