8000 [HttpKernel] Fix Bundle name regression by ogizanagi · Pull Request #20944 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[HttpKernel] Fix Bundle name regression #20944

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

Merged
merged 1 commit into from
Dec 17, 2016
Merged

[HttpKernel] Fix Bundle name regression #20944

merged 1 commit into from
Dec 17, 2016

Conversation

ogizanagi
Copy link
Contributor
@ogizanagi ogizanagi commented Dec 15, 2016
Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20117
License MIT
Doc PR N/A

The bundle name can be set manually instead of being guessed from the class name, as the property is protected.
However, a regression prevents this name to be used, as calling Bundle::getNamespace() recomputes the bundle name from class instead.

The ability to name explicitly bundles is appreciable when dealing with "virtual" ones, or when providing bundles in a library under a Vendor\MyPackage\Bridge\Symfony\Bundle namespace. No need to rename the bundle class VendorMyPackageBundle which will make the instantiation in Kernel::registerBundle() quite ugly:

- new Vendor\MyPackage\Bridge\Symfony\Bundle\VendorMyPackageBundle()
+ new Vendor\MyPackage\Bridge\Symfony\Bundle\Bundle()

What about removing Bundle::parseClassName() and processing the namespace and bundle name separately, but keeping the namespace property introduced in #20117?

@ro0NL
Copy link
Contributor
ro0NL commented Dec 15, 2016

Good catch. I think it should still try to parse both at once, if possible. So 👍 for this approach.

@xabbuh
Copy link
Member
xabbuh commented Dec 16, 2016

Do we have a test which ensures that the name will be guessed properly if it isn't already set when getNamespace() is called?

@ogizanagi
Copy link
Contributor Author

@xabbuh : We don't. I've just added one, thanks (ideally those tests should be backported to lower branches I guess).


use Symfony\Component\HttpKernel\Bundle\Bundle as BaseBundle;

class Bundle extends BaseBundle
Copy link
Member

Choose a reason for hiding this comment

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

We could put this class and the GuessedNameBundle class into the BundleTest.php file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@xabbuh
Copy link
Member
xabbuh commented Dec 16, 2016

👍

1 similar comment
@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member
fabpot commented Dec 17, 2016

Thank you @ogizanagi.

@fabpot fabpot merged commit 3b5127d into symfony:master Dec 17, 2016
fabpot added a commit that referenced this pull request Dec 17, 2016
This PR was merged into the 3.3-dev branch.

Discussion
----------

[HttpKernel] Fix Bundle name regression

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

The bundle name can be set manually instead of being guessed from the class name, as the property is protected.
However, a regression prevents this name to be used, as calling `Bundle::getNamespace()` recomputes the bundle name from class instead.

The ability to name explicitly bundles is appreciable when dealing with "virtual" ones, or when providing bundles in a library under a `Vendor\MyPackage\Bridge\Symfony\Bundle` namespace. No need to rename the bundle class `VendorMyPackageBundle` which will make the instantiation in `Kernel::registerBundle()` quite ugly:

```diff
- new Vendor\MyPackage\Bridge\Symfony\Bundle\VendorMyPackageBundle()
+ new Vendor\MyPackage\Bridge\Symfony\Bundle\Bundle()
```

What about removing `Bundle::parseClassName()` and processing the namespace and bundle name separately, but keeping the `namespace` property introduced in #20117?

Commits
-------

3b5127d [HttpKernel] Fix Bundle name regression
@ogizanagi ogizanagi deleted the fix/3.2/http_kernel/bundle_name_regression branch December 17, 2016 08:56
@andrerom
Copy link
Contributor

@fabpot this seems to be missing from 3.2 branch.

@xabbuh
Copy link
Member
xabbuh commented Jan 27, 2017

@andrerom Good catch, see #21432.

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

Successfully merging this pull request may close these issues.

7 participants
0