Laravel collect() returns the wrong class when using tightenco/collect

[tomcoonen]

It seems to be related to the loading sequence I guess, when installing the dev dependencies the issue is gone. So in dev I have no issues, but on non-dev environments the issue is 100% persistent.

How can we prevent the error?

PHP Version: 7.1.12
Laravel: 5.6.8
tightenco/collect: 5.6.7

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 xxx/vendor/laravel/framework/src/Illuminate/Routing/Router.php on line 676

[tomcoonen]

Confirmed the issue with the loading sequence (vendor/composer/autoload_static.php):

        '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',

[mpociot]

I'm also experiencing this.
Using this package in combination with Laravel 5.6.8 does not work for me anymore because it gets loaded before the Laravel helpers get loaded.

[mpociot]

I opened an issue on the Laravel repository too: laravel/framework#23420

[mpociot]

Wouldn't it be sufficient to just check if the Illuminate Collection class exists in the helpers.php file?

So basically just wrap everything inside:

if (! class_exists(Illuminate\Support\Collection::class)) {
    if (! function_exists('collect')) {
    }
    // ...
}

We're already aliasing the tighten classes in case the illuminate classes do not exist - so if we do the same thing for the helper methods, we're good.

At least that's easier than namespacing / prefixing all existing helper functions - which would break a lot :)

[mattstauffer]

@mpociot Great idea! @damiani and I will be in the same physical location for the next few days so we're going to try to map out a solution that fixes this and the other problems we've run into, and we'll try to have one single fix that hopefully addresses them all.

Thanks!

[antonioribeiro]

According to this issue in Composer: composer/composer#956 and this PR which fixed it: https://github.com/composer/composer/pull/1051/files#diff-33037407f7f293f37df2258567b56598R238

the order (for vendor autoloaded files) here should respect the order found when resolving the dependencies,

I agree this is a big issue, but I could not reproduce the bug, because, unfortunately (or luckily) it looks like the order on autoload_static is somehow random, or depends on something else, because, on every single test I did, both Collect's autoloaded files (helpers and alias) are the last:

But this should be an issue only for those requiring Collect before the laravel/framework requirement.

[mpociot]

@antonioribeiro I'm able to reproduce it with this:

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

[tomcoonen]

Since we have no control on the loading sequence of the files, this package should be Laravel aware by like @mpociot ‘s proposal e.g.

[mpociot]

Hey @mattstauffer ,
Any update on this? I can submit a PR with my proposed changes, if you want.

[mattstauffer]

@mpociot Thanks for asking! We still haven't found a good solution, to be honest.

Wouldn't your proposed changes suffer from the same problem: That if they run before Laravel is loaded, it wont' detect that the class exists?

I think any such conditional would have to be inside the definition, changing which class it returns, rather than outside of it. Thoughts?

[mpociot]

Wouldn't your proposed changes suffer from the same problem: That if they run before Laravel is loaded, it wont' detect that the class exists?

@mattstauffer No that is not a problem.

The helpers.php gets loaded from the composer autoloader and at this time, it already knows about the available classes and can check if the Illuminate\Support\Collection class exists.

You can manually test this by doing:

Reproduce bug

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

Open the autoload_static.php file and you'll see that the /tightenco/collect/src/Collect/Support/helpers.php gets loaded before the /laravel/framework/src/Illuminate/Foundation/helpers.php.

Apply fix

Edit the /tightenco/collect/src/Collect/Support/helpers.php and add my proposed changes.

Reload page - the error is gone 💥

Edit:
And of course, if using this package without Laravel loaded at all, it'll still work as expected, as the if statement returns false.

[mattstauffer]

@mpociot Ahhh... we're having two issues here.

One has to do with the load order (which you are NOT seeing), and another has to do with the collect helper.

Your proposed solution fixes the second one:

In a laravel project which includes collect or includes a package which includes collect
If Collect's helpers are autoloaded before Laravel
then the collect function is defined by Collect's helpers
and therefore returns a Collect Collection instead of a Tighten collection.

So go ahead with it.

Sadly it doesn't fix the issue here (right?)

In a Laravel project which includes Collect or includes a package which includes Collect
If Collect's aliases are autoloaded before Laravel
Then the aliasing doesn't see the Illuminate classes and, improperly, aliases them to Collect's classes.

... regardless, yes, please PR it! And remember to PR it to the stub as well, not just the helper in src. Thanks!

[mpociot]

@mattstauffer To be honest, I’m having a hard time wrapping my head around the load order issue, as I do not really see the difference between these two issues.

Isn’t the original issue posted by @tomcoonen only related to the helpers being defined by collect instead of Laravel? That’s the exact same issue that you can reproduce with my example.

Let me know if I get something wrong 😅

Regarding the PR: yup, I’ll prepare it.

[rodrigobendia]

What about removing the type hint from the Illuminate\Routing\Router sortMiddleware function?

Changing it to array is also a possibility, since the SortedMiddleware does not require the second argument to be a Collection instance.

I'm not able to test right now, but I think it would solve the problem.

[sisve]

@rodrigobendia That would just move the problem to the next method that type-hints Laravel's Collection class.

[antonioribeiro]

And remember to PR it to the stub as well, not just the helper in src.

You just have to change the stub and run the script upgrade.sh, which will use the stub to create the actual helper file and will also run all tests for you.

[antonioribeiro]

We have some issues here:

The autoloading order of vendors files

