Skip to content

Fix Tests #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Conversation

erikn69
Copy link

@erikn69 erikn69 commented Jul 21, 2023

Try this
Passport 11 don't support laravel 8

PHPUnit 9.6.10 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.2
Configuration: /laravel-permission/phpunit.xml.dist

...............................................................  63 / 525 ( 12%)
............................................................... 126 / 525 ( 24%)
............................................................... 189 / 525 ( 36%)
............................................................... 252 / 525 ( 48%)
............................................................... 315 / 525 ( 60%)
............................................................... 378 / 525 ( 72%)
............................................................... 441 / 525 ( 84%)
............................................................... 504 / 525 ( 96%)
.....................                                           525 / 525 (100%)

Time: 00:13.213, Memory: 62.00 MB

OK ( 525 tests, 1240 assertions)

I think that with the tests fixed you will be able to continue your work

@erikn69
Copy link
Author

erikn69 commented Jul 21, 2023

Hi @drbyte, Is this approach too much intricate?
Would it be convenient to leave Laravel 8 behind?

@drbyte
Copy link

drbyte commented Jul 23, 2023

@erikn69

Is this approach too much intricate?

This is probably the best approach for now.

I'd prefer to move the changes to missingTraitAuthorizable into a separate PR, because they're not entirely specific to this Passport PR.
I'll also push the fix to Show.php directly now.

Would it be convenient to leave Laravel 8 behind?

I'm not ready to do that in 6.0. Willing to do it in 7.0.

@erikn69 erikn69 force-pushed the patch-33 branch 3 times, most recently from d62bb72 to 1aa3b21 Compare July 24, 2023 15:42
@erikn69
Copy link
Author

erikn69 commented Jul 24, 2023

Hi @SuperDJ, try this version

@SuperDJ
Copy link
Owner

SuperDJ commented Jul 25, 2023

@erikn69 I think this will meet most of the scenarios. The only one I'm not sure of is the following. Passport can login as different "types":

Each of these types should fallback to something. For example the Password Grant should just use the User model. The Client Credentials Grant, the one we are interested in, should use the Client model. And currently I'm not sure if we don't interfere with the others. I mean the change we are making should only apply for the Client Credentials Grant. But found this method that could help. I don't know if it would be enough to check on those two values.

@SuperDJ
Copy link
Owner

SuperDJ commented Jul 25, 2023

@erikn69 tested the changes locally and they work as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants