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

allow Symfony 4 #346

Merged
merged 2 commits into from Dec 8, 2017

Conversation

Projects
None yet
3 participants
@lsmith77
Member

lsmith77 commented Dec 1, 2017

No description provided.

@lsmith77 lsmith77 added the in review label Dec 1, 2017

@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

That's strange, it looks like Symfony 3.4.0 is broken on Travis CI.

Collaborator

alexislefebvre commented Dec 1, 2017

That's strange, it looks like Symfony 3.4.0 is broken on Travis CI.

- php: 7.2
env: SYMFONY_VERSION="3.4.*"
- php: 7.2
env: SYMFONY_VERSION="4.0.*"

This comment has been minimized.

@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

I'm not sure what we'll be able to support Symfony 4.0, maybe we should only support the LTS versions (2.7, 2.8 and 3.4) for the moment.

@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

I'm not sure what we'll be able to support Symfony 4.0, maybe we should only support the LTS versions (2.7, 2.8 and 3.4) for the moment.

This comment has been minimized.

@lsmith77

lsmith77 Dec 1, 2017

Member

I will play around with it a bit ..
That being said .. some of the DI based features of this Bundle likely need to change for Symfony 4 to really make sense ..

but if we don’t support Symfony 4 .. then all Bundles using our code here will have a problem.

@lsmith77

lsmith77 Dec 1, 2017

Member

I will play around with it a bit ..
That being said .. some of the DI based features of this Bundle likely need to change for Symfony 4 to really make sense ..

but if we don’t support Symfony 4 .. then all Bundles using our code here will have a problem.

This comment has been minimized.

@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

You're right. But maybe we can focus this PR on supporting only LTS versions (so that users can use Symfony 2.7, 2.8 or 3.4 today) and work on the Symfony 4.0 support in another PR.

@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

You're right. But maybe we can focus this PR on supporting only LTS versions (so that users can use Symfony 2.7, 2.8 or 3.4 today) and work on the Symfony 4.0 support in another PR.

@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

I can reproduce the error on my computer, here is the content of the file that cause the error:

<?php

use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;

// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
// Returns the public 'console.command_loader' shared service.

