Skip to content
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

[HttpKernel][FrameworkBundle] Add alternative convention for bundle directories #32845

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@yceruto
Copy link
Member

commented Aug 1, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32453
License MIT
Doc PR TODO

We already know that bundles must be compatible with many Symfony's versions, so it is very likely that current bundles won't be able to use this feature soon, unless they create symbolic links to support both structures.

The point is that this is already happening, so in the future when our bundles stop to support <=4.3 then you'll be sure to change the current directory structure.

We have recently added the getPublicDir() method in #31975, here I'm removing it in favor of hardcoding a new convention.

I've added some functional tests in which I've changed everything to this structure:

-- ModernBundle
   |-- config/ 
   |-- public/
   |-- src/
       |-- ModernBundle.php
   |-- templates/
   |-- translations/

WDYT?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

As I wrote in #32453 (comment), I'm not sure we want to make dirs configurable. Even getPublicDir is suspicious to me.

@linaori

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

I think making them configurable isn't necessarily what I want, it's the confusing structure it has right now I'd like to change. Making dirs configurable is one way to do this, because it means I don't have to follow the current structure and that it provides a migration path to change the default.

I very much prefer the structure as shown in the example. The fact that the non-code directory is called "Resources" (mind you, capital R) is bothering me since the start. the "views" directory isn't named correctly anymore either as it's called "templates" in Symfony.

Not sure if the proposed API is a good idea, but I do think that the bundle is responsible for the exposure of certain directories.

  • "Hey Symfony, you can find templates in this directory!"
  • "Hey, this directory contains entities, if you use doctrine, please register this"
  • etc.

Symfony telling where everything should be feels like something we could change, though it should have sane defaults. I don't agree with the current structure being a sane default anymore due to the changes in the Symfony structure; app/Resources vs bundle/Resources.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

what if we create a single entrypoint similar like Kernel, e.g. $bundle->locateResource(BundlePaths::CONFIG_DIR[, 'validator.yaml'])

with a built-in deprecation for views in favor of templates

for DI extensions we should also consider injecting the config dir somehow, or access to any path for that matter

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

As I wrote in #32453 (comment), I'm not sure we want to make dirs configurable. Even getPublicDir is suspicious to me.

I'm fine if it's not configurable, but we still want a new bundle directory structure, do you want it too?

with a built-in deprecation for views in favor of templates

@ro0NL we shouldn't deprecating yet -> ... bundles must be compatible with many Symfony's versions. at least until we're sure we can all migrate to the new structure without any compatibility issue.

for DI extensions we should also consider injecting the config dir somehow, or access to any path for that matter

I'm not sure it's necessary, you can hardcode it safely there, it's in userland.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

we still want a new bundle directory structure, do you want it too

yes! that'd look consistent with the new directory structure of apps

I'd just do it in FrameworkExtension only (and remove/deprecate getPublicDir)

@yceruto yceruto requested review from dunglas and xabbuh as code owners Aug 1, 2019

@yceruto yceruto force-pushed the yceruto:bundle_paths branch from 22aa290 to 58dd974 Aug 1, 2019

@yceruto yceruto changed the title [HttpKernel][FrameworkBundle] Add new method for bundle paths configuration [HttpKernel][FrameworkBundle] Add alternative convension for bundle directories Aug 1, 2019

@yceruto yceruto force-pushed the yceruto:bundle_paths branch from 890ab3f to 10bc0f3 Aug 1, 2019

@yceruto yceruto changed the title [HttpKernel][FrameworkBundle] Add alternative convension for bundle directories [HttpKernel][FrameworkBundle] Add alternative convention for bundle directories Aug 1, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Update

Removed the configuration of the bundle directory, it is now a new convension that we can optionally use as of 4.4 version.

Added getBundleDir method to the Bundle class, which is intended to define the root directory of the bundle. It may differ from the directory where the Bundle.php file is located (aka getPath()), e.g. when we have the new structure src/Bundle.php then ->getBundleDir() is a folder higher up.

The current convention is not being obsolete yet, it will (in the future).

Convention from 3rd-party bundles like Doctrine (/Resources/config/doctrine) must progressively support the new method in order to finally be able to deprecating/remove the old structure.

@nicolas-grekas
Copy link
Member

left a comment

I really like this approach :)
What's the purpose of getPath now? Should we deprecated it? Or should we plan to deprecate it in 5.1, to help bundles migrate smoothly?

