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

Rendre compatible avec Symfony 6.0 #35

Merged

Conversation

klnjmm
Copy link
Contributor

@klnjmm klnjmm commented Mar 30, 2022

Environnement : Mac OS X
PHP Version : 8.1.3
Symfony version : 6.0.6
Version du bundle installée : 3.0.0

Lors de l'intégration du bundle avec Symfony 6.0, deux messages d'erreur remontaient lors du composer require / clear:cache :

  • Declaration of M6Web\Bundle\ElasticsearchBundle\M6WebElasticsearchBundle::getContainerExtension() must be compatible with Symfony\Component\HttpKernel\Bundle\Bundle::getContainerExtension(): ?Symfony\Component\DependencyInjection\Extension\ExtensionInterface

  • Declaration of M6Web\Bundle\ElasticsearchBundle\DependencyInjection\M6WebElasticsearchExtension::getAlias() must be compatible with Symfony\Component\DependencyInjection\Extension\Extension::getAlias(): string

@klnjmm klnjmm requested a review from a team as a code owner March 30, 2022 19:17
jdecool
jdecool previously approved these changes Mar 31, 2022
@Oliboy50
Copy link
Member

Oliboy50 commented Mar 31, 2022

ℹ️ we're still supporting symfony 4.4 (I hope these returned types are the same in 4.4 and 6.0 🤞)

Oliboy50
Oliboy50 previously approved these changes Mar 31, 2022
@Oliboy50 Oliboy50 dismissed their stale review March 31, 2022 10:43

CI does not pass

@jdecool jdecool dismissed their stale review March 31, 2022 11:24

CI is broken

@jdecool
Copy link

jdecool commented Mar 31, 2022

The Symfony\Component\DependencyInjection\Extension\ExtensionInterface interface already exists in the Symfony 4.4.

I think this PR should be OK if you fix the linter errors.

@klnjmm
Copy link
Contributor Author

klnjmm commented Mar 31, 2022

Je regarderais ce soir le linter et j'irais vérifier pour Symfony 4.4 au passage. (la CI ne s'était pas déclenchée hier quand j'ai fait la PR)

@klnjmm klnjmm force-pushed the feature/symfony-compatibility branch from 282e387 to abe1ed0 Compare March 31, 2022 19:04
@klnjmm
Copy link
Contributor Author

klnjmm commented Mar 31, 2022

@Oliboy50 @jdecool J'ai modifié le commit en corrigeant le linter.

Pour informations, cette modification est compatible SF4.4 : les classes Bundle et Extension existent en SF4.4 sauf que le retour des méthodes n'est pas typé :

Cela ne pose pas de problème de rajouter le typage dans une classe enfant : https://3v4l.org/F7QcB

Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

🙇

if you could also update our CI workflow to build against symfony 6.0, it would be perfect 👌

@klnjmm klnjmm force-pushed the feature/symfony-compatibility branch from abe1ed0 to 10f3634 Compare April 1, 2022 16:00
@klnjmm
Copy link
Contributor Author

klnjmm commented Apr 1, 2022

@Oliboy50 : voilà c'est mise à jour.

Je ne peux pas voir si c'est OK. Il faut approuver la modification de votre côté avant que le workflow ne soit déclenché ?

@Oliboy50
Copy link
Member

Oliboy50 commented Apr 1, 2022

@klnjmm it is normal for php 7.4 to fail with symfony 6.0 since it requires php 8.0+

so you will have to exclude this specific matrix combination:

      matrix:
        php-version: [ '7.4', '8.0', '8.1' ]
        symfony-version: ['^4.4', '^5.0', '^5.3']
        exclude:
          # excludes symfony 6.0 on php 7.4 
          - php-version: '7.4'
            symfony-version: '^6.0'

=> https://github.com/BedrockStreaming/ElasticsearchBundle/blob/master/.github/workflows/ci.yml#L9-L11

@Oliboy50
Copy link
Member

Oliboy50 commented Apr 1, 2022

about the php 8.0/8.1 + symfony 6.0 failed combinations, you will have to update the composer.json file to allow ^6.0 for symfony packages

=> https://github.com/BedrockStreaming/ElasticsearchBundle/blob/master/composer.json#L20-L25

@klnjmm
Copy link
Contributor Author

klnjmm commented Apr 1, 2022

Bon il va falloir que je regarde comment tester les github action en local … je regarde pour faire le correctif ce week-end.

@Oliboy50
Copy link
Member

Oliboy50 commented Apr 2, 2022

@klnjmm I updated the GitHub Actions triggering settings, now it should trigger the build of your PR each time you push something

Lors de l'intégration du bundle avec Symfony 6.0, deux messages d'erreur remontaient lors du clear:cache
* `Declaration of M6Web\Bundle\ElasticsearchBundle\M6WebElasticsearchBundle::getContainerExtension() must be compatible with Symfony\Component\HttpKernel\Bundle\Bundle::getContainerExtension(): ?Symfony\Component\DependencyInjection\Extension\ExtensionInterface`

* `Declaration of M6Web\Bundle\ElasticsearchBundle\DependencyInjection\M6WebElasticsearchExtension::getAlias() must be compatible with Symfony\Component\DependencyInjection\Extension\Extension::getAlias(): string`

Ces modifications sont compatibles avec Symfony 4.4 (les classes Bundle et Extension existent en SF4.4 sauf que le retour des méthodes n'est pas typé).
Cela ne pose pas de problème de rajouter le typage dans une classe enfant
@klnjmm klnjmm force-pushed the feature/symfony-compatibility branch from 10f3634 to ce7b2c5 Compare April 9, 2022 14:21
@klnjmm
Copy link
Contributor Author

klnjmm commented Apr 9, 2022

@Oliboy50 composer.json and GitHub workflow updated.
All checks are green !

@Oliboy50
Copy link
Member

@BedrockStreaming/cache-db I think you can merge/release this PR

@klnjmm
Copy link
Contributor Author

klnjmm commented Apr 27, 2022

Des nouvelles sur le tag de cette PR ?

@valentin-claras valentin-claras merged commit 76f4cd8 into BedrockStreaming:master Apr 28, 2022
@valentin-claras
Copy link
Contributor

Ahoy o/
Merged and released as 3.1.0, sorry for the delay 🙇

@klnjmm klnjmm deleted the feature/symfony-compatibility branch June 22, 2022 20:06
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