This is the original subject of this issue and this matters because Tighenco\Collect\Support\helpers.php may load before Laravel's helpers and the collect(), called on Laravel's internals, will instantiate Collect objects instead of Collection objects. @mpociot's proposal fixes this by completely disabling Collect's helpers when Laravel is available.

My opinion is that this will work and should be enough to fix it all.

The autoloading order of files of a single vendor

Here's the related part of the composer.json file

"files": [
    "src/Support/helpers.php",
    "src/Support/alias.php"
]

This is a problem for Collect because the alias.php file mimics the presence of Illuminate\Support\Collection, and if Composer autoloads it before helpers.php then

if (! class_exists(Illuminate\Support\Collection::class)) 

May return true, and the collect() helper (or all helpers if we chose to) will not be created even if Laravel is not present.

This should not happen, exactly the way loading vendors files required after laravel/framework should not load before it. But it's happening on @mpociot and @tomcoonen's end, but not on my end. This is something discussed on the Composer project and should not behave the way it is. So, should we rely on one order but not on the other?

My opinion is that this should not be a problem, because in @tomcoonen's comment, even though vendor files are loading in the wrong order, helpers.php are being loaded before alias.php, as it is supposed to, but, again, as it is behaving erratically in some environments, this may also fail with someone else.

Upgrade.sh must change

If we do not change it, the fix proposed by @mpociot, to be made only on stubs/src/Collect/Support/helpers.php will include a reference to Illuminate\Support\Collection which will, when creating src/Collect/Support/helpers.php, be renamed to Tightenco\Collect\Support\Collection and will not work. That's what the script does: it downloads the last version of Laravel Collection and renames Laravel's namespace to Tightenco's.

[mattstauffer]

I was under the impression that it was the entire loading that was out of order. If it's just that the autoloading of the files that's out of order, but not the PSR-4 classes, then yah, I think that would solve quite a few of the issues we're having.

Is that definitively true? The only issue here is that our files are loading out of order, but by the time our files have loaded, the classes have all been autoloaded??

[antonioribeiro]

The last thing the autoloader does, after loading PSR4, PSR0, ClassMap is to load "files", so classes should have been all loaded at this point:

public static function getLoader()
{
    if (null !== self::$loader) {
        return self::$loader;
    }

    spl_autoload_register(array('ComposerAutoloaderInit5d76e8a5c9ad4b47bad53bd090742538', 'loadClassLoader'), true, true);
    self::$loader = $loader = new \Composer\Autoload\ClassLoader();
    spl_autoload_unregister(array('ComposerAutoloaderInit5d76e8a5c9ad4b47bad53bd090742538', 'loadClassLoader'));

    $useStaticLoader = PHP_VERSION_ID >= 50600 && !defined('HHVM_VERSION') && (!function_exists('zend_loader_file_encoded') || !zend_loader_file_encoded());
    if ($useStaticLoader) {
        var_dump('static'); die;
        require_once __DIR__ . '/autoload_static.php';

        call_user_func(\Composer\Autoload\ComposerStaticInit5d76e8a5c9ad4b47bad53bd090742538::getInitializer($loader));
    } else {
        $map = require __DIR__ . '/autoload_namespaces.php';
        foreach ($map as $namespace => $path) {
            $loader->set($namespace, $path);
        }

        $map = require __DIR__ . '/autoload_psr4.php';
        foreach ($map as $namespace => $path) {
            $loader->setPsr4($namespace, $path);
        }

        $classMap = require __DIR__ . '/autoload_classmap.php';
        if ($classMap) {
            $loader->addClassMap($classMap);
        }
    }

    $loader->register(true);

    // ----------------------------------------------------------------
    // ----------------------------------------------------------------
    // ----------------------------------------------------------------
    // THEN LOADS FILES 
    // ----------------------------------------------------------------
    // ----------------------------------------------------------------
    // ----------------------------------------------------------------

    if ($useStaticLoader) {
        $includeFiles = Composer\Autoload\ComposerStaticInit5d76e8a5c9ad4b47bad53bd090742538::$files;
    } else {
        $includeFiles = require __DIR__ . '/autoload_files.php';
    }
    foreach ($includeFiles as $fileIdentifier => $file) {
        composerRequire5d76e8a5c9ad4b47bad53bd090742538($fileIdentifier, $file);
    }

    return $loader;
}

[mattstauffer]

Geez. Then yes. This Pr of marcel’s Work with antonio’s Suggested alterations of the upgrade script sound like exactly what we need. Thanks!

[tomcoonen]

The only issue I'm having is the loading order of the files, since its sort of random (depends on the packages you require).
A composer update now sometimes breaks a project when this package is loaded before Laravel's.

I didn't test the solution from @mpociot yet, but looking at it I think it solves the problem.

[kre8or69]

I was having this problem too, changing the file order worked. But after I changed them back (and it broke again) and applied @mpociot fix and its all good now! 👍

[mattstauffer]

@tomcoonen @kre8or69 and anyone else who had this issue: I've merged a fix to master but not tagged yet. Could you try pulling this in dev-master (if that's possible and easy) to see if it fixes it for you? If that's too hard, no worries.

[tomcoonen]

@mattstauffer I can't require the package on a current project since dependency restrictions won't allow me.
On a fresh project I experience the same as @mpociot , it fixes the helpers.

[kre8or69]

Yeah i'm the same sorry!

[mattstauffer]

OK, I've tagged it with 5.6.12 and I think we're good. Here's hoping!