Stricter Typesystem

[Jubeki]

Hey @claudiodekker,

I'd like to help a little bit with the package and thought about how I could contribute the best for now.

So I'd like to propose to use a stricter type system which would mean:

• use declare(strict_types=1) at the top of all files
• using types instead of phpdoc annotations

Why is that change important in my opinion?
A stricter type system would mean catching more unexpected bugs / type conversions. There are a lot of cases were an integer is passed but a string is expected. Because for Authentication Security is an important aspect this should be an explicit instead of an implicit type conversion.

If you want I would work on these things in multiple smaller iterations so that it would be better to review:

1. In multiple iterations I would add declare(strict_types=1) to all files (maybe 10 - 20 files per iteration) and fix the type conversions
2. Once all files have this declare, I would start with replacing phpdoc annotations with php types. (also in multiple iterations)

Note
If I am talking about all files I mean currently only internal files and not the stubs

What is your opinion on this?
(I could also work on this in one large PR, but this would make it quite hard to review)

[claudiodekker]

Hey @Jubeki,

First of all, thanks for your offer to help out. Really appreciated!

While I'm personally not a huge fan of the declare(strict_types=1) syntax, I can see it's benefits for a package like this. In the end, it's indeed more important that it provides a solid foundation rather than look pretty.

With that said, let me just ramble out a few thoughts / considerations that I've had on the past, so that you can see/have an idea of why certain decisions were made and how they influenced the current code. Feel free to disagree / change my mind on these, especially if there's a compelling argument (like the strict types one you mentioned). Anyway, here goes:

First and foremost, it was important to me that the package provided a fully customizable experience. The idea was that by having controllers published into the user's app, and those controllers only exposing intended-to-be-overwritten methods, the user "inherits" full control over the library.

For example, if they decide they want to add magic links or remove the Google Authenticator TOTP method, they can override certain package-internal methods, and change this "core" behaviour. Another example is if the user decides to use a model other than the User model: The goal is that they can just change it, and have the system still work.

Now, this will obviously bring significant risk, as there's a good chance of breakage, and that's where the tests come in: By having the tests written as traits that eventually all end up being included in the AuthenticationTest of the user's codebase, they can easily see what's broken, and if intended, they can either literally override the breaking test, or exclude the by-default included trait that includes the test (or only import "partials").

It's through this exact system, that security updates to the library itself bring "automatic" fixes to the user's authentication system: Since the tests are mostly traits included from the library, and both the "base" controllers in the library AND those tests are updated as part of the same release, their tests will keep passing, unless they've customized something & breakage occurs.

Those are more or less the pillars that I built on when developing this package. With that said, my understanding is that by having strict types in the base controllers/core, or even in some places strict method attributes, it limits what the user can overwrite.

For example, if I'd strictly hint User in the base-class instead of just nothing/mixed, then if the user has a custom User model that they use in their app (e.g. DepartmentUser), the library would prevent them from doing so due to the strict typing, which in turn would mean it wouldn't be a good fit. To me, it's important that this remains possible, and that's why I've so far decided to be fairly relaxed on this and make sure the system behaves properly through tests instead. If there's a way to work around this and have strict types, then I'm all in favor of it, but this is somewhat important to me.

Finally, for some last thoughts, and this is mostly unrelated, but just to explain what my current goals are (in order of planning): While the library is overall in a good shape (especially after this monorepo merge), there's still a few bugs / issues / limits that I'd like to address (in order):

• Recovery: No way to actually reset a password or passkey #3 (planning to pick this up this week)
• Right now, the system's fairly bound to the User model / Authenticatable type. I'd like to relax this, or even consider a custom interface instead (in which case I wouldn't mind stricter types there), as to both reduce the dependency on Laravel's internal authentication logic (it'll still use it, but I want to be able to change the internal integration if necessary), as well as free up the user to use their own models.
• A huge pain point is the tests themselves: While everything is properly covered, it's honestly kind of a shitshow right now, with lots of repetition. If the goal is for the goal to be able to easily override methods, it'd be good to have the tests themselves consists of even more granular "setup" calls through additional/better helper methods, because if the user wants to override just the assertion that handles their custom response, they often have to copy somewhere anywhere between 15-30 lines or so into their app right now, which honestly should be closer to ~5-7. Essentially, the goal is for both the tests to be easy to understand, as well as (similar as with the controllers) to expose as much logic as possible without "placing it" within the user's app: The more the user can rely on internal logic from the package, the more flexibility we keep to change / the less often we have to unnecessarily break their tests.

Once these are done, my plan was to pick up and work on more adapter packages (e.g. the Inertia one), and see what problems we'll run into then. If that works out fairly smoothly, then that's a good sign that the library is in a good shape for 1.0, and if not then we clearly have to do some more tweaking but at least we'll have an idea of what needs tweaking.

Anyway, that's pretty much the picture I have in my head regarding the library, feel free to ask if I missed something or if something's unclear (I am on Telegram as well if that's easier). In either case, I'm totally OK with the types being stricter, as long as we can kind of meet these thoughts/goals. Especially if we can only limit the declare(strict_types=1) to the core library and not to the extended controllers / user's app, as I know of more than a few developers that find that declaration really ugly and annoying.

I also think it makes sense to do a small PoC first with one controller or so, to kind of see/explore how it works in practice, and at that point if we decide it's a good direction I'm completely OK if you go all-out with it. Might just be easier to do smaller PR's anyway at that point, as to not end up with merge conflicts if we're both changing the same files.

Let me know what you think!

[Jubeki]

Thank you for all the plans you shared.

Then I would say we leave the stricter type system open for now. (Maybe use larastan on level 0 for now to already find smaller problems and then later on improve incrementally the type system.)

For now I will take a look at the Custom Interface and the Tests.

[claudiodekker]

Sounds good. Adding Larastan also isn't a bad idea!

[claudiodekker]

Closing this issue, as most/all of the fixable concerns have been addressed as per #35 and #37.