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

feat: add support for PHP 8.2 ✨ #2359

Closed
wants to merge 19 commits into from

Conversation

Nielsvanpach
Copy link
Contributor

@Nielsvanpach Nielsvanpach commented Dec 28, 2022

Main changes

  • Run tests with PHP 8.2
  • Run all php versions with --prefer-lowest and --prefer-stable
  • Added 8.1 where missing
  • Run actions on feature branches

@Nielsvanpach Nielsvanpach requested a review from a team as a code owner December 28, 2022 12:53
@Nielsvanpach Nielsvanpach changed the title Add support for PHP 8.2 ✨ feat: add support for PHP 8.2 ✨ Dec 28, 2022
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, but you're making a lot of changes here that we either don't want, are covered by #2350, or are confusing to me.

The only changes here I think we would need are:

  1. Upgrading nickinvision to v2 (why not?)
  2. Updating .github/sync-repo-settings.yaml, although we don't need to add all the prefer lowest / stables
  3. Potentially adding ReturnTypeWillChange to Google\Collection, although if this is a problem then I'm surprised that hasn't been reported elsewhere, and if so it still should be in a separate PR.

The changes I don't want here are:

  1. expanding the matrix to run prefer-lowest in every case
  2. adding all those extensions to the PHP build
  3. adding the plugin to composer.json (why?)
  4. running the CI on feature branches

@Nielsvanpach
Copy link
Contributor Author

Nielsvanpach commented Dec 29, 2022

  1. expanding the matrix to run prefer-lowest in every case
    By including this in the CI, we can ensure that the lowest required version of the composer packages are supported as claimed in composer.json. This feels important for maintaining the integrity of the package, especially since it supports a wide range of PHP versions. If a unit test fails with older dependency versions, it gives us the opportunity to determine if we need to increase the lowest allowed version of a package, drop support for a particular php version, or find a workaround to resolve the issue. By running this test on CI, you can catch and address any issues that may arise, ensuring that the package remains compatible for all users.
    That's why I also had to upgrade the lowest allowed versions of phpunit, they were not compatible. Fortunately this was only a dev requirement.

  2. adding all those extensions to the PHP build
    I originally included this to specify which extensions are required for the package (still have to remove what's not needed). But I can see why you don't need this, so I will remove it. By the way, most of the required extensions are already loaded by default: https://github.com/shivammathur/setup-php/wiki/Php-extensions-loaded-on-ubuntu-20.04

  3. adding the plugin to composer.json (why?)
    It's an approved security layer and added by composer itself. Perhaps you are running a version lower than 2.2.0 ? https://getcomposer.org/doc/06-config.md#allow-plugins

  4. running the CI on feature branches
    This makes it possible for contributors to have feedback while developing a feature for this repo. I think at the moment you only receive feedback when you open a PR? It feels better to only open a PR when you know the feature is stable and it avoids the need to fix issues after the PR has been opened.

@bshaffer
Copy link
Contributor

bshaffer commented Dec 29, 2022

  1. expanding the matrix to run prefer-lowest in every case
    This is a difference in philosophy. I feel that running prefer-lowest for the minimum version of PHP and maximum is sufficient.

  2. adding all those extensions to the PHP build
    Thank you!

  3. adding the plugin to composer.json (why?)
    My question is more that I don't know what dealerdirect/phpcodesniffer-composer-installer is, or why we need it.

  4. running the CI on feature branches
    I believe the reason you want this change is so that when contributors fork this repo and push to a feature branch on their own fork, their changes will execute a workflow run in their own fork. The problem is, if we made this change in our repo, every PR or push to a feature branch results in two workflow runs - once for the branch, and once for the PR. In the past we have found this duplication of workflow executions confusing, inefficient, and unnecessary, and that is why we have restricted workflow runs to only the main branch and PRs. To execute a workflow on a fork, you can either submit a PR to the main branch of the fork, or change the file as you have here in the fork. Additionally you can submit a PR here and we will run it. We could also add workflow_dispatch to the action, so contributors can manually run the actions on their forks.

@Nielsvanpach
Copy link
Contributor Author

  1. Updated
  2. Updated
  3. Removed it, but this will pop up again next time composer is run. It's coming from a dev dependency: phpcompatibility/php-compatibility
  4. Updated

@bshaffer bshaffer closed this May 1, 2023
@bshaffer
Copy link
Contributor

bshaffer commented May 1, 2023

These items will be addressed in #2431

@Nielsvanpach Nielsvanpach deleted the php82 branch May 2, 2023 07:29
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