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

Symfony flex support #1440

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zorn-v
Copy link

zorn-v commented Dec 8, 2017

Q A
Bug fix? No
New feature? Yes
BC breaks? No
Deprecations? No
Fixed tickets #1437

@zorn-v zorn-v force-pushed the zorn-v:symfony-flex-compat branch from 6df4be4 to 6be861c Dec 8, 2017

@zorn-v zorn-v force-pushed the zorn-v:symfony-flex-compat branch from 6be861c to 976de9a Dec 8, 2017

set('clear_paths', []);
// Symfony shared dirs
set('shared_dirs', ['var/log', 'var/sessions']);

This comment has been minimized.

@Cryde

This comment has been minimized.

@zorn-v

zorn-v Jan 5, 2018

Also nope. var/log, not var/logs

This comment has been minimized.

@LouTerrailloune

LouTerrailloune Jan 10, 2018

The default directory for logs is var/log (since 3.4)

set('shared_dirs', ['var/log', 'var/sessions']);
// Symfony writable dirs
set('writable_dirs', ['var/cache', 'var/log', 'var/sessions']);

This comment has been minimized.

@Cryde

This comment has been minimized.

@zorn-v

zorn-v Jan 5, 2018

Also var/log

// Environment vars
set('env', function () {
return [
'APP_ENV' => get('symfony_env')

This comment has been minimized.

@Cryde

Cryde Jan 4, 2018

We should probably add documentation to tell the user to put "symfony_env" in it's host.yml

This comment has been minimized.

@zorn-v

This comment has been minimized.

@gander

gander Jan 9, 2018

Contributor

Maybe:

// Environment vars
set('env', function () {
    return [
        'SYMFONY_ENV' => get('symfony_env'),
        'APP_ENV'     => get('symfony_env'),
    ];
});

This comment has been minimized.

@gander

gander Jan 12, 2018

Contributor

I can not set environment variables on my server so Dotenv is also included in the production environment. There were complications. I noticed that setting APP_ENV disables reading the .env file, therefore I set only symfony_env to have all commands with --env={{symfony_env}} option. My env for now:

// Environment vars
set('env', []);

This comment has been minimized.

@zorn-v

zorn-v Jan 12, 2018

Hmm. Maybe you are right and your solution is much better than my hacky workaround #1440 (comment)
AFAIR composer does not account --env=
Will test, and change it if all works fine.

*/
//No need to clear anything
set('clear_paths', []);

This comment has been minimized.

@Cryde

Cryde Jan 4, 2018

It's already define in symfony3 recipe :)

This comment has been minimized.

This comment has been minimized.

@Cryde

Cryde Jan 5, 2018

My bad ! Sorry !

set('writable_dirs', ['var/cache', 'var/log', 'var/sessions']);
//File for DotEnv if not using env vars
set('shared_files', ['.env']);

This comment has been minimized.

@Cryde

Cryde Jan 4, 2018

And btw the dotenv component is in require-dev when we do a fresh install ... I'm not sure if it's a good idea

This comment has been minimized.

@antonmedv

antonmedv Jan 4, 2018

Member

Actually there is a conversation on moving dotenv to require symfony/symfony#25643

@Cryde

This comment has been minimized.

Copy link

Cryde commented Jan 4, 2018

Another tought : what about commands launched that require DB access (in prod mode) ? 🤔
Like migration for instance

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Jan 5, 2018

And now think about is it ENV vars really good for deploy ?
But you can set env from some local file that not under version control anyway... And set that vars on remote somehow ;)

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Jan 5, 2018

@zorn-v no, then don't. 😄

I really want to combine all symfony recipes into one in next v7. Let's merge it now for v6 although.

Can you add init script for symfony flex?

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Jan 5, 2018

What do you mean by 'init script' ?

@antonmedv

This comment has been minimized.

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Jan 5, 2018

Yes, I already understand what do you mean 😄
How do you think, do it with accounting of DotEnv in prod ?

"Best practice" (env vars) not so conveniently... You need to set params in 3 different places.

  1. In deployer
  2. For console commands on remote
  3. For web server on remote
@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Jan 5, 2018

Let's leave it for user? 😁

Trim every thing related of setting params from recipe and add comment to init template about for to start using .env, if user want of cause :))