src/Symfony/Component/HttpKernel/Bundle/Bundle.php Outdated Show resolved Hide resolved

@yceruto yceruto force-pushed the yceruto:bundle_paths branch from 10bc0f3 to 4c235db Aug 5, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

What's the purpose of getPath now? Should we deprecated it? Or should we plan to deprecate it in 5.1, to help bundles migrate smoothly?

I think that as of 5.2 (NOV 2020) where we should start to deprecating the legacy structure as well. That'll match with the end of support for 3.4.

@Taluu

Taluu approved these changes Aug 5, 2019

@yceruto yceruto force-pushed the yceruto:bundle_paths branch from 2009560 to 0b2ceae Aug 6, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Sorry for the rebasing after the approved revisions, I needed to update my PhpUnit bridge to make my local phpunit work properly.

Status: This Is Ready In My Side ;)

@yceruto yceruto force-pushed the yceruto:bundle_paths branch from b818f54 to 09a870a Aug 8, 2019

@yceruto yceruto removed the Deprecation label Aug 8, 2019

@nicolas-grekas
Copy link
Member

left a comment

I love it :)
We shouldn't miss documenting how to put the bundle class inside src/

@Tobion

Tobion approved these changes Aug 8, 2019

Copy link
Member

left a comment

Well done. Please add a changelog entry (maybe in HttpKernel where the Bundle is defined) explaining the new possible bundle structure. This can then be referred to from the UPGRADE files just mentioning that this is now possible but not required.

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I missed the last comment, let me know if it's enough.

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I'm also working on a demo bundle/repo to show people how to migrate safely:
https://github.com/yceruto/acme-bundle

@yceruto yceruto force-pushed the yceruto:bundle_paths branch from 23ff2ff to ebe80c9 Aug 13, 2019

UPGRADE-4.4.md Outdated Show resolved Hide resolved

@yceruto yceruto force-pushed the yceruto:bundle_paths branch 4 times, most recently from 5de3197 to 12c4c16 Aug 13, 2019

@nicolas-grekas nicolas-grekas force-pushed the yceruto:bundle_paths branch from 12c4c16 to 6996e1c Aug 13, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Thank you @yceruto.

@nicolas-grekas nicolas-grekas merged commit 6996e1c into symfony:4.4 Aug 13, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Aug 13, 2019

feature #32845 [HttpKernel][FrameworkBundle] Add alternative conventi…
…on for bundle directories (yceruto)

This PR was squashed before being merged into the 4.4 branch (closes #32845).

Discussion
----------

[HttpKernel][FrameworkBundle] Add alternative convention for bundle directories

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32453
| License       | MIT
| Doc PR        | TODO

We already know that bundles must be compatible with many Symfony's versions, so it is very likely that current bundles won't be able to use this feature soon, unless they create symbolic links to support both structures.

The point is that this is already happening, so in the future when our bundles stop to support <=4.3 then you'll be sure to change the current directory structure.

We have recently added the `getPublicDir()` method in #31975, here I'm removing it in favor of hardcoding a new convention.

I've added some functional tests in which I've changed everything to this structure:
```
-- ModernBundle
   |-- config/
   |-- public/
   |-- src/
       |-- ModernBundle.php
   |-- templates/
   |-- translations/
```
WDYT?

Commits
-------

6996e1c [HttpKernel][FrameworkBundle] Add alternative convention for bundle directories

@yceruto yceruto deleted the yceruto:bundle_paths branch Aug 13, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Yujuuu!! 🎉

sstok added a commit to park-manager/park-manager that referenced this pull request Aug 21, 2019

refactor #68 Architecture directory changes (sstok)
This PR was merged into the 1.0-dev branch.
labels: bc-break

Discussion
----------

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | 
| License       | MPL-v2.0

(See commits for details). This refactoring is for from complete, but as the main focus of this pr is to change the directory structure.

In short the explicit layering structure is removed, the module specific conventions are removed in favor of symfony/symfony#32845 

Commits
-------

2617018 [Core] Move Web UserInterface to Infrastructure
d07d2dc [Core] Remove explicit Infrastructure layer
9ce08d0 Move Core(Module) to a new directory structure
3b60cd4 [Core] Remove Module specific DI handling
40e34d7 [Core] Fix broken class EmailAddress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.