Update generate:bundle for non-shared bundles and more #312

weaverryan · 2014-11-19T03:04:01Z

Hi guys!

If you're reading this, please try this out and offer any feedback! There are so many different cases, it will make @fabpot's life much much easier (and that's always good).

This replaces (adds to) @rafix's work on #299. The task ended up being a bit larger, because there are so many things we could change, and we need to communicate things well.

Fortunately, the changes here alter the generation process very little - most changes are to how we ask the questions. Here are the highlights:

if a bundle is non-shared, we only ask for a bundle name, not a namespace
removed "generate full directory structure" functionality entirely. This added only a few extra files, which I don't think add value.
make .yml the service configuration format if you choose annotation format. If you're using annotation for routing, it's likely you're following the best practices are you might be surprised by XML :)
many small output changes

BC breaks:

BundleGenerator::generate is now gone
The structure option was removed
You now get services.yml if you choose annotation as your format

In #299, we asked the question "should a non-shared bundle have a Resources/ directory?". I think it should. Why? I think the AppBundle is special - it's the one bundle that's truly coupled to your kernel (e.g. app/ directory), which is why it's ok to store the templates in app/. If we also stored templates from other bundles in app/, it gets a crowded. If you had a DefaultController in 2 bundles, their templates would "want" to occupy the same space (e.g. app/Resources/views/default/*). So, I think it makes sense to have only one special bundle (per kernel) that you really couple to the kernel. IF you choose to add other bundles (which you don't need to do), I think you should organize them more along the lines of traditional bundle, at least for templates.

TODOS

Import services.xml/yml manually from config.yml for non-shared bundle
Can we break BC in the ways described above? Or do I need to re-add somethings?
Fix the GenerateBundleCommandTest
Add more cases to BundleGeneratorTest for no DI directory
Fix BundleGeneratorTest

What does everyone think? /cc @javiereguiluz

Thanks!

weaverryan · 2014-11-19T03:13:09Z

Ah, and screenshots of different scenarios:

javiereguiluz · 2014-11-19T08:06:23Z

@weaverryan thanks for your impressive work! Before reviewing the code of your commits, I have two questions:

Does the Summary Before Generation really add value to this command? Have you ever typed in n to avoid generating the bundle?
Could we get rid of this sentence? Use / instead of \ for the namespace delimiter to avoid any problem. I really hate it because it's distracting and this is something that a computer could take care of (str_replace could do the trick).

javiereguiluz · 2014-11-19T08:07:09Z

Command/GenerateBundleCommand.php

@@ -39,7 +39,7 @@ protected function configure()
                new InputOption('dir', '', InputOption::VALUE_REQUIRED, 'The directory where to create the bundle'),
                new InputOption('bundle-name', '', InputOption::VALUE_REQUIRED, 'The optional bundle name'),
                new InputOption('format', '', InputOption::VALUE_REQUIRED, 'Use the format for configuration files (php, xml, yml, or annotation)'),
-                new InputOption('structure', '', InputOption::VALUE_NONE, 'Whether to generate the whole directory structure'),
+                new InputOption('shared', '', InputOption::VALUE_NONE, 'Are you planning on using/sharing this bundle across multiple applications?'),


Could we simplify the question as follows?

Are you planning on sharing this bundle across multiple applications?

javiereguiluz · 2014-11-19T11:08:37Z

@weaverryan I've tested this command with all kinds of option combinations and it always worked great for me.

weaverryan · 2014-11-19T16:08:17Z

@javiereguiluz One last thing. Do you agree that for non-shared bundles that we should continue generating a services.*ml file? And if so, since we don't generate the DependencyInjection directory, should we add an import to config.yml? This is the gray area we were talking about, where people might choose to create more than one coupled-application bundles (I don't see the point, but a valid thing to do).

Thanks!

javiereguiluz · 2014-11-19T16:18:40Z

@weaverryan I agree with importing the services.*ml file directly from app/config/config.yml for this kind of non-shared bundles.

samsonasik · 2014-11-25T20:59:46Z

