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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise PHP functional level to 7.0 #1242

Open
wants to merge 46 commits into
base: master
from
Open

Raise PHP functional level to 7.0 #1242

wants to merge 46 commits into from

Conversation

@tvdijen
Copy link
Member

tvdijen commented Nov 20, 2019

  • Raise the minimum version to PHP 7.0
  • Upgrade dev dependencies
  • Fix Psalm-issues
  • Add typehints for scalar types on private methods

TO DO:

@tvdijen tvdijen added this to the 1.19 milestone Nov 20, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 20, 2019

Codecov Report

Merging #1242 into master will decrease coverage by 0.28%.
The diff coverage is 44.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1242      +/-   ##
============================================
- Coverage     36.82%   36.53%   -0.29%     
+ Complexity     3785     3764      -21     
============================================
  Files           138      138              
  Lines         11544    11456      -88     
============================================
- Hits           4251     4186      -65     
+ Misses         7293     7270      -23
Impacted Files Coverage Δ Complexity Δ
lib/SimpleSAML/Metadata/MetaDataStorageHandler.php 33.58% <ø> (ø) 51 <0> (ø) ⬇️
lib/SimpleSAML/Error/Exception.php 20.45% <ø> (ø) 32 <0> (ø) ⬇️
modules/core/lib/Auth/UserPassBase.php 29.85% <ø> (ø) 21 <0> (ø) ⬇️
lib/SimpleSAML/IdP/TraditionalLogoutHandler.php 0% <ø> (ø) 9 <0> (ø) ⬇️
lib/_autoload_modules.php 68.53% <ø> (+0.79%) 0 <0> (ø) ⬇️
lib/SimpleSAML/Utils/EMail.php 0% <ø> (ø) 30 <0> (ø) ⬇️
modules/core/lib/ACL.php 0% <0%> (ø) 51 <39> (ø) ⬇️
modules/core/lib/Auth/Process/GenerateGroups.php 0% <0%> (ø) 17 <5> (ø) ⬇️
lib/SimpleSAML/XHTML/IdPDisco.php 0% <0%> (ø) 69 <4> (ø) ⬇️
lib/SimpleSAML/Metadata/Signer.php 0% <0%> (ø) 32 <26> (ø) ⬇️
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f389044...3a43b99. Read the comment docs.

@tvdijen tvdijen force-pushed the php70 branch from cf30191 to d528522 Nov 24, 2019
@tvdijen tvdijen force-pushed the php70 branch from ded0b90 to 4ea38fd Nov 28, 2019
Copy link
Member

jaimeperez left a comment

Looks good to me overall, but I believe there are a couple of bugs.

lib/SimpleSAML/Database.php Outdated Show resolved Hide resolved
lib/SimpleSAML/Logger.php Outdated Show resolved Hide resolved
lib/SimpleSAML/Utils/Config/Metadata.php Outdated Show resolved Hide resolved
lib/SimpleSAML/Utils/Net.php Show resolved Hide resolved
@@ -1165,7 +1165,7 @@ public static function setCookie($name, $value, $params = null, $throw = true)
'lifetime' => 0,
'expire' => null,
'path' => '/',
'domain' => null,
'domain' => version_compare(PHP_VERSION, '7.3.0', '>=') ? null : '',

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Nov 29, 2019

Member

I assume this was something psalm was complaining about. Do you remember what was it?

This comment has been minimized.

Copy link
@tvdijen

tvdijen Nov 29, 2019

Author Member

It has to do with the different signatures for setcookie and setrawcookie pre/post PHP 7.3..
Pre 7.3 we need a string, post we need $options['domain'].. We used to set it to null for pre 7.3 as well and that's an issue with strict_types.. This happened to be the easiest fix

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Dec 12, 2019

Member

I see...

From what I understand from the documentation, the options array accepts keys with values the same as the parameters in the original function's signature. Would it then be possible to just set it to ' ', regardless the PHP version we are running?

This comment has been minimized.

Copy link
@tvdijen

