Thoughts about type hinting and supporting old php versions

[NicolasCARPi]

Hello,

First, thank you for providing this library, I use it in my project: eLabFTW: the open source lab notebook ❤️

I see that the minimum version for php is 5.6. Does it really makes sense to support a version that has been out of the game for 2 years now?

See my arguments:

By requiring a modern php version you push admins to update their stack and increase global internet security ®, but more importantly you can move forward with your codebase and use modern features (that are not even new anymore) and make your life easier as a developer.

With that you open the door to better static analysis, and (this is why I'm here) users of the library can also type hint properly their code. My codebase is fully typed, except for getQRCodeImage ;)

Also, if users are still using php 5.6, it's quite unlikely that they regularly update their dependencies, so the argument of "but there are still people using 5.6 out there!" is moot IMHO. In the era of containers, one has no valid excuse for keeping around old php versions (except time and money of course, but that's another debate).

Please kindly indicate what are your reasons for supporting old php versions and if you'd consider dropping old versions in order to allow yourself and contributors to improve gradually the codebase with modern php language constructs, operators and features (and incidently, allow me to full type hint my code ;) ).

I'm willing to contribute to this work with PRs.

Best,
~Nico

PS: this blog post is an interesting read

[RobThree]

Lots of people are still stuck on pre 7.x versions because some organisations just don't move that fast.

By requiring a modern php version you push admins to update their stack

But why would we push them if there's no need to? I'm sure they're feeling the push already; companies like Google etc. already keep pushing ahead where some organisations struggle to keep up with all the latest and greatest.

But more importantly you can move forward with your codebase and use modern features

If this were a huge library or framework then I'd agree. However, let's not kid ourselves, the core of this package is 256 lines (including lots of comment lines). There's not THAT much "modern features" to be used. Yes, typehinting would improve code quality, as would some other newer features. But I don't think it would add that much benefits so I'd rather support some people stuck in some older versions,. That doesn't mean I don't think using type hinting wouldn't improve the code or that I'm against it doing it eventually but, for now, I don't think the benefits outweigh the drawbacks of dropping support for a lot of organisations.

Also, if users are still using php 5.6, it's quite unlikely that they regularly update their dependencies

It's not always because they're lazy. Sometimes they have other dependencies, hosting companies that don't update, invested huge amounts of money in software that's still in the progress of being updated to a newer version but not done yet, whatever. I don't think TwoFactorAuth is the library / package to force such an "agressive" update; there's enough packages that do already. Nothing is preventing you from using this package in 7.X (and soon 8.x).

one has no valid excuse for keeping around old php versions (except time and money of course, but that's another debate)

Well, you're saying it yourself: time and money can be / is an issue for some organisations. Why would "we" push them just so we can indulge in our Shiny-new-things syndrome?

Long story short: Yes, I see, know and understand that there will be benefits. And, yes, we will/should move to better code eventually. But I also don't want this library to suffer from "shiny new things" syndrome. A bit of a more moderate update policy goes a long way IMHO. The benefits don't outweigh the "cost" (time, money, resources, whatever) some organisations/users will have to make if we try to force them. Supporting 5.6 and upwards isn't THAT much of a bother, IMHO.

But I'm always open for (further) discussion.

[NicolasCARPi]

But why would we push them if there's no need to?

I think the push comes from the fact that old versions don't receive security patches, leaving systems vulnerables. Allowing the install on older versions is enabling "them". Look at wordpress, it used to support antique php versions for ages, leaving a lot of websites open to attacks (most of them quite easy to leverage).

Why would "we" push them just so we can indulge in our Shiny-new-things syndrome?

To decrease the burden of having to consider several versions to support, but maybe it's not a good argument. I don't know if you are getting paid to work on this or if it's more like a side project not generating money. You owe them nothing. Your time is precious, and having to fix bugs because something is broken in an unsupported php version is not your job (unless you're paid for it). Now as you said, this lib is not that big, and if everything is working fine, there is no need to push the minimum php version.

I think you made your point clearly and I understand your point of view and accept this (temporary) decision. If I really want it I could always fork it and add type hinting myself ;) (for the moment @phpstan-ignore-next-line will suffice)

Thanks for the discussion and keep up the good work! :)

[RobThree]

I don't know if you are getting paid to work on this

I don't 😩

or if it's more like a side project

It is 😄

not generating money

