-
Notifications
You must be signed in to change notification settings - Fork 19
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
Minimum PHP 7.4 #74
Minimum PHP 7.4 #74
Conversation
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
===========================================
- Coverage 100.00% 99.25% -0.75%
===========================================
Files 1 1
Lines 143 135 -8
===========================================
- Hits 143 134 -9
- Misses 0 1 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
if (null === $uri) { | ||
throw new InvalidUriException('Invalid, or could not parse URI'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this keeps phpstan happy, otherwise it gets worried that preg_replace_callback
might return null
and that would cause trouble in later code that uses $uri
. I couldn't find a value for the input parameter $uri
that will actually cause null
- I have to pass in a string (otherwise PHP will get an error on the call itself). So this seems untestable. And thus we get the Codecov warning.
ee584e0
to
438f408
Compare
lib/Version.php
Outdated
@@ -16,5 +16,5 @@ class Version | |||
/** | |||
* Full version number. | |||
*/ | |||
const VERSION = '2.2.2'; | |||
public const VERSION = '2.2.2'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should not cause any backward-compatibility problem for any consumer that is running at least PHP 7.4. The name and value of the constant is available to consumers just like before.
32fbacb
to
01e0b60
Compare
01e0b60
to
bdc592c
Compare
@staabm does this look OK to you? I think that we can do this repo without bumping the major version. |
@@ -1,12 +1,13 @@ | |||
<?php | |||
|
|||
$config = PhpCsFixer\Config::create(); | |||
$config->getFinder() | |||
$finder = PhpCsFixer\Finder::create() | |||
->exclude('vendor') | |||
->in(__DIR__); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
append([__FILE__])
This makes PHP 7.4 the minimum version of PHP.
I am doing this first in sabre-io/uri because it is at the bottom of the dependency tree, so it can happen first, and then other sabre-io repos can make use of it as they do similar.
Note: I don't see any real code changes that would cause any sort of incompatibility for current consumers of major v2 of this repo. I have only had to touch tool configs, parameter types in test code, and setting "public const" in 1 place. I don't think that any of that will be visible/noticeable to real consumers of this code. So probably we do not need to bump the major version in this repo. The "real" code is in
lib/functions.php
and that already has parameter and return types declared, and it is not a class, so there are no class variables etc to add type declarations...Just a bump from 2.2.3 to 2.3.0 should be fine.
composer will be smart enough to only select 2.3.0 for projects that have min PHP 7.4 specified. On projects that still support PHP 7.1 7.2 or 7.3, composer will only offer/install release 2.2.3.
Note: in other repos that have more "real" code that needs touching, we will likely need to bump the major version.