-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
3.x into Master #6780
3.x into Master #6780
Conversation
This fixes when the third argument of AdminMaker is a service name instead of a class. It retrieves the service name and replaces it with the class name.
* Fix template setters in AbstractAdmin Update tests/Templating/MutableTemplateRegistryTest.php Co-authored-by: Javier Spagnoletti <[email protected]> Update src/Templating/MutableTemplateRegistry.php Co-authored-by: Javier Spagnoletti <[email protected]> * Update src/Templating/MutableTemplateRegistry.php Co-authored-by: Javier Spagnoletti <[email protected]> Co-authored-by: Javier Spagnoletti <[email protected]>
* DevKit updates * Applied fixes from FlintCI (sonata-project#6788) * Fix lint Co-authored-by: Sullivan SENECHAL <[email protected]> Co-authored-by: Vincent Langlet <[email protected]>
@VincentLanglet can you also merge the most recent changes (with 5263515) again? Then I could continue to fix #6779 Or we do this separately? |
4520398
to
28f49a8
Compare
I made the merge and fixed the conflict, so you'll not need to do a PR on master ;) @sonata-project/contributors I skipped the failing tests, since there is an issue about it. |
@@ -27,7 +27,7 @@ jobs: | |||
- name: Install PHP with extensions | |||
uses: shivammathur/setup-php@v2 | |||
with: | |||
php-version: 7.4 | |||
php-version: 8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure about this? IMO we should stick to 7.4
or try to run it in both versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a merge, the change was not made here but in 3.x
https://github.com/sonata-project/SonataAdminBundle/blob/3.x/.github/workflows/qa.yaml#L30
It was automatically made by the dev-kit
https://github.com/sonata-project/dev-kit/blob/master/templates/project/.github/workflows/qa.yaml.twig#L32
which is by default the latest php version.
https://github.com/sonata-project/dev-kit/blob/3ce0e91eb8f5c0e70fa486e5e15e6703c4ea28bf/src/Domain/Value/Branch.php#L62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know, just pointing this out when I've seen it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only ran static analysis on the last supported php version.
If we start running on multiple version, we could have issue like
if (false === file_get_contents('foo')) {}
reported as useless in PHP8 but not in PHP7.
In this case, the right version might be the lowest one in a general way.
Do you want to open an issue or a PR on dev-kit ?
I'd like to avoid to delay this huge merge.
No description provided.