It isn't 😜

Again: I do agree we need to, at some point, I just don't think now is the time. Are we enabling then? Maybe. I also think we may be helping people out securing their applications with 2FA who otherwise wouldn't (or would have to jump more hoops). The goal, in the end, is to provide easy 2FA for as many projects as possible as simple as possible.

If it turns out I'm a minority in this, then, by all means, we should move ahead and drop 5.x support so anyone with an opinion on this is VERY welcome to discuss! That's why I'll reopen this issue (for a while at least).

[MasterOdin]

Lots of people are still stuck on pre 7.x versions because some organisations just don't move that fast.

PHP 7.0 was released 5 years and PHP 7+ been the standard install for apt install php on Ubuntu since 16.04. The number of composer install in the November 2020 using PHP 5.6 was 2.71% of all composer install (https://blog.packagist.com/php-versions-stats-2020-2-edition/). It really isn't used that heavily anymore to really justify continued support if it makes development of the package easier / safer with types (especially combined with declare(strict_types=1);). Could always combine this in the next major release with some of the other possible breaking changes you suggested in #39 (comment).

[NicolasCARPi]

I don't

Time to setup GitHub Sponsors ;). I'll be your first sponsor!

we need to, at some point, I just don't think now is the time

While writing this comment, @MasterOdin posted one, which shows that now is as good a time as any :)

And as @MasterOdin said, it's also a good opportunity to introduce breaking changes.

[RobThree]

It really isn't used that heavily anymore

Well, this source begs to differ; I wouldn't call ~39% "not that heavily used anymore" - but I'm not sure how reliable it is (though it is referred to by php.net). I can imagine people being stuck on older versions not using packagist either.

Could always combine this in the next major release

If (or rather: when) we're dropping 5.x it will only be done on a major release version number. As discussed somewhere in this PR, I'd like to combine it then with moving the providers out to separate packages.

So, let's say we'll start coordinating and planning for a V2.0 then?

• Drop 5.x support
• Use shiny "new" stuff like type hinting.
• Move providers to separate ('out of band') packages

Anything else we'd like to put on a roadmap for a V2.0? Maybe I'll create a V2.0 project and define some tasks? @willpower232 What is your opinion on this issue and 'plan'?

Time to setup GitHub Sponsors

Looking into that now 😜

[willpower232]

Honestly, my feelings are a bit mixed. I understand that newer PHP is faster, better, and more secure, but at the same time, as someone who has only in the past few weeks been able to move things off PHP 7.1 and is still struggling with it, pressure from literally everything to upgrade is easily overridden by "this works now so why change".

As mentioned above, the core of this package is very small compared to other libraries and frameworks so the additional pressure would be arguably minimal. I'm not even sure there are significant optimisations available to this code which would add a worthwhile performance improvement in newer PHP versions.

That said, with the approval of my test changes, the next thing I'd like to look at is code formatting, type hinting, and doc blocks. I believe we can apply some type hinting and continue to support PHP 5.6. The only thing we can't do currently is return type declarations that were introduced in PHP 7.

This may be a problem for those that have extended this package directly to make their own providers but them adding the type hints shouldn't be a massive task.

So all in all, the code happens to work in at least php 5.6 and later, if not earlier as well. Changing that to 7.whatever doesn't change the "happy accident" that the code will fully work in older versions, just that the architect has deemed it not worthy. At least until return type declarations are introduced.

I guess one question is whether you would want to release one final 1.x version which was "more robust" but not actually functionally different to its predecessors or 2.x.

It would be good to know whether you wanted to remove the recently added but not released bacon and endroid providers, asking those that implemented them to create their own repo and packagist package etc or whether they would be the last new providers and then wholly supported in here going forwards.

Hope this was useful and not too rambly :-P

[MasterOdin]

I believe we can apply some type hinting and continue to support PHP 5.6. The only thing we can't do currently is return type declarations that were introduced in PHP 7.

The codebase already has the extent of type hinting you can do with PHP 5.6, namely classes, interfaces, and arrays. You need to move to PHP 7.0 to get scalar type hint support. Best you could do would be to expand the docblocks.

[willpower232]

Ah yes, I had misread some notes I'd made, my bad

[MasterOdin]

