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

Run phpcs, php-cs-fixer, phpstan, coverage, versions as Php8.1 #3450

Merged
merged 7 commits into from
Mar 14, 2023

Conversation

oleibman
Copy link
Collaborator

They all run under Php7.4 in Github. 7.4 is EOL. We still have to run unit tests in 7.4, but I think it's time to move the tools. Note that we cannot currently run Phpstan in 8.2 because of phpstan/phpstan#8629.

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

They all run under Php7.4 in Github. 7.4 is EOL. We still have to run unit tests in 7.4, but I think it's time to move the tools. Note that we cannot currently run Phpstan in 8.2 because of phpstan/phpstan#8629.
@oleibman oleibman marked this pull request as draft March 10, 2023 18:00
@oleibman
Copy link
Collaborator Author

Hmm, everything worked, except coverage, which has failed twice. Once with no error that I could see, then once, after completing the unit tests, with the following trying to upload its results:

  [Guzzle\Common\Exception\InvalidArgumentException]  
  Invalid handle provided  

I will try it as 8.0 (leave the others at 8.1) and see if that helps.

Try running coverage as 8.0 rather than 8.1.
@oleibman
Copy link
Collaborator Author

Same error for Coverage with Php 8.0. Trying 7.4.

Revert Coverage to Php 7.4.
@oleibman
Copy link
Collaborator Author

@MarkBaker @PowerKiKi Any idea why Coverage results upload should fail using Php 8.1/8.0 but succeed with Php 7.4? Message for 8.1/8.0 documented above.

That error notwithstanding, is this change (with or without Coverage change) a bad idea? Good idea? It depends?

Command set-output is deprecated. Upgrading to using Environment files as suggested by Github messages.
@MarkBaker
Copy link
Member

@MarkBaker @PowerKiKi Any idea why Coverage results upload should fail using Php 8.1/8.0 but succeed with Php 7.4? Message for 8.1/8.0 documented above.

No idea why the upload should be failing, other than perhaps the underlying version of guzzle or curl.

That error notwithstanding, is this change (with or without Coverage change) a bad idea? Good idea? It depends?

Good idea, because we'll be dropping PHP 7.4 support in a few months time

@MarkBaker
Copy link
Member

scrutinizer-ci/ocular#62

@oleibman oleibman marked this pull request as ready for review March 12, 2023 16:51
@oleibman oleibman changed the title WIP Run phpcs, php-cs-fixer, phpstan, coverage, versions as Php8.1 Run phpcs, php-cs-fixer, phpstan, coverage, versions as Php8.1 Mar 12, 2023
@oleibman
Copy link
Collaborator Author

Thank you. It took a couple of tries to get it right, but now appears to be working.

@oleibman oleibman merged commit 4b7aa20 into PHPOffice:master Mar 14, 2023
@oleibman oleibman deleted the versiontools branch March 24, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants