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

Minimum PHP 7.4 #74

Merged
merged 4 commits into from
Aug 17, 2022
Merged

Minimum PHP 7.4 #74

merged 4 commits into from
Aug 17, 2022

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Aug 11, 2022

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.

@phil-davis phil-davis self-assigned this Aug 11, 2022
@phil-davis phil-davis changed the title Minimum php 7.4 Minimum PHP 7.4 Aug 11, 2022
@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #74 (01e0b60) into master (cd2476f) will decrease coverage by 0.74%.
The diff coverage is 50.00%.

❗ Current head 01e0b60 differs from pull request most recent head bdc592c. Consider uploading reports for the commit bdc592c to get more accurate results

@@             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     
Impacted Files Coverage Δ
lib/functions.php 99.25% <50.00%> (-0.75%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +312 to +326
if (null === $uri) {
throw new InvalidUriException('Invalid, or could not parse URI');
Copy link
Contributor Author

@phil-davis phil-davis Aug 11, 2022

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.

lib/Version.php Outdated
@@ -16,5 +16,5 @@ class Version
/**
* Full version number.
*/
const VERSION = '2.2.2';
public const VERSION = '2.2.2';
Copy link
Contributor Author

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.

@phil-davis phil-davis marked this pull request as ready for review August 17, 2022 08:29
@phil-davis
Copy link
Contributor Author

@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__);
Copy link
Member

Choose a reason for hiding this comment

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

append([__FILE__])

@phil-davis phil-davis merged commit b6204d6 into sabre-io:master Aug 17, 2022
@phil-davis phil-davis deleted the minimum-php-7.4 branch August 17, 2022 09:45
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.

2 participants