Looking at #95, I wonder if there's still any appetite to moving to a newer lower PHP version requirement? Reading the motivation and closing comment, I'd add that on moving to PHP 7+, you'd be able to add a string type hint and then if a user passed in an int, it'd do this implicit conversion, and if you wanted to forbid passing anything but a string, could use declare(strict_types=1); to enforce that. However, either of those requires moving to at least PHP 7.0 as minimal version and breaking PHP 5.6 support.

For reference, PHP 5.6 was end of life almost 4 years ago at this point.

[RobThree]

For reference, PHP 5.6 was end of life almost 4 years ago at this point.

We've had that discussion before. I agree people should move on, but, coming from a 5.6 codebase myself pretty recently, I know the reality of it is that a LOT of projects are still 5.x, EoL or not. And, quite honestly, I don't think requiring 7.x (or even 8.X) adds that much value over dropping support for 5.x.

Don't get me wrong, we will drop 5.x support eventually, but not right now if it's up to me. I am all for being on the latest and greatest, I really am, but if you are stuck on a huge codebase that takes many manhours to port and you're on your own the last thing you want is everything pushing you to the newest version for no good reason. Not saying there's no good reason to drop 5.x support, I just don't think it's good enough .

you'd be able to add a string type hint and then if a user passed in an int, it'd do this implicit conversion

It would convert 7292 to "7292" but it wouldn't 'magically' add two zeroes at the beginning of the string (e.g. "007292"). As I understood #95 user already had an int and passed that into verifyCode.

Also, the code has been type-hinted in the docblocks (or whatever it's called). Many (agreed, not all) IDE's will point out incorrect types because of those type-hints too.

Though this may not be the answer you were hoping to hear, and YES, eventually in the not too distant future I would like to move to greener fields, but I think, for now, we have a quite nice compromise and happy middleground solution.

[MasterOdin]

you'd be able to add a string type hint and then if a user passed in an int, it'd do this implicit conversion

It would convert 7292 to "7292" but it wouldn't 'magically' add two zeroes at the beginning of the string (e.g. "007292").

Yeah, I'm not saying I agree the implicit conversion is right here just that it would accomplish what #95 would change. I agree that it would probably be much better to enable the strict_types=1 as well so that the function just out right errors when given a non-string value, which would help prevent that class of usage errors.

[RobThree]

Yes, but strict_types=1 is a PHP7 feature, isn't it?

[MasterOdin]

Yes, sorry, I meant to include that using that would require PHP 7.0 (along with the scalar string type hint), but it got deleted in one of my rewrites of the comment.

[willpower232]

I, for one, look forward to adding PHP 8.2 to the automated tests in a few weeks 😅

[willpower232]

For future reference, there are apparently a significant chunk of people using PHP 5 still https://w3techs.com/technologies/details/pl-php

[MasterOdin]

The internet certainly has a very long tail of sites, and there's the caveat of not knowing how much of the 22.8% of sites using PHP 5 are using it because they've long since been abandoned, but haven't yet been taken down, vs that they're still actively updated, but sticking to PHP 5 for whatever reason.

Another data point is https://packagist.org/packages/robthree/twofactorauth/php-stats which shows that per week since basically 01/01/2022, only ~1% of people installing this package via composer are using PHP 5. Coupling with https://packagist.org/packages/robthree/twofactorauth/stats, then that means of the ~27905 installs per week, you're getting ~279 that are on PHP 5.

[RobThree]

@MasterOdin While that is true, in my experience the people using (voluntarily or being stuck on) PHP 5 are not often using composer. I don't have any hard numbers and I think we can all agree that there will be plenty of sites with all kinds of marketshare numbers for different PHP versions and I think we can all agree that trying to figure out which is correct will be an eternal quest.

I think the main point here is: I don't think the PHP7+ features add THAT much value to this library. There's absolutely value in type hinting (being a mainly C# dev, I know to appreciate that 😉 ) but the cost of keeping PHP 5 supported isn't very high (currently).

As I've said before, I think another route we can take is just create a "7+" branch (maybe even 8+?) and keep this one around as "legacy" and we'll have the best of two worlds. Amirite? 😅

[MasterOdin]

As I've said before, I think another route we can take is just create a "7+" branch (maybe even 8+?) and keep this one around as "legacy" and we'll have the best of two worlds. Amirite? 😅

Sure, that's a common strategy. Have your master branch point at say v2 that's 7+ (or as you say, 8+) and then can back port any changes to v1 as makes sense. From my experience, it's much easier to back port from a version that has type hinting to one that doesn't than forward porting from a version without type hinting to one that does.

[mar7t1n]

Isn't that what the RELEASES option is for. You simply state v2.x is for PHP7+ only. Anyone who would like to continue to use 1.x will find it still works on PHP5. You can lock composer to no update further, or of course if you're not updating from PHP5 you're unlikely to be updating anything.

[NicolasCARPi]

Cannot agree more with what @mar7t1n said. We should not care about who is using what. Release a new tagged release v2 for 8+ (PHP7 is dead already) and that's it. Those on PHP5 won't update anyway, so why should we care. And as @MasterOdin said, it's like 1% of users of this present lib.

Leaving here again this blog post that is in my first message: https://kinsta.com/blog/php-versions/

I would just be happy if IQRCodeProvider interface has type hints, because it is the only place in my code where I cannot type arguments because of this (and has been for quite some time now!).

Cheers!
~Nico

[RobThree]

And as @MasterOdin said, it's like 1% of users of this present lib.

According to 1 statistic - and of which the validity is dubious in the very least.

Leaving here again this blog post that is in my first message: https://kinsta.com/blog/php-versions/

The real world is, unfortunately, not a perfect world. You SHOULD use the latest (stable) PHP release and stay on it. Reality, though, is that many, many developers are stuck on older (and even EOL) versions.

As I've said; I would like a 'fresher' version, a 2.0, too. Totally agree. But I still don't see THAT much added value. Having said that, there's nothing stopping us from creating a V2 branch and start work on going PHP 8. I welcome a PR with the proposed changes; if not then this will have to wait _at least) till the second half of december where I have some spare time to work on this.

Edit: Whoopsie, sorry @NicolasCARPi, apparently I editted your comment instead of replying...

[NicolasCARPi]

I welcome a PR with the proposed changes

I just started working on it!

[NicolasCARPi]

@RobThree Is it okay if I target 8.1 so I can use enums? If we're gonna update this lib, let's do it properly ;)

[RobThree]

@RobThree Is it okay if I target 8.1 so I can use enums? If we're gonna update this lib, let's do it properly ;)