tvdijen Dec 12, 2019

Author Member

Maybe I'm missing the point, but the options-array wasn't added until PHP 7.3?

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Dec 12, 2019

Member

Yes, but I'm not talking about using that signature unconditionally, but about this specific change setting the domain to null or an empty string depending on the PHP version. Maybe I'm misinterpreting your change, but the outcome here is that for PHP 7.3 or later, you will call setcookie() or setrawcookie() with an $options array where the default value for 'domain' is null. If it's an older version of PHP, we're passing an empty string to the $domain parameter.

This comment has been minimized.

Copy link
@tvdijen

tvdijen Dec 12, 2019

Author Member

Ah right, I see what you mean now.. The empty string will cause an implicit cast to null, which is going to bug us again if we merge the strict_types=1 PR..

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Dec 12, 2019

Member

Uhm, why should it implicitly cast to null? I think I'm missing something, because setting an element of an array to an empty string will (and should) result in that element being a string. Empty, but a string:

php > $a = [ 'b' => '' ];
php > var_dump($a);
array(1) {
  ["b"]=>
  string(0) ""
}

This comment has been minimized.

Copy link
@tvdijen

tvdijen Dec 12, 2019

Author Member

Nevermind, I'm an idiot.. Took care of it!

lib/SimpleSAML/Metadata/SAMLParser.php Show resolved Hide resolved
tests/www/TemplateTest.php Show resolved Hide resolved
tests/lib/SimpleSAML/XML/SignerTest.php Outdated Show resolved Hide resolved
tests/lib/SimpleSAML/XML/SignerTest.php Show resolved Hide resolved
tests/lib/SimpleSAML/XML/ValidatorTest.php Outdated Show resolved Hide resolved
@tvdijen tvdijen force-pushed the php70 branch 2 times, most recently from 7e20e6f to 074b7cf Nov 29, 2019
@tvdijen tvdijen force-pushed the php70 branch from 074b7cf to e5c7981 Nov 29, 2019
Copy link
Member Author

tvdijen left a comment

See my answers.. Some things have been fixed, others are up for discussion

tests/www/TemplateTest.php Show resolved Hide resolved
@tvdijen tvdijen force-pushed the php70 branch 2 times, most recently from 2935f1d to 448a618 Nov 30, 2019
@tvdijen tvdijen force-pushed the php70 branch from 448a618 to 812b4de Dec 2, 2019
@tvdijen tvdijen requested a review from jaimeperez Dec 6, 2019
lib/SimpleSAML/Database.php Outdated Show resolved Hide resolved
lib/SimpleSAML/Logger.php Outdated Show resolved Hide resolved
Copy link
Member

jaimeperez left a comment

It looks really good! 👍

There's only one thing that I think should be addressed, and that's the added assertions.

@@ -1165,7 +1165,7 @@ public static function setCookie($name, $value, $params = null, $throw = true)
'lifetime' => 0,
'expire' => null,
'path' => '/',
'domain' => null,
'domain' => version_compare(PHP_VERSION, '7.3.0', '>=') ? null : '',

This comment has been minimized.

Copy link
@jaimeperez

jaimeperez Dec 12, 2019

Member

I see...

From what I understand from the documentation, the options array accepts keys with values the same as the parameters in the original function's signature. Would it then be possible to just set it to ' ', regardless the PHP version we are running?

lib/SimpleSAML/Utils/Config/Metadata.php Outdated Show resolved Hide resolved
@tvdijen

This comment has been minimized.

Copy link
Member Author

tvdijen commented Dec 12, 2019

I think all is taken care of now.. We do need a new release of saml2 though

.travis.yml Outdated
env: Psalm
before_script:
- composer update
- composer require simplesamlphp/saml2:dev-patch-return-type

This comment has been minimized.

Copy link
@tvdijen

tvdijen Dec 12, 2019

Author Member

@jaimeperez It's all about this line..

tvdijen added 3 commits Dec 12, 2019
A new release of saml2 will solve this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.