[fix] Updated to include the new Closure (Laravel 9.x support)

[almightynassar]

Similar issue to #31 except instead of Ops\Closure\SerializableClosure it's Laravel\SerializableClosure\SerializableClosure. Most likely because Laravel 9 uses a new closure system.

Probably can get rid of the !== false equality because it makes no sense....

[andreaselia]

Hi there, thanks for the PR. The codebase can be a rather tricky one with the variety of little workarounds to achieve what is achieved with the package. I welcome any PR's to improve the logic too.

Rather than us adding in more exception handles, could you post code to reproduce this error with a fresh Laravel install? That way we can try and narrow it down on whether we need another exclusion here, or if it's a different issue.

[almightynassar]

Just need to use a Laravel API resource and a closure in the route function:

So create a basic resource (App\Http\Resources\UserResource.php):

<?php

namespace App\Http\Resources;

use App\Models\User;
use Illuminate\Http\Resources\Json\JsonResource;

class UserResource extends JsonResource
{
    /**
     * Transform the resource into an array.
     *
     * @param Request $request
     *
     * @return array
     */
    public function toArray($request): array
    {
        return [
            'id' => $this->id,
        ];
    }
}

And then declare the route in your route file:

Route::get('/resource', function () {
    return new \App\Http\Resources\UserResource(Auth::user());
})->name('index');

This closure used in the route used to be handled as an Opis type, and is now the Laravel type of Serializable closure. Probably don't need a resource, to be honest, probably just a function in the route file.

[andreaselia]

@almightynassar what Laravel version are you using? I just tried it on a Laravel 8 and 9 project and was unable to reproduce the issue. Did you try this on a brand new, fresh install of Laravel?

Route::get('/users', function () {
    return new \App\Http\Resources\UserResource(
        \App\Models\User::first()
    );
})->name('users.index');

[almightynassar]

$ php -v
PHP 8.1.6 (cli) (built: May 11 2022 08:56:01) (NTS Visual C++ 2019 x64)
Copyright (c) The PHP Group
Zend Engine v4.1.6, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.6, Copyright (c), by Zend Technologies
    with Xdebug v3.1.6, Copyright (c) 2002-2022, by Derick Rethans
$ php artisan --version
Laravel Framework 9.45.1

Also that's weird, since Laravel forked the original opis/closure package with their own (see the package) and is a required package. So the Opis part of the check would not be valid for Laravel 9.x as that package is no longer used (only kept it for backwards compatibility).

Maybe my example didn't trigger the serializable closure part of the route.... I'll go through my stuff a bit more thoroughly to see.

Just an example of the error message:

$ composer install
Installing dependencies from lock file (including require-dev)
...
$ php artisan optimize

   INFO  Caching the framework bootstrap files.

  config ................................................................................................................................ 109ms DONE
  routes ................................................................................................................................ 330ms DONE
$ $ php artisan export:postman

   ReflectionException 

  Class "O:47:"Laravel\SerializableClosure\SerializableClosure":1:{s:12:"serializable";O:46:"Laravel\SerializableClosure\Serializers\Signed":2:{s:12:"serializable";s:297:"O:46:"Laravel\SerializableClosure\Serializers\Native":5:{s:3:"use";a:0:{}s:8:"function";s:121:"function () {
        return new \App\Http\Resources\User\UserResource(\Illuminate\Support\Facades\Auth::user());
    }";s:5:"scope";N;s:4:"this";N;s:4:"self";s:32:"0000000000000ef20000000000000000";}";s:4:"hash";s:44:"hfNsNYD8/bnkmbQ5epgdQHGJEkDWkZz+fLAL1qOpEYw=";}}" does not exist

[almightynassar]

@andreaselia would you prefer that I change this to Str::contains($action['uses'], 'SerializableClosure')? This will future proof the function in case future Laravel versions again change their Closure implementation (as mentioned above where Laravel 9x forked the original Opis closures in Laravel 8x).

This may be a niche case since most people will use the common Controller method for routing, but the official Laravel docs still mention using Closures in their routes (see here and here).

Just FYI, these route closures will be serialized when the routes are cached. It may still "work" if you do not clear you cache when switching Laravel versions. laravel-api-to-postman should not work in Laravel 9.x when using route closures, since Opis\\Closure\\SerializableClosure was removed as a dependency, and hence will not be properly detected as a Closure.

[almightynassar]

And just to give extra detail using the code inside of Laravel...

Laravel 9.x

In src/Illuminate/Routing/Route.php, Line 19 imports use Laravel\SerializableClosure\SerializableClosure; which is used in Line 1343 of the function prepareForSerialization()

There is no reference to Opis\Closure\SerializableClosure in Laravel 9.x as it has been moved.

Laravel 8.x

Actually reviewing the code, it seems that Laravel\SerializableClosure\SerializableClosure was always in use unless the PHP version was less than 7.4. This is probably in anticipation of the eventual removal of Opis\Closure\SerializableClosure from the codebase.

Line 17 imports Laravel\SerializableClosure\SerializableClosure
Line 19 imports Opis\Closure\SerializableClosure as OpisSerializableClosure
Lines 1282-1284 serializes the route depending on the version of PHP used.

