xdavidwu / laravel-oidc-auth

OpenID Connect authentication for Laravel
MIT License
8 stars 6 forks source link

Add features and organize code to be more customizable #1

Closed zhshize closed 1 year ago

zhshize commented 2 years ago

I think that Customizable is the most important property when I am applying this library to many old projects. So I do many modifications, it will be fantastic!

I merged new updates during development, don't worry about the inconsistency between two repos.

The doc is added in README.md, it is helpful.

I think that the API changes are so significant that it could be a major version release.

What's inside

Fixes:

Enhancements:

Features:

Modifications:

Significant Updates

xdavidwu commented 2 years ago

I think that the API changes are so significant that it could be a major version release.

Per SemVer, we won't bump major unless we feel easy to gurantee API stability.

config/oidc-auth.php renamed to config/oidc_auth.php Because of Laravel naming convention

Is that a thing in config/ naming? A quick find in my work environment over vendor/ of a few laravel projects shows a case with - (ide-helper.php in barryvdh/laravel-ide-helper, I believe this is popular), and nothing with a _.

This is a huge patchset and require time to review. You should probably split the changes into individual PRs next time.

zhshize commented 2 years ago

To Do:

zhshize commented 2 years ago

Breaking changes

Renamed & moved files

xdavidwu commented 2 years ago

about addition of composer.lock: I think we don't actually need to use lock file in library development. It will not be respected when used as a library anyway. I am against tracking a lock file as it may hinder potential chances on catching problems on development.

xdavidwu commented 2 years ago

config/oidc-auth.php renamed to config/oidc_auth.php

I'm still against this until this convention is documented in official documents, or there is widespread adoption.

LaravelOIDCAuth\OIDCAuthenticatableFactory

afaict it's LaravelOIDCAuth\Contracts\OIDCAuthenticatableFactory

zhshize commented 2 years ago

config/oidc-auth.php renamed to config/oidc_auth.php

I'm still against this until this convention is documented in official documents, or there is widespread adoption.

There is an example: https://github.com/laravel/cashier-mollie/tree/develop/config and a non-official guide: https://github.com/alexeymezenin/laravel-best-practices#follow-laravel-naming-conventions

I like config('oidc_auth.auto_refresh') better than config('oidc-auth.auto_refresh').

LaravelOIDCAuth\OIDCAuthenticatableFactory

afaict it's LaravelOIDCAuth\Contracts\OIDCAuthenticatableFactory

LaravelOIDCAuth\OIDCAuthenticatableFactory -> LaravelOIDCAuth\Contracts\OIDCAuthenticatableFactory updated.

zhshize commented 2 years ago

about addition of composer.lock: I think we don't actually need to use lock file in library development. It will not be respected when used as a library anyway. I am against tracking a lock file as it may hinder potential chances on catching problems on development.

I'll remove that commit.

xdavidwu commented 2 years ago

There is an example: https://github.com/laravel/cashier-mollie/tree/develop/config

While that is not a famous package, it seems like that is the only example with multiple words under laravel namespace (although it is moving out). If we are going for underscores, we should also rename the route definition file.

xdavidwu commented 2 years ago

Mark resolved threads as resolved (thread status, not by commenting) and I will do another review round.

xdavidwu commented 2 years ago

this needs a rebase

xdavidwu commented 1 year ago

Closing due to inactivity and lack of consensus.

To whom it may concern, discuss with maintainers before implementing such a big patchset.