DarkGhostHunter / Laraguard

"On-premises 2FA Authentication for all your users out-of-the-box
MIT License
266 stars 24 forks source link

Laravel 9 support #77

Closed it-can closed 2 years ago

it-can commented 2 years ago

Based on #75

I changed the test because jsonSerialize needs to be an array... Hopefully this fix is correct

it-can commented 2 years ago

@DarkGhostHunter do you know why tests are failing? my local machine the tests seems to work

DarkGhostHunter commented 2 years ago

@DarkGhostHunter do you know why tests are failing? my local machine the tests seems to work


PHP Fatal error:  During inheritance of IteratorAggregate: Uncaught ErrorException: Return type of Symfony\Component\HttpFoundation\ParameterBag::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /home/runner/work/Laraguard/Laraguard/vendor/symfony/http-foundation/ParameterBag.php:205
it-can commented 2 years ago

Is this an issue with laravel 8 and php8.1? Seems to be an issue with laravel v8.39, changing it to v8.83.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1833200837


Totals Coverage Status
Change from base Build 1830758048: -0.007%
Covered Lines: 260
Relevant Lines: 265

💛 - Coveralls
it-can commented 2 years ago

@DarkGhostHunter ok that was a little bit of work to get the tests to pass... Could you take a look if this is ok?

DarkGhostHunter commented 2 years ago

Found a fundamental problem with Laravel 9. The jsonSerialize() method must return an array, meaning, the TOTP URI stops being a string and becomes an array.

In other words, if you pass the model in the response (as I do) it will serialize into an array and break the frontend.

Because of this, I cannot approve this PR, hence Laravel 9 compatibility, unless it's a major version.

And alternative would discourage this serialization way, and remove the serialization helpers toJson and jsonSerialize altogether.

it-can commented 2 years ago

Seems this commit changed the return type https://github.com/laravel/framework/pull/40471

it-can commented 2 years ago

@DarkGhostHunter i removed the jsonSerialize method and left the toJson. I can remove this if you want, I also added a note in the upgrade guide.

DarkGhostHunter commented 2 years ago

Seems good 👍 Thanks for your fix.

it-can commented 2 years ago

@DarkGhostHunter Can you release a new version ?