I like .env file.

@zorn-v zorn-v force-pushed the zorn-v:symfony-flex-compat branch from 2795f09 to 06b888a Jan 5, 2018

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Jan 5, 2018

What do you think ?
Yes there need some workaround for composer install (coz scripts in composer.json)

before('deploy:symlink', 'database:migrate');
//If you don't use DotEnv in prod just wipe it out
task('clear:env', function () {

This comment has been minimized.

@antonmedv

antonmedv Jan 5, 2018

Member

What purpose of this task?

This comment has been minimized.

@zorn-v

zorn-v Jan 5, 2018

DotEnv does not run at all if APP_ENV is set https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/3.3/public/index.php#L11
Same in console commands
But it need for composer install (scripts can not load deps in require-dev if APP_ENV not 'prod')

This comment has been minimized.

@antonmedv

antonmedv Jan 5, 2018

Member

I think it's better not to hack APP_ENV but unstead write comment what user need to move dotenv to require section.

This comment has been minimized.

@zorn-v

zorn-v Jan 6, 2018

But this task needed exactly in order if DotEnv in require ;)
One more time - DotEnv does not run if APP_ENV is set.
If you don't set it in env, composer install will fail.

Yes I know that is hack. But I think that I can not convince community to change !isset($_SERVER['APP_ENV']) to file_exists(__DIR__.'/../.env')

set('web_dir', 'public');
// Assets
set('assets', ['public/css', 'public/images', 'public/js']);

This comment has been minimized.

@gander

gander Jan 9, 2018

Contributor
set('assets', ['{{web_dir}}/css', '{{web_dir}}/images', '{{web_dir}}/js']);
@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Jan 15, 2018

Okay, problem now with:

if (!isset($_SERVER['APP_ENV'])) {
    if (!class_exists(Dotenv::class)) {
        throw new \RuntimeException('APP_ENV environment variable is not defined. You need to define environment variables for configuration or add "symfony/dotenv" as a Composer dependency to load variables from a .env file.');
    }
    (new Dotenv())->load(__DIR__.'/../.env');
}

I think we just need mention not to refactor to:

(new Dotenv())->load(__DIR__.'/../.env');
@Cryde

This comment has been minimized.

Copy link

Cryde commented Jan 27, 2018

Any news on this ?

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Jan 27, 2018

Need to figure out how to properly handle env. Soon will create my recipe.

@antonmedv antonmedv referenced this pull request Feb 14, 2018

Closed

Symfony 4 Support #1547

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Mar 3, 2018

Any news, suggestions. Maybe need some recomendations ?

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Mar 3, 2018

In last project I just installed apache-pack and put SetEnv in .htaccess (with preg_replace with same vars for DB in deployer)
ENV for deployer only needed for doctrine migrations (except APP_ENV=prod). Maybe think in this direction ?

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Mar 4, 2018

I’m thinking on suggesting of using doyens for prod. This will require APP_ENV rrmoval

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Mar 5, 2018

Done in 443de26

@antonmedv antonmedv closed this Mar 5, 2018

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Mar 5, 2018

Maybe var/sessions also in shared ?
And default writable_mode no so good BTW.

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Mar 5, 2018

var stands for variable?

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Mar 5, 2018

var/sessions yes, my bad. Should go to shared.

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Mar 5, 2018

About writable_mode - I get errors with acl one. chown is my bet )

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Mar 5, 2018

But you should set writable_mode in deploy.php

@zorn-v

This comment has been minimized.

Copy link

zorn-v commented Mar 5, 2018

Yep, but I talk about default one.
Is acl so common ?

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Mar 5, 2018

No, but chown may require sudo. acl no. And acl more flexible.

But I'm thinking to refactor it in v7.

@Cryde

This comment has been minimized.

Copy link

Cryde commented Mar 12, 2018

Will there be a 6.0.1 ? for the missing var/sessions ? :p

@antonmedv

This comment has been minimized.

Copy link
Member

antonmedv commented Mar 12, 2018

Yes :)
Sorry for delay.

@Cryde

This comment has been minimized.

Copy link

Cryde commented Mar 12, 2018

Don't be sorry ! You do an amazing work ! We can still add it ourself :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment