search

ksassnowski / venture

Venture allows you to create and manage complex, async workflows in your Laravel apps.
https://laravel-venture.com
MIT License
804 stars 32 forks source link

Replace `opis/closure` with `laravel/serializable-closure` #52

Closed ksassnowski closed 2 years ago

ksassnowski commented 2 years ago

The current stable version of opis/closure is not yet compatible with PHP 8.1 and produces various deprecation warnings. While version 4 aims to fix this, it is still in beta. So in the meantime, we're switching to laravel/serializable-closure.

stevebauman commented 2 years ago

Nice! Will this cause any weird migration issues if devs have then/catch_callback serialized with Opis vs Laravel? I think these closures will contain references to their serializer class like Laravel does? Ex:

O:47:"Laravel\SerializableClosure\SerializableClosure":1:{s:12:"serializable";O:46:"Laravel\SerializableClosure\Serializers\Native":5:{s:3:"use";a:0:{}s:8:"function";s:0:"";s:5:"scope";N;s:4:"this";N;s:4:"self";s:32:"000000001bd94f12000000002cd7084c";}}
ksassnowski commented 2 years ago

I actually added a test to check for backwards compatibility initially but they seemed to already pass without me making any changes.

This is actually the only place were we explicitly check for the specific class (https://github.com/ksassnowski/venture/pull/52/files#diff-7a326dbbe514ecc910647cdcda7a3de91872f75b63651484f3605614e23b5eb3L239). I'm not sure why I even added this check but it's unnecessary as SerializableClosure (in both packages) implements the __invoke method, so we can just invoke it directly.

That being said, I think Laravel 9 removed its dependency on opis/closure completely so maybe I just haven't tested the correct combination of circumstances yet. As much as it annoys me, it might actually be safer to keep opis/closure in our composer.json until the next major version.

ksassnowski commented 2 years ago

I'll just add the two tests back and see what the pipeline says.

stevebauman commented 2 years ago

Ok cool! Yeah I'm not sure how PHP will handle unserializing an object with a reference to a class that no longer exists, since the __invoke() code will no longer be accessible right?

ksassnowski commented 2 years ago

So turns out we have to keep opis/closure as a dependency for now since otherwise unserializing existing job callbacks would fail. Good catch!

stevebauman commented 2 years ago

Dang, oh well -- thanks @ksassnowski! 🙏