Refactored structure.

[blaugueux]

Hi everyone,

Here is my proposal to fix #14, #12 and to improve this gem.

• Move all Symfony related commands into a symfony.rake file
• Move all deploy hook related stuff into a deploy.rake file
• Add a new symfony:assetic:dump command with configurable flags
• Add configurable flags to symfony:assets:install command
• Improve the doc
• Remove symfony:assets:install from the default hook structure. For projects using SensioDistributionBundle this task is already run during composer install.

Thanks for your comments :)

[blaugueux]

Maybe we can remove the symfony:build_bootstrap command because it's a part of SensioDistributionBundle and executed via composer on every install.

[bdossantos]

Small typo fix : "Execute"

[blaugueux]

Good catch thx 👍

[kingcrunch]

@blaugueux

Maybe we can remove the symfony:build_bootstrap command because it's a part of
SensioDistributionBundle and executed via composer on every install.

aaaand maybe somebody wants to remove the bundle 😄 Namely me. Because with a reasonable opcode cache (that doesn't perform a stat-call on every request to that file) the bootstrap file doesn't increase performance anymore and as such it's useless. And therefore the bundle is not used anymore.

Hadn't you said you think about dropping the console-related tasks in favor of custom tasks within the Capfile? At least thats what I understood.

[blaugueux]

@kingcrunch That was just a suggestion. You've right, I agree to let the symfony:build_bootstrap command as part of this gem.

Sure that's why i removed symfony:assets:install from the flow but kept it to avoid BC breaks.

Do you think more examples with symfony:command need to be written in the README ?

Sorry if my comments are not so clear but my english is really bad :(

[peterjmit]

@blaugueux @kingcrunch Hi guys, I'm just getting back from a London 🚲 Paris. I'll take a look at this when I am home this evening.

I am trying to get a VM up and running for testing (like the capistrano main project) to help us along with these MRs

[kingcrunch]

OK, I am a little bit into this again too. Because actually it is quite easy to integrate symfony commands into the workflow I think its better to not integrate assetic. It must be possible to disable it, which complicates things unnecessarily.

Oh, and please drop build_bootstrap 😄 Same reason actually. In most cases it is covered by the composers script handlers anyway, but depending on the setup it is simply not needed.

[blaugueux]

I agree to remove the build_boostrap:)

I've seen many people asking native Assetic support in this gem that's why i've add it. It can be easy to use for people using the Symfony standard edition with really simple project to deploy. No ?

[kingcrunch]

Nevermind

I got something wrong. I thought yesterday symfony:assetic:dump is already part of the flow.

[blaugueux]

@kingcrunch i've updated my code with your proposal and a small doc into the readme.

[peterjmit]

@blaugueux I am liking the look of all the changes here, nice work (deploy.rake makes a lot of sense)! I want to do a little bit of testing tonight (and watch the habs hopefully avoid elimination from the playoffs) and then I'll get it merged.

Thanks for the contributions!

[blaugueux]

@peterjmit That's a good news :)

Do you agree to remove the build_bootstrap command and to rename symfony:command to symfony:console ?

[kingcrunch]

@blaugueux Yeah, looks quite nice

Regarding symfony:console: I'd prefer that, but maybe it makes sense to also keep symfony:command with a nice looking "deprecated"-message printed out.

Regard build_bootstrap: If you refer my suggestions, than it may be related to the same stupid mistake I made with assetic:dump, that I thought it is part of the flow. As long as it is not (and I can safely remove the distribution-bundle) I don't care 😄 Maybe it makes sense to keep it, because creating oneself might be quite annoying, because of the untrivial pathname

[peterjmit]

@kingcrunch @blaugueux I feel like this bundle should aim to support the symfony standard edition in its default state (as that will satisfy the majority of users), beyond that accomodations/guides to supporting non-standard configurations can be given.

Accordingly I would like to keep build_bootstrap as part of the default flow for capistrano/symfony

[blaugueux]

@peterjmit @kingcrunch Ok so to finish this PR i've renamed symfony:command to symfony:console but kept the old one with a deprecated warning.

I think it's ready unless you want i made other updates :)

[kingcrunch]

@peterjmit Imo it is much easier to add it, but to remove it later. And there are reasons, why some don't want build_bootstrap in the flow

• Actually symfony-standard defines build_bootstrap as part of the composer script handlers
• build_bootstrap is not required at all.

Can you tell me, how to remove a single task from the flow again? (Imo introducing yet-another-configuration-option is not that clean...)

[blaugueux]

Totally agree with @kingcrunch. build_bootstrap must not be a part of the flow.

[peterjmit]

A task can be easily removed (thanks to Rake)!

Rake::Task['deploy:build_bootstrap'].clear

If the task is not included by default, then a standard symfony project cannot be deployed out of the box, which is something I think we should support

[blaugueux]

@peterjmit IMHO most of Symfony project are based on Standard Edition and the build_bootstrap command require that the DistributionBundle is installed.

I think it's better to add a command manually to the user configuration (with before or after) instead of removing it with your proposal.

[peterjmit]

@blaugueux

DistributionBundle (and bootstrap.php.cache) is part of the standard edition?

By removing it from the flow, you are placing the onus of configuration on users that probably know less about what they are doing.

1. If you know enough to have removed https://github.com/symfony/symfony-standard/blob/master/web/app.php#L6 and potentially removed the DistribtuionBundle from your installation of the standard edition, then removing deploy:build_bootstrap should be no problem.
2. Conversely for those less familiar with Symfony, and more pertinently less familiar with capistrano - you are asking them to understand why deploy:build_bootstrap must be included with their deploys and worse if they skip that part of the documentation, their deploy will not work (for reasons they may not understand).

Group 1 will understand why you are forcing them to add configuration. Group 2 may not understand why they have to add configuration

The expectation should be set with this library that if you have removed something/changed something from the standard edition, then you can expect to be adding/changing something in capistrano/symfony.

[kingcrunch]

DistributionBundle (and bootstrap.php.cache) is part of the standard edition?

... and it's performed with composer. So now you execute it twice.

If you know enough to have removed https://github.com/symfony/symfony
standard/blob/master/web/app.php#L6 and potentially removed the DistribtuionBundle from your
installation of the standard edition, then removing deploy:build_bootstrap should be no problem.

So you assume, that somebody, who knows enough PHP to remove a include from a PHP-file also knows enough about Ruby, Capistrano and Rake to add this line (where I must admit, that I've never seen it yet. Probably usefuly anyway 😄 )?

The expectation should be set with this library that if you have removed something/changed
something from the standard edition, then you can expect to be adding/changing something in
capistrano/symfony.

As pointed out above you already have to change something either in the standard-distribution, or the capistrano flow for a reasonable behaviour 😕

[blaugueux]

Case 1 : Power user that customizing symfony and don't want the SensioDistributionBundle
=> need to add symfony:build_bootstrapto their flow

Case 2 : Standard user using Symfony Standard Edition (most of our personas here)
=> don't need to do anything because the bootstrap will be build on each composer action.

[peterjmit]

I think we are all talking cross purposes here

... and it's performed with composer. So now you execute it twice.

For now composer is run with the --no-scripts flag - I think this point may be tripping this discussion up?

[blaugueux]

You can notice that capistrano/composer run install by default without the --no-scripts.

[kingcrunch]

For now composer is run with the --no-scripts flag - I think this point may be tripping
this discussion up?

Wrong 😄

Yeah, @blaugueux already said that, but I want to add, that this should have been considered harmful too. One may add additional scripts to the scripthandler and may expect, that they were executed. Even when they would overwrite the default to not include --no-scripts again, it would now execute the bootstrap-building twice.

For me it is like that: Actually keeping build_bootstrap in the flow is what differs from the expected default behaviour of symfony-standard and leads to confusion. I think it is ok to keep the possibility to add it on ones own, but that should be the default.

[peterjmit]

My only ask was that build_bootstrap happens at some point during a deploy by default - which it plainly is (as a side note https://github.com/capistrano/symfony/blob/master/lib/capistrano/symfony/defaults.rb#L40 should probably be removed as it is misleading - and confused me!).

I think we are all good here @blaugueux @kingcrunch

[blaugueux]

Ok @peterjmit I've remove this flags to have a clear situation.

And now its good to me :)

[kingcrunch]

Yep. 😄 But (not critical) couldn't you remove the composer flags completely? I mean, isn't that something, that should be covered by the composer gem instead?

[blaugueux]

@kingcrunch i was thinking about that few minutes ago! @peterjmit ok for you?

[peterjmit]

@blaugueux yea

should probably be removed as it is misleading

My intention was for it to be removed completely - but that wasn't clear!

[blaugueux]

@peterjmit #done 👍

[peterjmit]

One last thing (I have only just remembered) I originally started this library with a deploy.rake but had to move away from it (to symfony.rake and symfony_console.rake) because it appeared to conflict with https://github.com/capistrano/capistrano/blob/master/lib/capistrano/tasks/deploy.rake.

This is where my ruby knowledge lacks a bit, but I am assuming this has not been a problem here?

[blaugueux]

I run my deploy to a vagrant VM without issue with this code. So maybe your issue has been fixed.

[blaugueux]

@peterjmit awesome thx :)

[kingcrunch]

👍 🍰