[DX] Re-work generate:bundle for simple app bundles #290#299
Conversation
|
Relates to issue #290 :) |
There was a problem hiding this comment.
the first line, for example, should be:
array(array('--dir' => $tmp, '--format' => 'annotation', '--shared=true'), "Foo/BarBundle\n", array('Foo\BarBundle', 'FooBarBundle', $tmp.'/', 'annotation', false)),I removed true from the third argument (which is not the input, but is used for asserting things) and passed --shared just as an option. This makes it not hang :)
There was a problem hiding this comment.
Add a $shared = false. I'm not sure if it's needed, but it will protect backwards compatibility
|
Not sure about this change. Maybe I'm creating application bundle but, I still will define custom services without need to manual load them config.yml. Maybe just omit configuration of bundle? |
|
@s7anley In that case, it's just up to you to create those classes. But for most devs who are building an app bundle, try should just import the yaml file if they want to define services in the bundle. In other words, you're use-case is legitimate, but we want to discourage the average dev from having the DI directory :) |
There was a problem hiding this comment.
I think we could expand a little bit this explanation (needs reword from a English native):
* ``--shared``: set this option if you are creating a bundle that will be shared
across several of your applications or if you are developing a third-party
bundle. Don't set this option if you are developing a bundle that will be
used solely in your application (e.g. ``AppBundle``).
|
@rafix @weaverryan I've tested this PR and here are some comments. When using the command non-interactively, everything works perfectly: # bundle without vendor
$ php app/console generate:bundle --namespace=AppBundle --dir=src --format=annotation --no-interaction
# bundles with vendor
$ php app/console generate:bundle --namespace=Acme\AppBundle --dir=src --format=annotation --no-interaction
$ php app/console generate:bundle --namespace=Acme\Bundle\AppBundle --dir=src --format=annotation --no-interactionWhen using it interactively, everything still works, but I think it's confusing: Welcome to the Symfony2 bundle generator
# the command starts with a very direct question ...
Are you planning on using/sharing this bundle across multiple applications [no]?
# ... but then I see the same boring comments about vendors
Your application code must be written in bundles. This command helps
you generate them easily.
Each bundle is hosted under a namespace (like Acme/Bundle/BlogBundle).
The namespace should begin with a "vendor" name like your company name, your
project name, or your client name, followed by one or more optional category
sub-namespaces, and it should end with the bundle name itself
(which must have Bundle as a suffix).
See http://symfony.com/doc/current/cookbook/bundles/best_practices.html#index-1 for more
details on bundle naming conventions.
Use / instead of \ for the namespace delimiter to avoid any problem.
Bundle namespace: AppBundle
...So basically, the question disables the vendor requirement but all the other things remain the same. Here it is my proposal: why don't we remove the question to the user about the shared/non-shared bundles and we just reword a bit the paragraphs before the |
|
@javiereguiluz Hmm, the issue is also that a few other changes will be made based on the question I propose:
A) Keep things like they are now, print this big long thing
Obviously, (B) is better, but we could do that in a different PR too :). |
|
I like your proposal Ryan. I agree that for now we should go for the option (A). Thanks. |
There was a problem hiding this comment.
no need to re-assign it to $shared here
|
you also need to avoid creating the |
|
I think @javiereguiluz already raised this question in one of the related issues, but let's discuss it here: is it really needed to have the |
|
@xabbuh thanks for bringing up this concern. The thing I'm not sure about is the following: for the already generated But since this tool is used to generate any bundle, one company can decide to use it to generate several "application bundles" without vendors ( That's why right now I'm thinking about removing |
|
@javiereguiluz I agree. It should be done based on the |
|
@javiereguiluz For a |
|
Wouldn't removing the In that case, should we rewrite those to use the |
|
@felds +1 for new app/Resources folder structure |
|
People keep posting bug reports because they cannot generate bundles without vendors (see the latest: symfony/symfony-docs#4470). Could we please treat this PR as a high priority PR and merge it as soon as possible? Thanks! |
|
AFAIU, this is not ready to be merged yet, right? |
|
ping @rafix |
…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
10 participants
@weaverryan please review my pull request ;)