return $this->services['console.command_loader'] = new \Symfony\Component\Console\CommandLoader\ContainerCommandLoader(new \Symfony\Component\DependencyInjection\ServiceLocator(array('console.command.about' => function () {
    $f = function (\Symfony\Bundle\FrameworkBundle\Command\AboutCommand $v) { return $v; }; return $f(${($_ = isset($this->services['console.command.about']) ? $this->services['console.command.about'] : $this->load(__DIR__.'/getConsole_Command_AboutService.php')) && false ?: '_'});
}, …

It looks like return $this->services['console.command_loader'] = new … is causing the error.

Collaborator

alexislefebvre commented Dec 1, 2017

I can reproduce the error on my computer, here is the content of the file that cause the error:

<?php

use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;

// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
// Returns the public 'console.command_loader' shared service.

return $this->services['console.command_loader'] = new \Symfony\Component\Console\CommandLoader\ContainerCommandLoader(new \Symfony\Component\DependencyInjection\ServiceLocator(array('console.command.about' => function () {
    $f = function (\Symfony\Bundle\FrameworkBundle\Command\AboutCommand $v) { return $v; }; return $f(${($_ = isset($this->services['console.command.about']) ? $this->services['console.command.about'] : $this->load(__DIR__.'/getConsole_Command_AboutService.php')) && false ?: '_'});
}, …

It looks like return $this->services['console.command_loader'] = new … is causing the error.

@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

When i try to rerun only one test in order to identify the error, then the error disappear!

Example when launching tests with --filter:

./vendor/bin/phpunit --filter ConfigurationConfigTest
PHPUnit 6.5.1 by Sebastian Bergmann and contributors.

...........                                                       11 / 11 (100%)

Time: 1.33 seconds, Memory: 8.00MB

OK (11 tests, 33 assertions)
Collaborator

alexislefebvre commented Dec 1, 2017

When i try to rerun only one test in order to identify the error, then the error disappear!

Example when launching tests with --filter:

./vendor/bin/phpunit --filter ConfigurationConfigTest
PHPUnit 6.5.1 by Sebastian Bergmann and contributors.

...........                                                       11 / 11 (100%)

Time: 1.33 seconds, Memory: 8.00MB

OK (11 tests, 33 assertions)
@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 1, 2017

Collaborator

The fact that the error only appeared when all the test suite was run made me think that it was due to the fact that tests are launched in separate processes (due to different Kernel which load different configuration).


I think that I found a solution:

Replace this:

 * @runTestsInSeparateProcesses

With this:

 * @runTestsInSeparateProcesses
 * @preserveGlobalState disabled

See related documentation: https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.preserveGlobalState

Collaborator

alexislefebvre commented Dec 1, 2017

The fact that the error only appeared when all the test suite was run made me think that it was due to the fact that tests are launched in separate processes (due to different Kernel which load different configuration).


I think that I found a solution:

Replace this:

 * @runTestsInSeparateProcesses

With this:

 * @runTestsInSeparateProcesses
 * @preserveGlobalState disabled

See related documentation: https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.preserveGlobalState

@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 3, 2017

Collaborator

Using @preserveGlobalState disabled solve the PHP errors: alexislefebvre#52

But the testRunCommandVerbosity* tests are broken with Symfony 3.4. It looks like the way used to change verbosity does not work anymore.

Collaborator

alexislefebvre commented Dec 3, 2017

Using @preserveGlobalState disabled solve the PHP errors: alexislefebvre#52

But the testRunCommandVerbosity* tests are broken with Symfony 3.4. It looks like the way used to change verbosity does not work anymore.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Dec 4, 2017

Sorry to complicate things, but there is one issue you guys may not be aware of. In DoctrineFixturesBundle 3.0 (which is what people will need to start using in a bundleless setup), fixture classes are actually tagged services. This creates a problem with this bundle, as it tries to instantiate the fixtures. Instead, for DoctrineFixturesBundle 3.0, in a compiler pass, we call addFixtures() on the loader service, passing it all tagged services.

One solution might be to create your own service (in a compiler pass) that is passed all tagged repositories. This class would be a simple map of the repo objects: it could have a method like getFixture($fixtureClass) on it. Then, when the user passes the array of fixture classes, you would use that service to fetch off only the fixtures objects you want. Then, create the loader:

$loader = new SymfonyFixturesLoader($container);
$loader->addFixtures($fixtureObjects);

Hopefully the other fixtures libraries will follow this new pattern, and eventually it might simplify some logic in the library. If you have any questions, please let me know!

weaverryan commented Dec 4, 2017

Sorry to complicate things, but there is one issue you guys may not be aware of. In DoctrineFixturesBundle 3.0 (which is what people will need to start using in a bundleless setup), fixture classes are actually tagged services. This creates a problem with this bundle, as it tries to instantiate the fixtures. Instead, for DoctrineFixturesBundle 3.0, in a compiler pass, we call addFixtures() on the loader service, passing it all tagged services.

One solution might be to create your own service (in a compiler pass) that is passed all tagged repositories. This class would be a simple map of the repo objects: it could have a method like getFixture($fixtureClass) on it. Then, when the user passes the array of fixture classes, you would use that service to fetch off only the fixtures objects you want. Then, create the loader:

$loader = new SymfonyFixturesLoader($container);
$loader->addFixtures($fixtureObjects);

Hopefully the other fixtures libraries will follow this new pattern, and eventually it might simplify some logic in the library. If you have any questions, please let me know!

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Dec 4, 2017

Member

I am beginning more and more to accept that maybe its not feasible to bring this Bundle to v4 .. and maybe we should focus those efforts on making version 2 of this Bundle work with Symfony v3.4+

Member

lsmith77 commented Dec 4, 2017

I am beginning more and more to accept that maybe its not feasible to bring this Bundle to v4 .. and maybe we should focus those efforts on making version 2 of this Bundle work with Symfony v3.4+

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Dec 4, 2017

Indeed. As nice as this library is, it's loaded with a lot of bloat (well, at least the one giant class is very big). Of course, we need someone to make this effort :)

weaverryan commented Dec 4, 2017

Indeed. As nice as this library is, it's loaded with a lot of bloat (well, at least the one giant class is very big). Of course, we need someone to make this effort :)

@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 7, 2017

Collaborator

@lsmith77 Can I please take your work and work on a new PR focused on the Symfony 3.4 support?

Collaborator

alexislefebvre commented Dec 7, 2017

@lsmith77 Can I please take your work and work on a new PR focused on the Symfony 3.4 support?

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Dec 7, 2017

Member

sure!

Member

lsmith77 commented Dec 7, 2017

sure!

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Dec 7, 2017

Member

or alternatively we can close this PR in favor of completing #347

for Symfony 4 we focus on 2.0

Member

lsmith77 commented Dec 7, 2017

or alternatively we can close this PR in favor of completing #347

for Symfony 4 we focus on 2.0

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Dec 7, 2017

@lsmith77 What's the advantage of #347? Dropping 2.7 is a good idea, but it doesn't really help any end users, right? I mean, it seems like we should merge that... but yea... then we're still in a "let's do work fro 2.0 support sf 4.0".

weaverryan commented Dec 7, 2017

@lsmith77 What's the advantage of #347? Dropping 2.7 is a good idea, but it doesn't really help any end users, right? I mean, it seems like we should merge that... but yea... then we're still in a "let's do work fro 2.0 support sf 4.0".

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Dec 7, 2017

Member

#347 drops 2.7 but should cover adding support for 3.4

Member

lsmith77 commented Dec 7, 2017

#347 drops 2.7 but should cover adding support for 3.4

@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 7, 2017

Collaborator

I only want to add 3.4 to the test matrix for the moment, by using code from #347 and alexislefebvre#52. Once this Bundle will work with all the current LTS releases, we'll be able to think about the Symfony 4.0 support.

Collaborator

alexislefebvre commented Dec 7, 2017

I only want to add 3.4 to the test matrix for the moment, by using code from #347 and alexislefebvre#52. Once this Bundle will work with all the current LTS releases, we'll be able to think about the Symfony 4.0 support.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Dec 8, 2017

👍 let's do it! But not lose track of this!

weaverryan commented Dec 8, 2017

👍 let's do it! But not lose track of this!

@alexislefebvre alexislefebvre merged commit 07248b1 into master Dec 8, 2017

3 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/styleci/push The StyleCI analysis has passed
Details

@lsmith77 lsmith77 removed the in review label Dec 8, 2017

@lsmith77 lsmith77 deleted the symfony-4 branch Dec 8, 2017

@alexislefebvre

This comment has been minimized.

Show comment
Hide comment
@alexislefebvre

alexislefebvre Dec 8, 2017

Collaborator

Oops, I merged #351 that was based on this PR and it closed this PR too.

Collaborator

alexislefebvre commented Dec 8, 2017

Oops, I merged #351 that was based on this PR and it closed this PR too.

@alexislefebvre alexislefebvre added this to the 2.0 milestone Dec 8, 2017

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Dec 8, 2017

Member

no worries.

Member

lsmith77 commented Dec 8, 2017

no worries.

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Dec 8, 2017

Member

for the dependency issues I am following up here #352

Member

lsmith77 commented Dec 8, 2017

for the dependency issues I am following up here #352

@alexislefebvre alexislefebvre referenced this pull request Dec 8, 2017

Closed

Summary of Symfony 4.0 support #353

4 of 4 tasks complete

@alexislefebvre alexislefebvre modified the milestone: 2.0 Dec 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment