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

[5.1] Add PHP 8.4 to CI #126

Merged
merged 3 commits into from
Jul 25, 2024
Merged

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jul 24, 2024

Part of issue #123

  • add PHP 8.4 to CI
  • chore: run cs-fixer regardless of PHP version (forcing it to run on PHP 8.4 in CI - it passes)
  • explicitly mark nullable parameters like ?callback ?string. This was supported since PHP 7.1, so that is very convenient because the 5.1.x release series still supports all the way back to PHP 7.1

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.72%. Comparing base (2413496) to head (558d67f).
Report is 1 commits behind head on 5.1.

Additional details and impacted files
@@            Coverage Diff            @@
##                5.1     #126   +/-   ##
=========================================
  Coverage     99.72%   99.72%           
- Complexity      110      118    +8     
=========================================
  Files             7        7           
  Lines           365      365           
=========================================
  Hits            364      364           
  Misses            1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phil-davis phil-davis marked this pull request as ready for review July 24, 2024 15:12
@phil-davis phil-davis self-assigned this Jul 24, 2024
@@ -74,5 +74,5 @@ public function removeListener(string $eventName, callable $listener): bool;
* removed. If it is not specified, every listener for every event is
* removed.
*/
public function removeAllListeners(string $eventName = null);
public function removeAllListeners(?string $eventName = null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these kind of changes, if someone was implementing this interface, and has string $eventName = null and we change this "underneath them" adding the ?, then will their code work unchanged?

LSP is not broken - the old and new interface, and the implementation of the interface all have exactly the same set of allowable things that can be passed in the parameter.

I wonder if this will break anything for anyone (e.g. in older versions of PHP) (callers will be fine - now it is explicit that they can pass null)

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the following code in both PHP 7.4 and PHP 8.3 and it does not trigger any error.

interface TestI {
    public function test(?string $a = null);
}
class Cls implements TestI {
    public function test(string $a = null) {}
}

@phil-davis phil-davis requested a review from staabm July 24, 2024 15:24
@phil-davis
Copy link
Contributor Author

@cedric-anne IMO this will support PHP 8.4 for the 5.1.x series. Please review and comment.

Then I can check these things in master (which might already be OK), and try similar in other sabre-io repos.

@@ -74,5 +74,5 @@ public function removeListener(string $eventName, callable $listener): bool;
* removed. If it is not specified, every listener for every event is
* removed.
*/
public function removeAllListeners(string $eventName = null);
public function removeAllListeners(?string $eventName = null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the following code in both PHP 7.4 and PHP 8.3 and it does not trigger any error.

interface TestI {
    public function test(?string $a = null);
}
class Cls implements TestI {
    public function test(string $a = null) {}
}

@phil-davis phil-davis merged commit abebcbd into sabre-io:5.1 Jul 25, 2024
11 checks passed
@phil-davis phil-davis deleted the add-php-8.4-to-ci branch July 25, 2024 08:00
@phil-davis phil-davis mentioned this pull request Jul 25, 2024
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.

3 participants