Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix syntax error #1962

Merged
merged 1 commit into from Nov 23, 2023
Merged

fix syntax error #1962

merged 1 commit into from Nov 23, 2023

Conversation

LordSimal
Copy link
Contributor

@LordSimal LordSimal commented Nov 23, 2023

maybe a github action checking the syntax of all php files via php -l would prevent such errors 馃槈

@terrafrost terrafrost merged commit f71cc09 into phpseclib:3.0 Nov 23, 2023
@terrafrost
Copy link
Member

Well it was failing the unit tests:

https://github.com/phpseclib/phpseclib/actions/runs/6967598648

I was just in a rush, trying to wrap that up before I headed to my Mom's place.

Thanks!

@terrafrost
Copy link
Member

A pre-commit hook wouldn't be a universal panacea because what's valid syntax for one PHP version might not be valid for another version. I've been burned a lot on phpseclib 2.0, which is supposed to support PHP 5.3+. Like I'll often default to the short array syntax, which is only valid on PHP 5.4+ and a pre-commit hook prob wouldn't catch that unless I had a complicated one that supported multiple simultaneous versions. And if I had one that complicated it prob wouldn't work on any system but mine.

@LordSimal
Copy link
Contributor Author

LordSimal commented Nov 23, 2023

I admire your mindset to keep this library working for ancient PHP versions as long as possible.

It surely is not my right to decide this but the fact that PHP 8.0 is EOL in 2 days would indicate, that users should upgrade their apps.

Especially looking at https://packagist.org/packages/phpseclib/phpseclib/php-stats <1% of users of this lib are using PHP <7.0

What you do with that information is of course up to you, but I would drop active support for those old versions and go with the modern PHP times which are far more enjoyable 馃榿

@terrafrost
Copy link
Member

For sure, people should upgrade. But dropping support for older PHP versions without corresponding major releases of phpseclib would be BC breaks and if I'm going to break BC in minor releases then all new releases, even if only minor ones, should be treated with extreme skepticism.

Like what if there's a vulnerability in a version of phpseclib but no one wants to upgrade to their latest one because it might break their stuff because I didn't care about BC? And sure, as https://xkcd.com/1172/ illustrates, sometimes even seemingly innocuous changes can break stuff for people, but I'm trying to minimize how often that occurs - not maximize it.

The master branch, which will eventually become phpseclib 4.0, only supports PHP 8.0+. Maybe by the time its released it'll only support 8.3+ idk. And sure, I could stop supporting the 1.0 and 2.0 branches but (1) I do say that they're LTS releases and (2) even I dropped support for those if I drop support for 3.0 before releasing 4.0 then phpseclib is effectively dead in the water.

And the 5.3 / 5.4 example is just the first one that comes to mind. If I did pre-commit hooks with PHP 8.3 and then did \Random\Randomizer::getFloat() and nextFloat() (which is new to PHP 8.3) it'd pass even tho that wouldn't work (without a shim) on PHP 8.2. So if you're gonna support even just the two most recent releases you could still run into issues. I will confess that that example is pretty benign and obscure but the examples do get less obscure the more versions of PHP that you start supporting. And unless I could quit my day job and work on phpseclib fulltime it's just a sad fact of life that phpseclib major releases will take years.

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.

None yet

2 participants