Go nuts! 😉

[NicolasCARPi]

Only 1 hour in and already: 15 files changed, 101 insertions(+), 376 deletions(-) 🚀 Library will be even easier to maintain ;)

I removed MCrypt stuff because it doesn't exist anymore since 7.2!

[RobThree]

Only 1 hour in and already: 15 files changed, 101 insertions(+), 376 deletions(-) 🚀 Library will be even easier to maintain ;)

I removed MCrypt stuff because it doesn't exist anymore since 7.2!

Noice. Try not to break the API though (or if you have to, with care and consideration).

[NicolasCARPi]

At the moment, the only breaking change is with Algorithm enum. Instead of providing a string, user of the lib should provide an instance of \RobThree\TwoFactorAuth\Algorithm, such as Algorithm::Sha256.

We're going for a major version change, so breaking API is okay I think (that's the whole point of semver), as long as this breaking change is properly documented in the changelog. And any good IDE will mention this issue right away, and it's an easy fix.

Currently: 46 files changed, 393 insertions(+), 788 deletions(-)

Found a few bugs here and there, too ;)

You can see the changes here: https://github.com/RobThree/TwoFactorAuth/compare/master...NicolasCARPi:TwoFactorAuth:php8?expand=1

[RobThree]

We're going for a major version change, so breaking API is okay

Yes, but breaking as little as possible will provide the easiest path to upgrade to the latest version. I agree with the Algorithm enum though.

Found a few bugs here and there, too ;)

What? Where?

[NicolasCARPi]

@RobThree I see a major issue with the suggested packages EndroidQrCode and BaconQrCode.

Reading up a bit on this issue, I see four solutions:

• We add it as a dependency (not great but most simple, straightforward and efficient solution)
• We create a package that requires both EndroidQrCode and TwoFactorAuth (and another one for BaconQrCode), meaning with take this code out.
• We keep it as it is, but how am I supposed to test this, as I cannot install composer dependencies without touching the composer.json file, see: Install a package without saving it into composer.json ? composer/composer#7061 (comment)
• Another solution that I don't know about!

Adding it as require-dev is a bad pattern, see: composer/composer#8184 (comment).

[NicolasCARPi]

What? Where?

