OpenConext / Stepup-saml-bundle

A PHP Symfony bundle that adds SAML capabilities to your application using simplesamlphp/saml2
Apache License 2.0
14 stars 24 forks source link

Add support for Symfony 4.3 #89

Closed epinxteren closed 4 years ago

epinxteren commented 5 years ago

https://www.pivotaltracker.com/story/show/167697164

jorissteyn commented 5 years ago

Hi Eric,

The MetadataFactory service depends on @templating - which is deprecated in symfony 4.3 and no longer loaded by default. In order for this bundle to work in symfony 4.3 you must first require symfony/templating and your framework config should read:

framework:
    templating:
        engines:
        - twig

Perhaps the README.md should mention this. In the future, to support symfony 5, the MetadataFactory should use a TwigEngine instead of a EngineInterface.

epinxteren commented 4 years ago

@jorissteyn Hi Joris, Added documentation. For now i have only done the most minimal adjustment to support sf4.

MKodde commented 4 years ago

@pablothedude and @epinxteren I've changed the composer.json slightly. Sharpening the Symfony requirements. Any feedback is welcome. If none is provided, I suggest to merge this and tag the 4.1.9 release!

MKodde commented 4 years ago

The only concern I have is that not all dependencies could be resolved. We're not able to detect this because the Symfony integration doesn't seem to have any test coverage. src/Metadata/MetadataFactory.php:58 uses \Symfony\Component\Templating\EngineInterface which couldn't be found.

Thanks for spotting this. Fixed this by requireing symfony/templating, I'm not sure if this could ever have worked in SF 3.4 installations. As I could not find any dependencies on the templating dependency in any of our dependencies. Only in the dev-dependencies of the framework, twig and security bundles..

I was wondering if some of the components needed to be retrieved with composer to support Symfony 4 but is seems that all components used are resolved with the Symfony framework bundle.

Thanks for checking this out :male_detective:

Personally I would be a an advocate to drop the Symfony 3 support completely and start a major 5 release.

Yes, we must certainly discuss this with SURFnet!

MKodde commented 4 years ago

@pablothedude after your review I've changed some things:

  1. Require Symfony templating
  2. Pin the 3.4 versions to known safe versions
  3. Update the README.md

Please review those changes and put your token of approval once satisfied.

Opened a story in our issue tracker to address the larger concerns in the (near) future https://www.pivotaltracker.com/story/show/169838423

pablothedude commented 4 years ago

@MKodde thanks for addressing the raised concerns and opening up a ticket to discuss the roadmap any further. https://www.pivotaltracker.com/n/projects/1163646/stories/169838423