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

Update to symfony7 #70

Merged
merged 14 commits into from
Apr 5, 2024
Merged

Update to symfony7 #70

merged 14 commits into from
Apr 5, 2024

Conversation

larowlan
Copy link
Contributor

@larowlan larowlan commented Apr 3, 2024

Please review the 4 matrix jobs in CI to make sure they provide good coverage.

Comment on lines 102 to 106
php-version: "8.3"
dependencies: highest

- os: ubuntu-latest
php-version: "8.0"
dependencies: highest
php-ini-values: assert.exception=1, zend.assertions=1, opcache.enable=1, opcache.enable_cli=1, opcache.optimization_level=-1, opcache.jit_buffer_size=4096M, opcache.jit=1205

- os: ubuntu-latest
php-version: "8.1"
php-version: "8.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have two 8.3 and highest here - do we need both? is it because of the ini values?

Copy link
Member

Choose a reason for hiding this comment

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

The tests were previously 8.0 and 8.1. As you said, there is no need to do 8.3 twice. The base test should be 8.2, so if we wanted to test all versions, we could list 8.0, 8.1 and 8.3 here. Maybe it's okay to not do all of versions and just pick some representative ones.

@@ -24,7 +24,7 @@ jobs:
- name: Install PHP
uses: shivammathur/setup-php@v2
with:
php-version: 7.4
php-version: 8.3
Copy link
Member

Choose a reason for hiding this comment

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

It's best to make the base test the lowest supported version PHP supported by the composer.lock file, e.g. 8.2.

@@ -82,30 +85,25 @@ jobs:
- windows-latest

php-version:
- "7.4"
- "8.3"
Copy link
Member

Choose a reason for hiding this comment

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

8.2


php-ini-values:
- assert.exception=1, zend.assertions=1

dependencies:
- locked
Copy link
Member

Choose a reason for hiding this comment

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

The base test should be "locked", not "highest"


include:
- os: ubuntu-latest
php-version: "7.4"
dependencies: lowest

- os: ubuntu-latest
php-version: "7.4"
php-version: "8.3"
Copy link
Member

Choose a reason for hiding this comment

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

Keep this "7.4" so that we can do a "highest" test on the older php version.

composer.json Outdated
@@ -53,7 +53,7 @@
"optimize-autoloader": true,
"sort-packages": true,
"platform": {
"php": "7.4"
"php": "8.3"
Copy link
Member

@greg-1-anderson greg-1-anderson Apr 5, 2024

Choose a reason for hiding this comment

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

8.2.17

@greg-1-anderson greg-1-anderson merged commit df830bd into consolidation:main Apr 5, 2024
7 checks passed
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