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: Laravel 12.x Compatibility #63

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

laravel-shift
Copy link

This is an automated pull request from Shift to update your package code and dependencies to be compatible with Laravel 12.x.

Before merging, you need to:

  • Checkout the l12-compatibility branch
  • Review all comments for additional changes
  • Thoroughly test your package

If you do find an issue, please report it by commenting on this PR to help improve future automation.

@laravel-shift
Copy link
Author

⚠️ Shift detected GitHub Actions which run jobs using a version matrix. Shift attempted to update your configuration for Laravel 12. However, you should review these changes to ensure the desired combination of versions are built for your package.

@laravel-shift
Copy link
Author

⚗️ Using this package? If you would like to help test these changes or believe them to be compatible, you may update your project to reference this branch.

To do so, temporarily add Shift's fork to the repositories property of your composer.json:

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/laravel-shift/openfoodfacts-laravel.git"
        }
    ]
}

Then update your dependency constraint to reference this branch:

{
    "require": {
        "openfoodfacts/openfoodfacts-laravel": "dev-l12-compatibility",
    }
}

Finally, run: composer update

@epalmans epalmans changed the title Laravel 12.x Compatibility feat: Laravel 12.x Compatibility Feb 19, 2025
@denniseilander
Copy link

@teolemon Hi, thanks for your work on this package!

Do you have an estimated timeline for the Laravel 12 support release?

If you need any help on this, please let me know, I'd love to help.

Choose a reason for hiding this comment

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

PR Overview

This pull request updates the CI configuration to support Laravel 12.x compatibility and adjusts the dependency matrix accordingly.

  • Updated the Laravel version matrix to include 12.x and paired the corresponding testbench version.
  • Added an exclusion for Laravel 12.* with PHP 8.1 and adjusted the cli-args formatting in the code coverage upload step.

Reviewed Changes

File Description
.github/workflows/ci.yml Updated test matrix for Laravel compatibility and minor YAML formatting adjustments for code coverage upload

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

.github/workflows/ci.yml:15

  • Ensure that the test suite adequately covers the new Laravel 12.x configuration. Consider adding explicit test cases for Laravel 12.x to verify compatibility with the updated dependencies.
laravel: ['9.*', '10.*', '11.*', '12.*']

.github/workflows/ci.yml:80

  • [nitpick] Removing quotes from the cli-args value might lead to YAML parsing inconsistencies. Reintroduce quotes (e.g., "--format=php-clover build/coverage.clover") to ensure consistent string interpretation.
cli-args: --format=php-clover build/coverage.clover
@teolemon
Copy link
Member

teolemon commented Mar 2, 2025

@denniseilander it's all about reviewing the PR, ensuring it doesn't break everything.
I'm not a PHP expert, beyond teenage websites back in the day ;)
I've pinged @epalmans and @Benoit382

@Benoit382
Copy link
Collaborator

@denniseilander it's all about reviewing the PR, ensuring it doesn't break everything. I'm not a PHP expert, beyond teenage websites back in the day ;) I've pinged @epalmans and @Benoit382

We should fix CI error

@denniseilander
Copy link

denniseilander commented Mar 3, 2025

@Benoit382 I've forked the repository and tested these changes. It looks like the larastan/larastan dependency also needs to be updated to: "larastan/larastan": "^2.9|^3.1".

This resolves the dependency issues but introduces some PHPStan errors that need to be addressed.

@Benoit382 Benoit382 requested a review from a team as a code owner March 3, 2025 16:09
@Benoit382
Copy link
Collaborator

Benoit382 commented Mar 3, 2025

@Benoit382 I've forked the repository and tested these changes. It looks like the larastan/larastan dependency also needs to be updated to: "larastan/larastan": "^2.9|^3.1".

This resolves the dependency issues but introduces some PHPStan errors that need to be addressed.

I will test something

@Benoit382 Benoit382 force-pushed the l12-compatibility branch from 935ade2 to e323d49 Compare March 3, 2025 16:13
@denniseilander
Copy link

@Benoit382 The only unresolved issue is that PHPStan encounters problems in the Laravel 12 build, whereas earlier versions don’t have this issue. When fixing the missingType.generics PHPStan errors, everything works fine in the L12 build, but in older versions, it throws an error stating: Ignored error pattern was not matched in reported errors.

@Benoit382
Copy link
Collaborator

@Benoit382 The only unresolved issue is that PHPStan encounters problems in the Laravel 12 build, whereas earlier versions don’t have this issue. When fixing the missingType.generics PHPStan errors, everything works fine in the L12 build, but in older versions, it throws an error stating: Ignored error pattern was not matched in reported errors.

Work in progress 😉

@Benoit382 Benoit382 force-pushed the l12-compatibility branch from ff9dccd to 7ab0551 Compare March 3, 2025 17:11
Copy link

sonarqubecloud bot commented Mar 3, 2025

@Benoit382
Copy link
Collaborator

Benoit382 commented Mar 3, 2025

It was not so easy and we should think to remove Laravel 9.0 (EOL since 1 year https://endoflife.date/laravel )
What did you think @teolemon

--

I will add some comment tomorrow (if I have time)

@@ -5,10 +5,12 @@ includes:

parameters:
level: 9
reportUnmatchedIgnoredErrors: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be removed when phpstan 1.* is updated to 2.*

Comment on lines +18 to +22
<source>
<include>
<directory suffix=".php">src/</directory>
</include>
</source>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update of the configuration to phpunit (from 9 to 11)

@epalmans
Copy link
Member

epalmans commented Mar 4, 2025

It was not so easy and we should think to remove Laravel 9.0 (EOL since 1 year https://endoflife.date/laravel ) What did you think @teolemon

--

I will add some comment tomorrow (if I have time)

thanks for the fixes @Benoit382 ! Okay for me to merge, but I agree we should indeed consider dropping L9.x support. Might even be a good idea to do now, along with the 12.x support?

@teolemon
Copy link
Member

teolemon commented Mar 5, 2025

Yeah, you can go ahead @Benoit382 @epalmans , you are the experts there :-)

@Benoit382
Copy link
Collaborator

Benoit382 commented Mar 5, 2025

It was not so easy and we should think to remove Laravel 9.0 (EOL since 1 year https://endoflife.date/laravel ) What did you think @teolemon

--

I will add some comment tomorrow (if I have time)

thanks for the fixes @Benoit382 ! Okay for me to merge, but I agree we should indeed consider dropping L9.x support. Might even be a good idea to do now, along with the 12.x support?

I should be on new PR (One PR at a time)

@epalmans
Copy link
Member

epalmans commented Mar 5, 2025

agree. Though I suggest to not let CI tag a release just yet

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.

5 participants