Tests/Generator/BundleGeneratorTest.php

@@ -125,7 +125,7 @@ public function testExistingEmptyDirIsFine()
    {
        $this->filesystem->mkdir($this->tmpDir.'/Foo/BarBundle');

-        $this->getGenerator()->generate('Foo\BarBundle', 'FooBarBundle', $this->tmpDir, 'yml', false);
+        $this->getGenerator()->generate('Foo\BarBundle', 'FooBarBundle', $this->tmpDir, 'yml', true);


generateBundle

weaverryan · 2014-11-25T22:35:59Z

This is now ready for review! As I mentioned in #315, I think we should merge that in the short-term, then put this into a new version. I'd also like to update several of the other commands for that new version.

The last change - other than a new Bundle class to clean some things up - was to import the services.xml/yml file from config.yml if there is no DI directory. Everything should be good now. There are a lot of nice tests, but we'll still need some manual testers :).

Thanks!

…ndor namespace (weaverryan) This PR was merged into the 2.4.x-dev branch. Discussion ---------- generate:bundle Allowing a namespace to be used without a vendor namespace Hey guys! This is a quick, BC version of #312, which has become a major update to the entire command. That's great, but it will break some BC and will need to be released as a new version. But right now, it's still impossible to generate a bundle without a vendor namespace. That was the original issue, and the fix is very simple. This fixes that in a BC way. If you *do* omit the vendor namespace, it even gives you a warning (in case someone accidentally says `Acme/FooBundle`, which looks like `AcmeFooBundle` to the system). I've tested both sides of the flow and it works fine. If someone else can double-check, that'll make Fabien's merge much easier. Screenshots Below! Thanks! ![screen shot 2014-11-25 at 5 16 28 pm](https://cloud.githubusercontent.com/assets/121003/5192677/59c114c2-74c7-11e4-9f0e-d4a36366598c.png) ![screen shot 2014-11-25 at 5 16 51 pm](https://cloud.githubusercontent.com/assets/121003/5192676/59c0e0e2-74c7-11e4-9e77-8ade8dc48ac1.png) Commits ------- 6fe3188 Allowing a namespace to be used without a vendor namespace

fabpot · 2014-12-04T05:47:25Z

Sorry about that, but this PR should be rebased as #321 removed the usage of the deprecated helpers.

wouterj · 2015-02-05T19:51:03Z

ping @weaverryan

fabpot · 2015-03-17T06:37:52Z

@weaverryan Can you rebase this PR?

…fig.yml

…en if they're passed to the command line (it simplifies the code a lot)

…g twice

…g situation

…n messaging)

weaverryan · 2015-03-23T01:37:51Z

This is ready to go again! And I have tested all the different paths I can find. Highlights:

Added the shared option
Removed the structure option
The task now ALWAYS asks questions (even if the values are passed as args) unless you explicitly say --no-interactive (this drastically simplified the code)
For non-shared bundles, you don't get a DependencyInjection directory, and your services.yml file is imported directly from config.yml.
The signature to BundleGenerator::generateBundle() changed - there is now a nice Bundle class instead of passing around all the config for a bundle.

@javiereguiluz if you have a few minutes to try this, it would probably help Fabien have the confidence to merge (since the diff is too big to really study).

Thanks!

…s --no-interactive

fabpot · 2015-03-23T07:23:44Z

As there are some "BC breaks", I've bumped the version to 3.0: 176ea17

fabpot · 2015-03-23T07:24:11Z

... which makes merging easier :)

fabpot · 2015-03-23T07:24:44Z

Thanks @weaverryan for working on this feature, this is much appreciated.

@fabpot

