Skip to content

Add PHP 7.4, fix Psalm issues#113

Merged
paragonie-security merged 7 commits into
paragonie:masterfrom
Slamdunk:fix_build
Jan 20, 2021
Merged

Add PHP 7.4, fix Psalm issues#113
paragonie-security merged 7 commits into
paragonie:masterfrom
Slamdunk:fix_build

Conversation

@Slamdunk

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread composer.json Outdated
"require-dev": {
"phpunit/phpunit": "^6|^7",
"vimeo/psalm": "^1|^2"
"vimeo/psalm": "^2|^3"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version 2 has 7.0 as minimum requirement, so v1 is not required anymore

Comment thread psalm.xml
<MoreSpecificReturnType errorLevel="info" />
<InternalMethod>
<errorLevel type="suppress">
<referencedMethod name="ParagonIE_Sodium_Core_Util::store64_le" />

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #111 point [1]: IMHO ParagonIE_Sodium_Core_Util should be a isolated package

Comment thread src/Protocol/Version2.php
(string) $footer
);
if (!\is_string($message)) {
throw new PasetoException('Invalid message decryption');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot test this, but needed for proper return type consistency

Comment thread src/Protocol/Version2.php
SymmetricKey $key,
string $footer = ''
): string {
) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that happens, then we need to be stricter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/
public static function protocolFromHeaderPart(string $headerPart): ProtocolInterface {
if (empty(self::$headerLookup)) {
/** @var ProtocolInterface $protocolClass */

@Slamdunk Slamdunk Apr 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This messes IDEs, but Psalm correctly infers type from @const array<int, class-string<ProtocolInterface>> notation added above

Comment thread .travis.yml
- php: 7.2
- php: 7.3
- php: 7.4
env: FULL_TEST=1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error given in the last failing build:
https://travis-ci.org/github/paragonie/paseto/jobs/677231516#L318-L321
is due to Psalm bugs fixed in more recent version, but not in the older ones.
In general, test shall be done on every version, of course, but static analysis is enough only on the latest one.

@Slamdunk

Copy link
Copy Markdown
Contributor Author

Ping @paragonie-security

@lhchavez

Copy link
Copy Markdown

would you mind also adding

diff --git a/src/Parsing/PasetoMessage.php b/src/Parsing/PasetoMessage.php
index 8fe4839..132775d 100644
--- a/src/Parsing/PasetoMessage.php
+++ b/src/Parsing/PasetoMessage.php
@@ -47,7 +47,6 @@ final class PasetoMessage
      * @throws SecurityException
      * @throws InvalidVersionException
      * @throws InvalidPurposeException
-     * @return PasetoMessage
      * @throws \TypeError
      */
     public static function fromString(string $tainted): self

? That fixes another issue with the latest Psalm, since that function has a duplicate @return docstring tag.

@Slamdunk

Copy link
Copy Markdown
Contributor Author

@lhchavez done, thank you

@Slamdunk

Copy link
Copy Markdown
Contributor Author

@Slamdunk

Slamdunk commented Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

@paragonie-security as soon as this gets merged, I'll add a PR for PHP 8 compatibility

@stayallive

Copy link
Copy Markdown

I was looking to see if PHP 8 was on the radar, would be greatly appreciated if that is happening 👍

@paragonie-security paragonie-security merged commit 77ae275 into paragonie:master Jan 20, 2021
@Slamdunk Slamdunk deleted the fix_build branch January 20, 2021 05:51
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.

4 participants