[almightynassar]

When I get more time, I can do some quick testing by updating tests/TestCase.php with a one of the closure routes from Laravel's RoutingRouteTest.php and get the serialized closure stuff triggering

[andreaselia]

@almightynassar thanks for the extra information. So long as a test case is added for this that supports the backwards compatibility of the package, I'm happy to roll with that. I have still been unable to reproduce the issue, so if there's a test case that can do that, then I look forward to seeing that too.

[almightynassar]

@andreaselia any idea on how to trigger caching of the route data in tests? Your current tests will only check Closures and will never get serialised. So this feature was never checked in the first place.

Also, it uses the older version of the orchestra testbench, so it will only pull laravel v8 instead of v9

[almightynassar]

Just renamed this with Laravel 9 in the header so people who are using caching (i.e. you generate bootstrap/cache/routes.php with php artisan route:cache and don't want to clear it just to run this.... btw, to clear the cache then run php artisan route:clear before running this command) and running into this issue can get stuff working.

In composer.json add above the require entry:

"repositories": [
    {
        "type": "vcs",
        "url": "https://github.com/almightynassar/laravel-api-to-postman"
    }
  ],

And then change the entry to:

"andreaselia/laravel-api-to-postman": "dev-laravel-9.x",

And you will be using the Laravel 9.x closure libraries instead of the incompatible older ones. The code is backwards compatible (as it still checks the older Opis library that Laravel 8.x uses).

[almightynassar]

Alternative

@andreaselia If you don't want to support Serialized closures, why not add a route cache clear command at the start of the Export?

So just run a $this->call('route:clear'); at the top of the handle() function?

Only issue is that the cache is still loaded in memory for the export, so need to clear that too.... but running the command a second time works.

I'm probably forgetting something that will clear and reload the routes in memory, but for now my patch fixes the issues when there is a cache file and goes into your Serialized Closure checks.

[andreaselia]

@andreaselia any idea on how to trigger caching of the route data in tests? Your current tests will only check Closures and will never get serialised. So this feature was never checked in the first place.

Also, it uses the older version of the orchestra testbench, so it will only pull laravel v8 instead of v9

I don't have any ideas off the top of my head at the moment, but will try and check this out again in the AM. I'm open to any suggestions regarding the testing setup though, and if anything needs to change to make sure we catch all cases (like updating the testbench) then I'm all for it 👍

[andreaselia]

@almightynassar I think in previous issues where people have mentioned using a closure in their routes, I've simply suggested moving the code to a controller, as I also believe that to be a lot more maintainable than setting a standard of having closures within your routes file.

Edit: I'm heading off for the night now, but will check back here tomorrow with a fresh mind and re-read what you've commented. Thanks again for your work on this, it is very much appreciated.

[almightynassar]

@almightynassar I think in previous issues where people have mentioned using a closure in their routes, I've simply suggested moving the code to a controller, as I also believe that to be a lot more maintainable than setting a standard of having closures within your routes file.

Edit: I'm heading off for the night now, but will check back here tomorrow with a fresh mind and re-read what you've commented. Thanks again for your work on this, it is very much appreciated.

@andreaselia all good, this is a very niche bug that only happens when you use caching and closures. And only because PHP doesn't have a way to serialize closures natively. Most people won't encounter that in development as they will never generate the cache file in the first place.

Another idea would be to catch the ReflectionException in getReflectionMethod() and inform the user that they may have to clear the route cache rather than have the current cryptic error message about Closures.

Testing is a pain since there is practically no documentation about testing with a route cache generated, other than some cryptic reference on line 304 of Concerns/Testing in orchestra/testbench.

[andreaselia]

Apologies for the late reply @almightynassar, I haven't had time or energy today to properly go through this. I'll aim to have gone through this by the end of the week at the latest, and come back with hopefully a better idea of what's going on within this issue (I still do not fully understand it, but I'm pretty tired, so that could be it), and also a direction for the solution.

[tomirons]

