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: drop support of php 7.4 #376

Merged
merged 4 commits into from
May 17, 2023

Conversation

Chris53897
Copy link
Contributor

#374

Probably more could be changed. But i tried to not introduce any BC changes.

I am not sure how to handle the --prefer-lowest failing run (indirect deprecation notices from twig 1). https://github.com/Chris53897/KnpMenu/actions/runs/4971653642/jobs/8896326293

https://symfony.com/doc/current/components/phpunit_bridge.html#direct-and-indirect-deprecations

Copy link
Collaborator

@garak garak left a comment

Choose a reason for hiding this comment

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

Sorry I didn't notice this PR before proposing mine, see #378
I'm inclined to accept yours and close mine, but do you mind to take a look and take some changes from it?

@Chris53897
Copy link
Contributor Author

I merged your changes, except from the static data providers, as i made a seperate PR for that. #377
If you want me to merge it in this PR, just let me know.

I think the php 8.1 test-run should be on symfony 6.2 as 6.1 is not maintained anymore. https://symfony.com/releases/6.1
But i just took over your changes, so it is up to you to decide.

.github/workflows/build.yaml Show resolved Hide resolved
symfony: 5.4.*
- description: 'Symfony 6.0'
php: '8.0'
symfony: 6.0.*
- description: 'Dev deps'
- description: 'Symfony 6.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me, we should have jobs for a specific Symfony version only for LTS versions

Copy link
Contributor

Choose a reason for hiding this comment

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

… or at least supported branches only. Symfony 6.0 and 6.1 are dead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should test all the Symfony versions we declare support for

Copy link
Contributor

Choose a reason for hiding this comment

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

all the Symfony versions we declare support for

Which are?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the ones resolved in our composer.json: ^5.4 || ^6.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should we test each minor version of Symfony while not doing the same for other dependencies (Twig).
To me, we should be pragmatic here, having specific jobs only for LTS versions, which would avoid having to update the CI setup every 6 months.
The jobs running with unmodified dependencies already test with the latest stable Symfony version anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK then, let's go with the LTS versions only

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 hope i did the changes correct like suggested. Please have a look.

@garak garak mentioned this pull request May 15, 2023
src/Knp/Menu/Renderer/TwigRenderer.php Outdated Show resolved Hide resolved
src/Knp/Menu/Twig/Helper.php Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
symfony: 6.0.*
- description: 'Dev deps'
symfony: 5.4.*
- description: 'Symfony 5.4'
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need multiple jobs for Symfony 5.4

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 changed it, can you please have a look if it now ok?

@garak garak merged commit 923b18a into KnpLabs:master May 17, 2023
9 checks passed
@Chris53897 Chris53897 deleted the feature/drop-support-of-php7 branch May 17, 2023 09:37
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

5 participants