search

Elhebert / laravel-sri

Subresource Integrity hash generator for laravel
MIT License
40 stars 16 forks source link

(Update) PHP8 Support #68

Closed HDVinnie closed 3 years ago

HDVinnie commented 3 years ago

@owenvoke @Elhebert anyway this can be merged?

owenvoke commented 3 years ago

Nice, I don't actually have any say in this package (I'm just a past contributor). But probably seems fine. Might be worth updating the CI workflows as well to run against PHP 8. 👍🏻 If you need any help with that, let me know.

HDVinnie commented 3 years ago

@owenvoke thanks. Updated the PHPUnit configs and the composer.json with your suggestion.

Elhebert commented 3 years ago

Thanks for the PR, the code looks good. However some tests are failing due to some deprecation. Can you have a look?

owenvoke commented 3 years ago

Looks like the depreciations are due to versions of Mockery lower than 1.4.1. 🤔

When running on the --prefer-lowest on PHP 8, it seems to select the exact version that is specified by orchestra/testbench-core, which for Testbench 4.5 is Mockery 1.2.3, not sure how to force that to be the latest version without adding Mockery ^1.4.2 to the Composer JSON.

HDVinnie commented 3 years ago

Looks like the depreciations are due to versions of Mockery lower than 1.4.1. 🤔

When running on the --prefer-lowest on PHP 8, it seems to select the exact version that is specified by orchestra/testbench-core, which for Testbench 4.5 is Mockery 1.2.3, not sure how to force that to be the latest version without adding Mockery ^1.4.2 to the Composer JSON.

I have update tests and composer.json but IMO for them to all pass will need to drop PHP 7.2 support and only allow 7.3, 7.4 and 8.0.

HDVinnie commented 3 years ago

@Elhebert all tests are now passing. Would mean the new release oof this package would require PHP 7.3+

owenvoke commented 3 years ago

I feel like that's probably fine. PHP 7.2 is EOL, and people can still use the existing version perfectly fine if they are on 7.2. 🤔

Elhebert commented 3 years ago

Do we still need to mockery as a dev dependency if we drop 7.2?

HDVinnie commented 3 years ago

Do we still need to mockery as a dev dependency if we drop 7.2?

Yes for PHP8. New release of this package should be v3.0.0 and requires PHP 7.3+

Elhebert commented 3 years ago

I already started work on a v3 (#57) which drop the support of Laravel 7 (and PHP 7.2.5).

I'm going to merge (soon) #62 and #65 into master. Once this is done, you can rebase and see if we still need to have mockery as a dev dependency or not.

I'll try to do it before the end of the day (in the next 6-8 hours).

That way we can release v3 with PHP8 support.

owenvoke commented 3 years ago

If it's Laravel 8+, that should be fine. 👍🏻

Elhebert commented 3 years ago

@HDVinnie just merged both branches. You can now rebase on master ;)

HDVinnie commented 3 years ago

@Elhebert PR is good to go.

owenvoke commented 3 years ago

Should be able to just do ^7.3 || ^8.0 for the PHP version. 👍🏻 Otherwise this looks great.

Elhebert commented 3 years ago

v3 has been tagged and released 🎉