For instance, and this is related to migrating to 8.1, openssl_random_pseudo_bytes throws exceptions now, so we don't need to check for false because it will never happen, not really a bug, but dead code.

BTW, does it makes sense to keep it around, given that the superior random_bytes() is guaranteed to be there? Given that we're an authentication library, I believe it makes sense to only use cryptographically secure generators (and remove providers that are not secure altogether).

EDIT: I would also suggest removing external http calls to apis to get the qrcode. It's fine to use it, but not in a context where the data holds a secret. Makes the lib inherently more secure because you can't configure it to leak secrets, and removes maintenance!

I've also found a few type hints in the doc that were incorrect (regarding nullability), nothing major, don't worry ;)

Code is super clean and very easy to work with, you've left comments where needed and the structure makes sense 👏

I've installed php-cs-fixer and phpstan to make it even cleaner (and automate things). Currently phpstan is mostly barking about the issue above.

[willpower232]

You can see I was able to skirt around the issue with separate github workflows to only run specific tests when bacon and endroid were specifically installed

https://github.com/RobThree/TwoFactorAuth/tree/master/.github/workflows

Being a phpstan user myself, I understand it will be getting upset about testing classes that don't exist. Off the top of my head, I don't think there is an easy solution other than having multiple phpstan.neon files with specific conditions in.

Alternatively, they'd have to be spun off into their own packages with completely separate maintenance and testing regimes applied.

Whilst I do agree with the sentiment around the external HTTP calls, the providers that do this provide a quick start without needing the users to add an additional dependency so spinning them off into their own packages would definitely add some confusion IMHO.

Looking forward to the PR!

[NicolasCARPi]

Looking forward to the PR!

Look no more! ;) #97

[willpower232]

Exciting times! I was just flicking through it. I don't know if you can include 8.2 in the matrix in the workflow files yet but other than that, my only critique would be listing the constructor arguments so it isn't one long line 😅

    public function __construct(
        public string $errorcorrectionlevel = 'L',
        public string $bgcolor = 'ffffff',
        public string $color = '000000',
        public string $format = 'p'
    ) {

[willpower232]

Oh I just activated actions for you so that should be happening now. Also what setting is phpstan on? I vaguely remember the default level being quite low and I think I've been using level 8 at the moment but maybe it goes higher now?

[NicolasCARPi]

my only critique would be listing the constructor arguments so it isn't one long line

Yes I agree.

Also what setting is phpstan on?

3. Planning on increasing gradually.

[NicolasCARPi]

I don't know if you can include 8.2 in the matrix

Yep, just had to add PHP_CS_FIXER_IGNORE_ENV=1 for php-cs-fixer.

[willpower232]

Just popping in whilst clearing up my notes, github is changing how they handle PHP versions in the actions workflow, I'm not sure if we'd have to involve docker or something to ensure we can still test against unsupported PHP versions or if the existing steps in the workflow will just deal with it anyway.

actions/runner-images#6331
actions/runner-images#6399

[NicolasCARPi]

@willpower232 We can stay on 20.04 for the time being. Then use https://github.com/shivammathur/setup-php. I don't see this as a big issue.

[NicolasCARPi]

Thanks everyone for help with reviews, closing this now :)

[f1-outsourcing]

I think the push comes from the fact that old versions don't receive security patches, leaving systems vulnerables. Allowing the install on older versions is enabling "them".

@NicolasCARPi But you are not responsible for security. You are a developer. Or are you telling me that you monitor all backports of all linux distributions into older versions every single day? Of course not. If you are dentist, you should not be advising on neurology. Very simple concept. How is it possible that we still need to discuss the division of labour, a concept that has been around for millenniums. Just stick to your library, looks like quite good work.

Look at wordpress, it used to support antique php versions for ages, leaving a lot of websites open to attacks (most of them quite easy to leverage).

Yes I wrote them also about it. Besides with wordpress you really do not need to focus on php versions, with so many crappy developers contributing there.

[NicolasCARPi]

Just stick to your library

If you're going to tell me what to do, allow me to do the same:

Please leave us alone. You're toxic and unwelcome here. I've looked at some of the other issues you opened: you're very negative, toxic and disagreeable everywhere you go. We don't need people like you. Go away.

[RobThree]

I blocked the user. Nothing but negativity and a big mouth for someone with zero contributions to anything.

[NicolasCARPi]

Good decision.