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

Symfony 5 support #98

Merged
merged 8 commits into from
May 8, 2020
Merged

Symfony 5 support #98

merged 8 commits into from
May 8, 2020

Conversation

thesunrise1983
Copy link
Contributor

These changes are working on Symfony 5.0 and Symfony 4.4, I didn't test on lower versions.

@dmaicher
Copy link
Collaborator

@Seldaek
Copy link
Member

Seldaek commented Feb 18, 2020

Maybe it works if you just remove the type hint? I'm not sure if that'll be allowed by PHP.

@thesunrise1983
Copy link
Contributor Author

Seems this change is incompatible with Symfony 3.4:

https://travis-ci.org/nelmio/NelmioSolariumBundle/jobs/649368661?utm_medium=notification&utm_source=github_status

Fixed now by removing the type hint.
Another solution would be to end Symfony 3.4 support by creating a new version.

@thesunrise1983
Copy link
Contributor Author

@dmaicher All tests passed successfully.

Copy link
Collaborator

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

👍

composer.json Outdated Show resolved Hide resolved
@dmaicher
Copy link
Collaborator

dmaicher commented Apr 3, 2020

I would also suggest to adjust the travis build to test all 3 major Symfony versions explicitly.

This could be done after #99 is merged

@dmaicher
Copy link
Collaborator

dmaicher commented Apr 3, 2020

@thesunrise1983 could you add Symfony 5.0.* to the matrix here?

https://github.com/nelmio/NelmioSolariumBundle/blob/master/.travis.yml#L12

With PHP 7.4 should be enough I think 😊

@mhitza
Copy link

mhitza commented Apr 18, 2020

Can a maintainer make the last change mentioned and merge this PR? I need to fork the repository in order to use this bundle for now.

@thesunrise1983
Copy link
Contributor Author

@thesunrise1983 could you add Symfony 5.0.* to the matrix here?

https://github.com/nelmio/NelmioSolariumBundle/blob/master/.travis.yml#L12

With PHP 7.4 should be enough I think blush

done. I did my best ;-)

Copy link
Collaborator

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

👍

@dmaicher
Copy link
Collaborator

@Seldaek can we move forward with this? 😊

@dmaicher
Copy link
Collaborator

@thesunrise1983 @mhitza not sure if this will ever get released.

I decided to maintain my own bundle instead: https://packagist.org/packages/dama/solarium-bundle

Its fully compatible with Symfony 5 and requires the latest Solarium 5.2

@thePanz
Copy link
Collaborator

thePanz commented May 8, 2020

Sorry for the late responses @thesunrise1983 and @dmaicher .
I will merge this into master, thank you!

To @dmaicher I noticed that you introduced phpstan into your fork, would you like to contribute it here and merge again your fork?

I am willing to add you as co-maintainer of this bundle/repository, if @Seldaek agrees :)

@thePanz thePanz merged commit b449fb5 into nelmio:master May 8, 2020
@dmaicher
Copy link
Collaborator

dmaicher commented May 8, 2020

Sorry for the late responses @thesunrise1983 and @dmaicher .
I will merge this into master, thank you!

To @dmaicher I noticed that you introduced phpstan into your fork, would you like to contribute it here and merge again your fork?

I am willing to add you as co-maintainer of this bundle/repository, if @Seldaek agrees :)

@thePanz I would be happy to help maintaining this bundle instead 😉 You can also reach me on Symfony Slack if you want to discuss things.

@dmaicher dmaicher mentioned this pull request Oct 27, 2021
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