-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Refactoring/Cleanup/Fixes for Symfony-related Recipes #688
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,14 @@ | |
|
||
require_once __DIR__ . '/common.php'; | ||
|
||
|
||
/** | ||
* Symfony Configuration | ||
*/ | ||
|
||
// Symfony build env | ||
env('env', 'prod'); | ||
|
||
// Symfony shared dirs | ||
set('shared_dirs', ['app/logs']); | ||
|
||
|
@@ -22,17 +26,29 @@ | |
|
||
// Assets | ||
set('assets', ['web/css', 'web/images', 'web/js']); | ||
// Default true - BC for Symfony < 3.0 | ||
set('dump_assets', true); | ||
|
||
// Requires non symfony-core package `kriswallsmith/assetic` to be installed | ||
set('dump_assets', false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old code
Why do you change BC? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't assume asset compilation is desired b/c it isn't a core Symfony command and there is no reason to believe the user has it installed. It relies on https://github.com/symfony/assetic-bundle, which may (or may not) be installed. Previous behavior:
The above is inconsistent and incorrect behavior. New behavior:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The alternative to retain previous behavior and avoid a BC break is to define Also, while the above would avoid a BC break, it doesn't fix the bug for Symfony 2.8, which removed symfony/assetic-bundle from the default install and will break when including symfony.php during asset compilation. @oanhnn Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about remove anything about assets from symfony recipe? This will break BC, but we can put in in v4. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is what I would do, actually. I don't think it should call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, me neither. But a lot of people using it as it. Well be cool, to not break somethink for any guys :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The one thing to keep in mind is the current solution is broken for Symfony 2.8. This solution makes the recipes compatible with all versions with the small tweak of people having to decide if they want assetic run via the bool env switch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, may be fix it right in 3.x. Need to investigate a little. @oanhnn your thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @robfrawley I think you should split this problem to a PR, because it is break BC. We will discuss about it more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oanhnn Split what into a PR? There isn't really anything that can be separated from this PR. Can you elaborate? |
||
|
||
// Environment vars | ||
env('env_vars', 'SYMFONY_ENV=prod'); | ||
env('env', 'prod'); | ||
env('env_vars', 'SYMFONY_ENV={{env}}'); | ||
|
||
// Adding support for the Symfony3 directory structure | ||
set('bin_dir', 'app'); | ||
set('var_dir', 'app'); | ||
|
||
// Symfony console bin | ||
env('bin/console', function () { | ||
return sprintf('{{release_path}}/%s/console', trim(get('bin_dir'), '/')); | ||
}); | ||
|
||
// Symfony console opts | ||
env('console_options', function () { | ||
$options = '--no-interaction --env={{env}}'; | ||
|
||
return env('env') !== 'prod' ? $options : sprintf('%s --no-debug', $options); | ||
}); | ||
|
||
|
||
/** | ||
* Create cache dir | ||
|
@@ -60,48 +76,53 @@ | |
return "{{release_path}}/$asset"; | ||
}, get('assets'))); | ||
|
||
$time = date('Ymdhi.s'); | ||
|
||
run("find $assets -exec touch -t $time {} ';' &> /dev/null || true"); | ||
run(sprintf('find %s -exec touch -t %s {} \';\' &> /dev/null || true', $assets, date('Ymdhi.s'))); | ||
})->desc('Normalize asset timestamps'); | ||
|
||
|
||
/** | ||
* Install assets from public dir of bundles | ||
*/ | ||
task('deploy:assets:install', function () { | ||
run('{{env_vars}} {{bin/php}} {{bin/console}} assets:install {{console_options}} {{release_path}}/web'); | ||
})->desc('Install bundle assets'); | ||
|
||
|
||
/** | ||
* Dump all assets to the filesystem | ||
*/ | ||
task('deploy:assetic:dump', function () { | ||
if (!get('dump_assets')) { | ||
return; | ||
if (get('dump_assets')) { | ||
run('{{env_vars}} {{bin/php}} {{bin/console}} assetic:dump {{console_options}}'); | ||
} | ||
|
||
run('{{bin/php}} {{release_path}}/' . trim(get('bin_dir'), '/') . '/console assetic:dump --env={{env}} --no-debug'); | ||
})->desc('Dump assets'); | ||
|
||
|
||
/** | ||
* Warm up cache | ||
*/ | ||
task('deploy:cache:warmup', function () { | ||
run('{{bin/php}} {{release_path}}/' . trim(get('bin_dir'), '/') . '/console cache:warmup --env={{env}} --no-debug'); | ||
run('{{env_vars}} {{bin/php}} {{bin/console}} cache:warmup {{console_options}}'); | ||
})->desc('Warm up cache'); | ||
|
||
|
||
/** | ||
* Migrate database | ||
*/ | ||
task('database:migrate', function () { | ||
run('{{bin/php}} {{release_path}}/' . trim(get('bin_dir'), '/') . '/console doctrine:migrations:migrate --env={{env}} --no-debug --no-interaction'); | ||
run('{{env_vars}} {{bin/php}} {{bin/console}} doctrine:migrations:migrate {{console_options}}'); | ||
})->desc('Migrate database'); | ||
|
||
|
||
/** | ||
* Remove app_dev.php files | ||
*/ | ||
task('deploy:clear_controllers', function () { | ||
run("rm -f {{release_path}}/web/app_*.php"); | ||
run("rm -f {{release_path}}/web/config.php"); | ||
run('rm -f {{release_path}}/web/app_*.php'); | ||
run('rm -f {{release_path}}/web/config.php'); | ||
})->setPrivate(); | ||
|
||
// Run after code is checked out | ||
after('deploy:update_code', 'deploy:clear_controllers'); | ||
|
||
|
||
|
@@ -116,11 +137,13 @@ | |
'deploy:shared', | ||
'deploy:assets', | ||
'deploy:vendors', | ||
'deploy:assets:install', | ||
'deploy:assetic:dump', | ||
'deploy:cache:warmup', | ||
'deploy:writable', | ||
'deploy:symlink', | ||
'cleanup', | ||
])->desc('Deploy your project'); | ||
|
||
// Display success message on completion | ||
after('deploy', 'success'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Elfet Is this acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still allow users to use update. And this is ban practice. @oanhnn what you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crosses fingers... I understand your gut-reaction against running update ad-hoc, but it can be useful for automated deployments to dev server.
compoaser_options
already exists, so it's easy enough to overwrite the whole string, but this just pulls outcomposer_action
. Give me a heads up if this is okay @oanhnn and @Elfet .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Elfet I think it is ok. Not break BC and users can customize
composer_action
easily.I think in Deployer,
composer_options
will not includecomposer_action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oanhnn Is this what you mentioned breaking into a separate PR?