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

Allow Laravel v9 and Symfony v6 #64

Merged
merged 11 commits into from
Feb 26, 2022
Merged

Allow Laravel v9 and Symfony v6 #64

merged 11 commits into from
Feb 26, 2022

Conversation

hmazter
Copy link
Contributor

@hmazter hmazter commented Jan 31, 2022

Update the package to allow it to be included in a Laravel v9 application that uses Symfony component v6.

Also, updated the CI tests to run on PHP 8.1
And bump shivammathur/setup-php to 2.16.0, since version 2.1.0 did not want to install PHP 7.3

@hmazter hmazter changed the title Run tests on PHP 8.1 Allow Laravel v9 and Symfony v6 Jan 31, 2022
@hmazter
Copy link
Contributor Author

hmazter commented Jan 31, 2022

Not sure if it's possible to have a Test App that is installable on both PHP 7.3 and PHP 8.1 with appropriate dependencies.

@mnapoli
Copy link
Member

mnapoli commented Feb 1, 2022

Would it help if we dropped support for Laravel 6?

@hmazter
Copy link
Contributor Author

hmazter commented Feb 2, 2022

I have updated the test app to a version (Laravel 8) that should be installable on everything from PHP 7.3 to PHP 8.1.
But it is actually one version of Laravel that is tested, I don't know if that is an issue?

@kris914g
Copy link

What needs to be done to merge this PR? Would like to have Laravel 9 support 😄

@hmazter
Copy link
Contributor Author

hmazter commented Feb 10, 2022

The thing that fails now in CI is PHPCS can not run because of this:

Composer detected issues in your platform: Your Composer dependencies require a PHP version ">= 8.0.2". You are running 7.4.12.

I'm not sure if I upgrade some dependencies that do not need an upgrade, or if phpcs needs to be updated.

Copy link

@kris914g kris914g left a comment

Choose a reason for hiding this comment

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

Alright, I believe the issue is regarding the composer.json since Laravel 9 has dropped support for all PHP versions below PHP 8. So to fix the issue, we must update the PHP version in composer.json. Laravel version 6 also supports PHP 8 so Laravel version 6 can still be supported, I suppose the following changes should do the trick.

@@ -19,16 +19,16 @@
"php": ">=7.3",

Choose a reason for hiding this comment

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

Suggested change
"php": ">=7.3",
"php": "^8.0",

with:
php-version: 7.3
php-version: '7.3'

Choose a reason for hiding this comment

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

Suggested change
php-version: '7.3'
php-version: '8.0'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this can be bumped, then the test for the old versions will fail.

It's this one that fails, chekalsky/phpcs-action@v1 is built on PHP 7.4 I think.

phpcs:
name: PHPCS
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v2
- name: Download dependencies
run: composer update --no-interaction --prefer-dist --no-progress --no-suggest --dev
- name: PHPCS check
uses: chekalsky/phpcs-action@v1
with:
phpcs_bin_path: './vendor/bin/phpcs'

Choose a reason for hiding this comment

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

I see, just looked at the GA and it is based on cytopia/phpcs:3 docker image, which only supports PHP version 7.4 and it haven't been upgraded for more than a year now, so to satisfy the test we must replace the GA with another CodeSniffer action which supports PHP 8

Copy link
Member

Choose a reason for hiding this comment

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

I'm 👍 with requiring PHP 8 if it helps here.

Also maybe this approach can be copied to get rid of this custom action: https://github.com/brefphp/bref/blob/master/.github/workflows/ci.yml#L75-L96 ?

Copy link

@kris914g kris914g Feb 10, 2022

Choose a reason for hiding this comment

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

That would work, we just need to add the Code_Sniffer package into composer.json and maybe it could be a good idea to just add a matrix to use several PHP versions. I can make a PR with an updated phpcs action later today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the phpcs workflow according to the link @mnapoli sent

@hmazter hmazter marked this pull request as ready for review February 11, 2022 11:40
@kris914g
Copy link

Can we get this merged soon?

@kris914g
Copy link

Can we get this merged soon?

@mnapoli is there something we are waiting for before we can merge?

@omarkdev
Copy link

omarkdev commented Feb 25, 2022

@mnapoli It's possible to merge this one soon?

@mnapoli
Copy link
Member

mnapoli commented Feb 26, 2022

@omarkdev please calm down, this is becoming rude.

@mnapoli mnapoli merged commit 0abe1e2 into brefphp:master Feb 26, 2022
@mnapoli
Copy link
Member

mnapoli commented Feb 26, 2022

Thanks @hmazter!

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.

None yet

4 participants