PHP 8.1 #10
PHP 8.1 #10
Conversation
| uses: shivammathur/setup-php@v2 | ||
| with: | ||
| php-version: '8.2' | ||
| coverage: xdebug |
There was a problem hiding this comment.
This is just for "convenience". There could also be a tighter integration with coverage tools in GitHub (I think, but I have never done that).
|
|
||
| - name: Run PHPUnit | ||
| run: vendor/bin/phpunit | ||
| run: vendor/bin/phpunit --coverage-text |
There was a problem hiding this comment.
See comment about coverage above. Currently the result is just printed in the action run.
| use AltchaOrg\Altcha\Altcha; | ||
|
|
||
| $hmacKey = 'secret hmac key'; | ||
| $altcha = new Altcha('secret hmac key'); |
There was a problem hiding this comment.
New object-oriented usage of Altacha.
The idea is having to "know" the key only once. This could enable tighter integration with frameworks, where Altcha could be initialized using a central key, e.g., APP_SECRET in Symfony.
|
|
||
| namespace AltchaOrg\Altcha\Hasher; | ||
|
|
||
| enum Algorithm: string |
There was a problem hiding this comment.
An enum makes it a lot easier for users to discover all available options. They don't even have to be documented or validated when they're used (only when instantiating from input data).
|
|
||
| namespace AltchaOrg\Altcha\Hasher; | ||
|
|
||
| class Hasher implements HasherInterface |
There was a problem hiding this comment.
A separate implementation for methods related to hashing remove public methods from Altcha (which apparently were only used for tests anyway) and help with testing at the same time.
| $score, | ||
| $time, | ||
| $verified, | ||
| classification: isset($params['classification']) && is_string($params['classification']) ? $params['classification'] : '', |
There was a problem hiding this comment.
This is just to make the code easier to read. Still not optimal, but close to the original.
| } | ||
|
|
||
| return new ServerSignaturePayload($data['algorithm'], $data['verificationData'], $data['signature'], $data['verified']); | ||
| $algorithm = Algorithm::tryFrom($data['algorithm']); |
There was a problem hiding this comment.
Here's where validation of the "user" input happens, so I'm also checking for the value of algorithm.
|
I wasn't sure whether, apart from the already bigger changes to the public interface, you'd approve of switching to Exceptions instead of The big benefit of exceptions would be that for each failure (invalid algorithm, invalid expiry time, invalid solution) an exception could tell the users what went wrong. On the other hand, this, again, completely changes to interface. Let me know what you think! |
The interface should be more or less consistent between libraries, but I think you can add a new function |
|
I wouldn't add that to this PR anyway. |
|
@ovx From my point of view this PR is good to go. PHP 8.1 will get security updates until 12/2025, so I guess that's a reasonable version to support. |
|
Since the PR is about code improvements and modernization, another question in this regard. |
|
@EvilKarter That's a good question! For my daily work such tools have always gotten in my way, because I often want to format code breaking a CS rule. For me ultimately code formatting is to improve its readability and if I, for example, want to align some array content to make it easier see columns, an automated tool will undo that. I've had numerous other issues like that in the past forcing my to switch off formatting with comments, making the code even more polluted. So I don't use them any more and rely on the IDE and manual intervention. For a library with multiple contributors PHP-CS would be a good idea I think. |
|
@aleho I think a combination of PHP-CS-Fixer and a matching .editorconfig file should make contributions simpler, because there are no diskussion about the "right" formatting. The project is relatively small and the functions are not overly complex. So I don't think there are any edge cases here where formatting by hand significantly increases readability. |
|
Agreed. I'd opt for a separate PR though. This one's too big in my opinion already. |
3 participants
This is a follow-up for #9.
It switches to OOP, uses modern PHP 8.1+ features (enums, promoted properties, union types) and improves tests.
Refs #8