8000 [framework-bundle] [bug] the proposed `Kernel::registerBundles` implementation is not consistent · Issue #46 · symfony/recipes · GitHub
[go: up one dir, main page]
Skip to content

[framework-bundle] [bug] the proposed Kernel::registerBundles implementation is not consistent #46

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.

Sign up for GitHub

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

Jump to bottom
Closed
docteurklein opened this issue May 3, 2017 · 5 comments
Closed

[framework-bundle] [bug] the proposed Kernel::registerBundles implementation is not consistent #46

docteurklein opened this issue May 3, 2017 · 5 comments

Comments

@docteurklein
Copy link
docteurklein commented May 3, 2017 •
edited
Loading

yield ing bundle instances makes it incompatible with multiple iterations (Generators cannot be rewinded once iterated, generating this kind of exceptions:

bin/console config:dump framework
                                               
  [Exception]                                  
  Cannot traverse an already closed generator  
                                               

Exception trace:
 () at /home/florian/work/symfony/demo/vendor/symfony/framework-bundle/Command/AbstractConfigCommand.php:127
 Symfony\Bundle\FrameworkBundle\Command\AbstractConfigCommand->initializeBundles() at /home/florian/work/symfony/demo/vendor/symfony/framework-bundle/Command/AbstractConfigCommand.php:56
 Symfony\Bundle\FrameworkBundle\Command\AbstractConfigCommand->findExtension() at /home/florian/work/symfony/demo/vendor/symfony/framework-bundle/Command/ConfigDumpReferenceCommand.php:88
 Symfony\Bundle\FrameworkBundle\Command\ConfigDumpReferenceCommand->execute() at /home/florian/work/symfony/demo/vendor/symfony/console/Command/Command.php:264
 Symfony\Component\Console\Command\Command->run() at /home/florian/work/symfony/demo/vendor/symfony/console/Application.php:887
 Symfony\Component\Console\Application->doRunCommand() at /home/florian/work/symfony/demo/vendor/symfony/console/Application.php:223
 Symfony\Component\Console\Application->doRun() at /home/florian/work/symfony/demo/vendor/symfony/framework-bundle/Console/Application.php:80
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /home/florian/work/symfony/demo/vendor/symfony/console/Application.php:130
 Symfony\Component\Console\Application->run() at /home/florian/work/symfony/demo/bin/console:33

config:dump-reference [--format FORMAT] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] <command> [<name>] [<path>]

I propose either to return a typical array, or to use iterator_to_array in portions of code that needs multiple iterations, or refactor the way bundles are initialized.
@docteurklein docteurklein changed the title [framework-bundle] the proposed Kernel::registerBundles implementation is not consistent [framework-bundle] [bug] the proposed Kernel::registerBundles implementation is not consistent May 3, 2017
@sstok
Copy link
Contributor
sstok commented May 3, 2017

I wonder why this was even done, you are going to initialize all anyway 🤔

@nicolas-grekas You have worked with this before, can you explain why it is done here?

@sstok
Copy link
Contributor
sstok commented May 3, 2017

@docteurklein Thinking about this, why do load the bundles multiple times? Because it would initialize them multiple times then 😐 you can use getBundles() after initialization.

I guess we need to validate if the bundles are already initialized.

@docteurklein
Copy link
Author

the problem lies here:

https://github.com/symfony/framework-bundle/blob/343754d1d30351b4029675bf27747718a217df6d/Command/AbstractConfigCommand.php#L121-L127

Not sure yet why this is done like this

@stof
Copy link
Member
stof commented May 3, 2017

I think it should be calling getBundles instead

@docteurklein
Copy link
Author

indeed, based on what I see here: symfony/framework-bundle@cfcfae0 it used to call getBundles()

@ogizanagi ogizanagi mentioned this issue May 8, 2017
Merged
fabpot added a commit to symfony/symfony that referenced this issue May 11, 2017
@fabpot
bug #22669 [FrameworkBundle] AbstractConfigCommand: do not try regist…
0ad2f2e 
…ering bundles twice (ogizanagi)

This PR was merged into the 2.7 branch.

Discussion
----------

[FrameworkBundle] AbstractConfigCommand: do not try registering bundles twice

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/recipes#46
| License       | MIT
| Doc PR        | N/A

As spotted in symfony/recipes#46, there is no reason to call `registerBundles` on the kernel instance, as it's already booted. So we just have to use `getBundles` instead and `registerBundles` can be implemented in a non-rewindable way, as done with flex.

Commits
-------

040edfe [FrameworkBundle] AbstractConfigCommand: do not try registering bundles twice
@fabpot fabpot closed this as completed May 11, 2017
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue May 11, 2017
@fabpot
bug #22669 [FrameworkBundle] AbstractConfigCommand: do not try regist…
0701025 
…ering bundles twice (ogizanagi)

This PR was merged into the 2.7 branch.

Discussion
----------

[FrameworkBundle] AbstractConfigCommand: do not try registering bundles twice

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/recipes#46
| License       | MIT
| Doc PR        | N/A

As spotted in symfony/recipes#46, there is no reason to call `registerBundles` on the kernel instance, as it's already booted. So we just have to use `getBundles` instead and `registerBundles` can be implemented in a non-rewindable way, as done with flex.

Commits
-------

040edfec4a [FrameworkBundle] AbstractConfigCommand: do not try registering bundles twice
@rendler-denis rendler-denis mentioned this issue May 23, 2017
Closed
VolCh pushed a commit to VolCh/recipes that referenced this issue Nov 30, 2017
@symfony-bot
Merge pull request symfony#46 from 20uf/fos-elastica-bundle
22b836d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fabpot @docteurklein @stof @sstok
0