It might be worth looking here (https://github.com/orchestral/testbench-core/blob/b1a35c479091ce6e3876971ffa0a4648d96b21f2/src/Concerns/HandlesRoutes.php) to see if you can create cached routes for testing.

[andreaselia]

It might be worth looking here (https://github.com/orchestral/testbench-core/blob/b1a35c479091ce6e3876971ffa0a4648d96b21f2/src/Concerns/HandlesRoutes.php) to see if you can create cached routes for testing.

This looks like a good start so that we can replicate things as part of our testing. What do you think @almightynassar?

[almightynassar]

Been a bit busy (sorry if I sound terse and rushed, had other projects on the line so did my quick fix and moved on 😅)

Definitely interesting, as my biggest roadblock is basically creating the cache file in testing and then running the command (if that cache doesn't exist then you won't get the error)

As to my last point about just catching the exception and letting the user know to clear the cache is what caught my mind... most of the time you want to generate the api from the latest route changes, which may not be reflected in the route cache. So clearing the cache might be desirable default behaviour.... only issue is user scripts that might have cached the routes and cruised on the old checks

[andreaselia]

Thanks for the update @almightynassar. I think this PR can be merged if you're also happy with that? We can always add a test case for this another time.

cc @tomirons

[almightynassar]

Before we merge, just wondering what you think of (can spin this into a different topic):

1. Add a check at the start of the artisan command that detects if the routes are cached; if so, just clear the cache by default (most likely intended behaviour) using \Artisan::call('route:clear');
2. Can add an option that allows the user to manually opt-out of route clearing; If the user wants to use the cached routes anyway, this PR will correctly de-serialise the cached routes

The test itself should be pretty straightforward, just gotta figure out how all the parts fit in the trait file that @tomirons linked... at a glance it does what we need, just gotta get code onto the screen 😅

I'll put any orchestra and dependency update that I get time to do into another PR since that doesn't affect this issue

[andreaselia]

Before we merge, just wondering what you think of (can spin this into a different topic):

1. Add a check at the start of the artisan command that detects if the routes are cached; if so, just clear the cache by default (most likely intended behaviour) using \Artisan::call('route:clear');
2. Can add an option that allows the user to manually opt-out of route clearing; If the user wants to use the cached routes anyway, this PR will correctly de-serialise the cached routes

The test itself should be pretty straightforward, just gotta figure out how all the parts fit in the trait file that @tomirons linked... at a glance it does what we need, just gotta get code onto the screen 😅

I'll put any orchestra and dependency update that I get time to do into another PR since that doesn't affect this issue

Sounds good the me @almightynassar, I don't see any issues with us clearing the route cache.

[almightynassar]

@andreaselia and @tomirons

Created a test, but had to update orchestra to successfully use the HandlesRoutes trait.... using the older version ran into issues with the route:cache command as it tried to ping a database and couldn't a PDO driver.... not sure exactly what is happening there, but updating orchestra seems to fix that.

Once updated the Tests run correctly, then removing the second part of my Serialize check in the Export Command will fail the cached export test as we cannot deserialize correctly (in Laravel 9).

I got to move onto another project right now so I'll look at the style guide issues.... also not 100% sure if the APP_ENV environment variable is needed here so will come back when I get the chance.

Note: Orchestra update means that it will be testing in Laravel 9 and not 8.

[tomirons]

@almightynassar I'm sure there's a way to keep the BC for Laravel 8 as well?

@andreaselia we still want to support Laravel 8, right?

[andreaselia]

Fantastic work @almightynassar! Your time is greatly appreciated on this, and I'm grateful that you're taking the time to contribute to the package.

@tomirons so long as it's not too much of a pain, it would be nice to still support Laravel 8 until at least the official bug fix phase support ends for it in July. I wonder why there were issues with the previous version of the Orchestra package though 🤔

[almightynassar]

@tomirons I'm probably sleep deprived but I'm drawing a blank on BC acronym 😅

@andreaselia For some reason the route:cache command in Laravel 8 would trigger a database call.... checking the compare of Illuminate\Console\Command and Illuminate\Foundation\Console\RouteCacheCommand doesn't reveal anything other than they made commands able to detect other instances of artisan running. I haven't done a deep dive into Orchestra though so maybe the issue is in the differences there.

[tomirons]

@almightynassar BC = backwards compatible.

I haven't thought much on how to resolve this route caching issue, but we could add some version checks within the package and do different logic based on the Laravel version at hand.

[almightynassar]

@tomirons oh lol, of course 😅

So the actual change isn't a breaking change, since it checks both Laravel 8 (Opis closure) and Laravel 9 (in-house Laravel closure). So it'll work for either version of Laravel if the routes are cached.

return is_string($action['uses']) && Str::startsWith($action['uses'], ['C:32:"Opis\\Closure\\SerializableClosure', 'O:47:"Laravel\SerializableClosure\SerializableClosure']);

The route caching stuff only comes into play with testing, and that requires the Orchestra Testbench designed on Laravel 9.

My quick suggestions on the path forward (@andreaselia):

• Create a v2 of this package (using this pull request) that is meant only for Laravel 9, while v1 supports Laravel 8. Will mean that need to pull fix requests to both versions.
• Do something like the tests in laravel/framework and have a matrix that triggers two different jobs.... one for running tests in Laravel 8 and the other 9. These different tests will explicitly install the required orchestra testbench version, and we will update the tests to check for which version of laravel is installed

Just to reiterate, the actual code change (Export command) will work for both Laravel 8 and 9. It's only the tests that are a bit finicky with Laravel versions for some reason

[almightynassar]

@tomirons and @andreaselia

Went ahead and did the second option.... had an inkling that the error was probably because of my local environment set-up

The two test jobs for each laravel version seem to pass; not 100% sure if they are using the corresponding Laravel versions (orchestra v6 => Laravel 8, orchestra v7 => Laravel 9) because the CI command is set to quiet, but at least on my machine composer installs the correct version

[andreaselia]

Sorry for the late reply @almightynassar, and thanks for rolling with the 2nd option as I think it's the one I'd have gone with too. This looks good to me, and seems to be passing, so thanks again for all of your time and effort on this.

@tomirons are you happy with this too?

[tomirons]

@almightynassar thanks for the contribution on this!