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

PHP Docker update from 8.0 to 8.3 in CI #9530

Closed
wants to merge 63 commits into from

Conversation

kongtiaowang
Copy link
Contributor

@kongtiaowang kongtiaowang commented Jan 13, 2025

First Step: 8.3
PHP Docker update from 8.0 to 8.3 in CI
Test: pass CI
todo: add 8.4

@kongtiaowang kongtiaowang reopened this Jan 13, 2025
@kongtiaowang kongtiaowang changed the title PHP Docker update from 8.0 to 8.3 PHP Docker update from 8.0 to 8.3 in CI Jan 13, 2025
@kongtiaowang kongtiaowang added the State: Needs work PR awaiting additional work by the author to proceed label Jan 13, 2025
@driusan
Copy link
Collaborator

driusan commented Jan 20, 2025

@kongtiaowang does this still need work or should it be assigned?

@kongtiaowang kongtiaowang added Area: CI PR or issue related to CI (GitHub actions, automated tests, static checks...) Testing PR contains test plan or automated test code (or config files for Travis) and removed State: Needs work PR awaiting additional work by the author to proceed labels Jan 24, 2025
@kongtiaowang kongtiaowang force-pushed the 2025-01-09-a branch 2 times, most recently from 5edb4d2 to 1c02bdd Compare January 24, 2025 16:48
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

This is nearly impossible to review because of all the declare(strict_type) whitespace changes. Can you revert them? If a default changed in something that is causing them, you can update the config to use the old behaviour of no space after <?php

@@ -31,7 +31,10 @@
"suppress_issue_types" => [
"PhanUnusedPublicNoOverrideMethodParameter",
// Until phan/phan#4746 is fixed
"PhanTypeMismatchArgumentInternal"
"PhanTypeMismatchArgumentInternal",
"PhanUndeclaredMethod",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these being added? If we've already fixed this class of errors they shouldn't be re-added.

"phan/phan": "^5.0",
"phpstan/phpstan": "^1.4",
"slevomat/coding-standard": "^6.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If these changes are causing large amounts of changes, I think they should be done 1 at a time in different pull requests to make it easier to review and test them individually.

@skarya22
Copy link
Contributor

@kongtiaowang todo: close this PR and create a new one with less files changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI PR or issue related to CI (GitHub actions, automated tests, static checks...) Testing PR contains test plan or automated test code (or config files for Travis)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants