PHP-DI / Silex-Bridge

PHP-DI integration in Silex
http://php-di.org/doc/frameworks/silex.html
MIT License
24 stars 8 forks source link

Silex 2 #27

Closed kordos closed 6 years ago

kordos commented 6 years ago

Hi,

This changes allow to use silex2 with php di. Changes:

  1. ArgumentResolver class: silex 2 uses it for resolving controller arguments so I moved this code from ControlerResolver to new class.

  2. Proxy for request in Application - now php di is able to inject real Request object which Application uses internally. Example definition for request class: Request::class => \DI\get('request'),

In 1.x such DI definition allows to inject Request class used in Application and now also in 2.x:

mnapoli commented 6 years ago

The build is failing because of PHP versions it seems. Which PHP version is required by Silex 2?

We could bump the requirements to these versions. Feel free also to drop HVVM from Travis (I'm dropping support on all my projects).

mnapoli commented 6 years ago

One last comment :) : you should update composer.json to require Silex v2

kordos commented 6 years ago

There is already silex 2 in composer.json.

About PHP version: silex 2 requires php >=5.5.9 and I'm using php 7.1 Without composer.lock build will be probably working correctly. Please answer to my comment about it.

kordos commented 6 years ago

Now build is failing for php 5.5.x version which isn't compatible with silex 2. Silex to requires php in >=5.5.9. Other test is with HHVM. Can you remove failing steps from build which doesn't meet minimal requirements for php by silex(two with php 5.5 and one with HHVM)?

jdreesen commented 6 years ago

Now build is failing for php 5.5.x version which isn't compatible with silex 2. Silex to requires php in >=5.5.9.

Travis uses PHP 5.5.38 which Silex is compatible with, so that's not the problem. Albeit I'm not sure why Job #83.1 failed, though (the tests run successfully on my machine).

Other test is with HHVM. Can you remove failing steps from build which doesn't meet minimal requirements for php by silex(two with php 5.5 and one with HHVM)?

As @mnapoli said: feel free to drop HVVM from Travis. The PHP 5.5 tests should not be dropped because Silex supports these PHP versions, so we should too.

Job #83.5 failed because it installed the lowest possible dependencies that are allowed in the composer.json, which is Symfony 2.8, but this version does not support the ArgumentResolverInterface yet (seems it was added in 3.1). I think there is still a bit of work to do here, to support Symfony 2.8/3.0 and Symfony >= 3.1.

kordos commented 6 years ago

Travis uses PHP 5.5.38 which Silex is compatible with, so that's not the problem. Albeit I'm not sure why Job #83.1 failed, though (the tests run successfully on my machine).

Nope, minimal version for silex 2 is 5.5.9 which is a bit higher than 5.5.38

As @mnapoli said: feel free to drop HVVM from Travis. The PHP 5.5 tests should not be dropped because Silex supports these PHP versions, so we should too.

I'm not familiar with Travis. Can you do it?

jdreesen commented 6 years ago
Travis uses PHP 5.5.38 which Silex is compatible with, so that's not the problem. Albeit I'm not sure why Job #83.1 failed, though (the tests run successfully on my machine).

Nope, minimal version for silex 2 is 5.5.9 which is a bit higher than 5.5.38

The minimum PHP version for Silex is 5.5.9, so there should be no problem with PHP 5.5.38 because it is much newer ;)

As @mnapoli said: feel free to drop HVVM from Travis.
The PHP 5.5 tests should not be dropped because Silex supports these PHP versions, so we should too.

I'm not familiar with Travis. Can you do it?

Yes, I can do it (I'll try to find some time this evening).

kordos commented 6 years ago

The minimum PHP version for Silex is 5.5.9, so there should be no problem with PHP 5.5.38 because it is much newer ;)

Right, It should work.

jdreesen commented 6 years ago

I removed HHVM from the Travis test matrix and added PHP 7.1. I also required newer PHPUnit versions and changed Travis to use the local version instead of the global one (the global version somehow failed on PHP 5.5).

The last job with the lowest possible dependencies still fails, because we use interfaces that don't exist in Symfony < 3.1 at the moment (see comments above).

kordos commented 6 years ago

I removed HHVM from the Travis test matrix and added PHP 7.1. I also required newer PHPUnit versions and changed Travis to use the local version instead of the global one (the global version somehow failed on PHP 5.5).

great. thanks :D

The last job with the lowest possible dependencies still fails, because we use interfaces that don't exist in Symfony < 3.1 at the moment (see comments above).

Ok I will fix that

kordos commented 6 years ago

I add some changes for symfony 2.8.x and now it should work also with that version. Build also works

kordos commented 6 years ago

Hi. Builds are passing now and It works also with symfony 2.8+. Can you merge this pull request?

kordos commented 6 years ago

Hi, I resolved all your comments. Can you merge it?

kordos commented 6 years ago

@mnapoli, @jdreesen can you merge it?

mnapoli commented 6 years ago

:+1:, busy times lately :)