…rafix, weaverryan) This PR was merged into the 3.0.x-dev branch. Discussion ---------- Update generate:bundle for non-shared bundles and more Hi guys! If you're reading this, please try this out and offer any feedback! There are so many different cases, it will make @fabpot's life much much easier (and that's always good). This replaces (adds to) @rafix's work on #299. The task ended up being a bit larger, because there are so many things we *could* change, and we need to communicate things well. Fortunately, the changes here alter the generation process very little - most changes are to *how* we ask the questions. Here are the highlights: * if a bundle is non-shared, we only ask for a bundle name, not a namespace * removed "generate full directory structure" functionality entirely. This added only a few extra files, which I don't think add value. * make `.yml` the service configuration format if you choose annotation format. If you're using annotation for routing, it's likely you're following the best practices are you might be surprised by XML :) * many small output changes BC breaks: * `BundleGenerator::generate` is now gone * The `structure` option was removed * You now get `services.yml` if you choose annotation as your format In #299, we asked the question "should a non-shared bundle have a `Resources/` directory?". I think it should. Why? I think the `AppBundle` is special - it's the one bundle that's truly coupled to your kernel (e.g. `app/` directory), which is why it's ok to store the templates in `app/`. If we also stored templates from other bundles in `app/`, it gets a crowded. If you had a `DefaultController` in 2 bundles, their templates would "want" to occupy the same space (e.g. `app/Resources/views/default/*`). So, I think it makes sense to have only one special bundle (per kernel) that you really *couple* to the kernel. *IF* you choose to add other bundles (which you don't need to do), I think you should organize them more along the lines of traditional bundle, at least for templates. ### TODOS * [x] Import services.xml/yml manually from config.yml for non-shared bundle * [x] Can we break BC in the ways described above? Or do I need to re-add somethings? * [x] Fix the GenerateBundleCommandTest * [x] Add more cases to BundleGeneratorTest for no DI directory * [x] Fix BundleGeneratorTest What does everyone think? /cc @javiereguiluz Thanks! Commits ------- e632cb8 Removing docs for now-gone option 712235e updating the test because we now ALWAYS ask questions, unless you pass --no-interactive 483ab4b Coding standards 1dd31e3 Removing an extra / in the target directory (was ok, but looked bad in messaging) 8ee423b Fixing a bug where we required the vendor namespace in the exact wrong situation a802785 Displaying path to AppKernel in messaging 2673bb4 Fixing wrong separator for namespace 079250f Not asking for the bundle if you're in non-shared mode - it was asking twice ae3db97 Fixing default value to be dependent on the "shared" value baab95e Cleaning some things up and changing the behavior to ask questions even if they're passed to the command line (it simplifies the code a lot) c5785e4 Trying to fix my rebase 89d3ee6 Fixing coding standards 5ae77db Fixing another test now that the Bundle object is being passed c66ed09 Fixing bad indentation (caught by tests!) de0c682 Fixing test by using new Bundle object 8a768ac Fixing bad base class for test cb4b1b9 Tweaking language based on comment from @samsonasik 7a41f5c Adding the ability to import a configuration file from app/config/config.yml 7a6033b Adding a Bundle model object to help centralize its metadata 48ced20 Removing an old, unused method 955fae6 Fixing code standards 5fa687a Fixing a number of tests! e7645da Improving error message on the validator - just a bit more helpful when I'm doing tests 5537e9c Revert "fixing a bug where we'd ask shared even if it were passed as false" e935050 fixing a bug where we'd ask shared even if it were passed as false e388b40 Fixing bug that caused paths to be missing a slash a55f65f Adding line breaks, but no changes bfd1916 Fixing coding standards f0d0b95 Tweaks thanks to @javiereguiluz ad28da8 Major overhaul of the interaction part of the generate:bundle command 42c32d4 Very minor code tweaks (no functional stuff) 4af1618 CS fix 3d65d3c better explanation for --shared option bc058c7 = false in generate method :) 1b5f7fb [WIP] optional vendor's name 8e4e023 shared option set to false by default 5f3233b droped shared element from parameters array in Generator/BundleGenerator.php 5f4d06c docs fixed 763e2f1 more on test 1d4600c [WIP] more on test f541d16 fix test errors in Tests/Command/GenerateBundleCommandTest.php ac39c7a patch applied 778caf2 [DX] Re-work generate:bundle for simple app bundles #290

fabpot · 2015-03-23T07:25:28Z

It's in now, but if @javiereguiluz would test it thoroughly, that would be tremendously helpful (we can even tweak things as we are working on a version nobody is going to use before we decide to tag 3.0.0).

weaverryan mentioned this pull request Nov 19, 2014

[DX] Re-work generate:bundle for simple app bundles #290 #299

Closed

javiereguiluz reviewed Nov 19, 2014
View reviewed changes

weaverryan force-pushed the shared branch from 80d51a3 to fa497e2 Compare November 25, 2014 01:03

samsonasik reviewed Nov 25, 2014
View reviewed changes

weaverryan mentioned this pull request Nov 25, 2014

generate:bundle Allowing a namespace to be used without a vendor namespace #315

Merged

javiereguiluz mentioned this pull request Dec 29, 2014

Clarify tip for creating a new AppBundle symfony/symfony-docs#4702

Merged

rafix added 5 commits March 22, 2015 20:04

[DX] Re-work generate:bundle for simple app bundles #290

778caf2

patch applied

ac39c7a

fix test errors in Tests/Command/GenerateBundleCommandTest.php

f541d16

[WIP] more on test

1d4600c

more on test

763e2f1

weaverryan added 11 commits March 22, 2015 20:08

Fixing code standards

955fae6

Removing an old, unused method

48ced20

Adding a Bundle model object to help centralize its metadata

7a6033b

Adding the ability to import a configuration file from app/config/con…

7a41f5c

…fig.yml

Tweaking language based on comment from @samsonasik

cb4b1b9

Fixing bad base class for test

8a768ac

Fixing test by using new Bundle object

de0c682

Fixing bad indentation (caught by tests!)

c66ed09

Fixing another test now that the Bundle object is being passed

5ae77db

Fixing coding standards

89d3ee6

Trying to fix my rebase

c5785e4

weaverryan force-pushed the shared branch from ca38901 to c5785e4 Compare March 23, 2015 00:25

weaverryan added 8 commits March 22, 2015 21:00

Cleaning some things up and changing the behavior to ask questions ev…

baab95e

…en if they're passed to the command line (it simplifies the code a lot)

Fixing default value to be dependent on the "shared" value

ae3db97

Not asking for the bundle if you're in non-shared mode - it was askin…

079250f

…g twice

Fixing wrong separator for namespace

2673bb4

Displaying path to AppKernel in messaging

a802785

Fixing a bug where we required the vendor namespace in the exact wron…

8ee423b

…g situation

Removing an extra / in the target directory (was ok, but looked bad i…

1dd31e3

…n messaging)

Coding standards

483ab4b

weaverryan added 2 commits March 22, 2015 21:48

updating the test because we now ALWAYS ask questions, unless you pas…

712235e

…s --no-interactive

Removing docs for now-gone option

e632cb8

fabpot merged commit e632cb8 into sensiolabs:master Mar 23, 2015

weaverryan deleted the shared branch March 23, 2015 14:42

Update generate:bundle for non-shared bundles and more #312

Update generate:bundle for non-shared bundles and more #312

Uh oh!

Conversation

weaverryan commented Nov 19, 2014

TODOS

Uh oh!

weaverryan commented Nov 19, 2014

Uh oh!

javiereguiluz commented Nov 19, 2014

Uh oh!

javiereguiluz Nov 19, 2014

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Nov 19, 2014

Uh oh!

weaverryan commented Nov 19, 2014

Uh oh!

javiereguiluz commented Nov 19, 2014

Uh oh!

samsonasik Nov 25, 2014

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Nov 25, 2014

Uh oh!

fabpot commented Dec 4, 2014

Uh oh!

wouterj commented Feb 5, 2015

Uh oh!

fabpot commented Mar 17, 2015

Uh oh!

weaverryan commented Mar 23, 2015

Uh oh!

fabpot commented Mar 23, 2015

Uh oh!

fabpot commented Mar 23, 2015

Uh oh!

fabpot commented Mar 23, 2015

Uh oh!

fabpot commented Mar 23, 2015

Uh oh!

Uh oh!