EZP-23445: Take SYMFONY_ENV into account in asset dump composer scripts #1627

andrerom · 2016-03-29T13:38:14Z

Issue: https://jira.ez.no/browse/EZP-23445
Target: 6.3 (contains deprecations and changes)

With Symfony_ENV being now used most other places as of eZ Platform 1.0, it now makes sense to take it into account here as well.
Get rid of the question, behave more like other commands, and use ezpublish-asset-dump-env for bc but deprecate with a error saying use of Symfony_ENV is preferred.

andrerom · 2016-03-29T14:37:00Z

eZ/Bundle/EzPublishCoreBundle/Composer/ScriptHandler.php

-                "<question>Which environment would you like to dump production assets for?</question> (Default: 'prod', type 'none' to skip) ",
-                'prod'
+                "<question>'SYMFONY_ENV' environment variable not defined, which environment would you like to dump production assets for?</question> (Default: 'dev', type 'none' to skip) ",
+                'dev'


This switch to dev by default, as well as the one below, is a separate commit here as means to better solve https://jira.ez.no/browse/EZP-23159 so we can change composer.json to once again generate assets on composer update.

This is somewhat needed since composer 1.0.0-beta2, where composer install executes post-update-cmd instead of post-install-cmd, when there is no composer.lock file: https://github.com/composer/composer/releases/tag/1.0.0-beta2

This implies we should change composer.json in ezplatform, and ideally also adjust install instructions to mention that SYMFONY_ENV should be used for installing with prod env.

This sounds both major and minor... I'm not sure I understand that part. Why does it matter of update or install script is executed ? Did this affect the environment we run things for ?

What bothers me is that this prod / dev change is a behaviour change (I'm thinking automatic scripts).

This sounds both major and minor... I'm not sure I understand that part. Why does it matter of update or install script is executed ? Did this affect the environment we run things for ?

It matters for us only because we don't currently dump assets on update, which was done to avoid the crashes caused when this command picked prod, while others before it (like cache clearing sometimes needed on updates) picked dev.

What bothers me is that this prod / dev change is a behaviour change (I'm thinking automatic scripts).

This is true, but it also untangles a slew of workarounds, so for me this part should be 6.3 (master) only.

@bdunogier about automated scripts: it was hard to automate before this PR, as you normally have only 1 composer.json in the repo but use it in many envs, so you can not put in it the name of one environment. The only way for me to actually automate it is to have shell code alter the composer.json after a git pull and before a composer-install call.
In other words: someone might have relied on the old default, but I think that it is not many users / not the heavy-automating users

Afaik the automated cases falls into two categories:

Those that had ezpublish-asset-dump-env set to something else then prod => still works

Those that set SYMFONY_ENV and relied on defaults here => still works

Other cases basically had bugs, so they might have dumped twice to workaround, which will still work, and they can remove the extra dump as soon as they'd like.

This is true, but it also untangles a slew of workarounds, so for me this part should be 6.3 (master) only.

Perfect.

But according to your arguments, you sounded like you'd rather apply this to earlier branches (I'm not dead set against it), as it entangles workaround (a positive thing to have). Misunderstanding ?

Note: if we end up backporting, please make sure you reopen a PR against 6.2, so that Travis runs (because of install process).

But according to your arguments, you sounded like you'd rather apply this to earlier branches (I'm not dead set against it), as it entangles workaround (a positive thing to have). Misunderstanding ?

so both commits in 6.2?

gggeek · 2016-03-29T15:11:29Z

finally!!!

emodric · 2016-03-30T22:55:59Z

eZ/Bundle/EzPublishCoreBundle/Composer/ScriptHandler.php

@@ -25,12 +25,13 @@ public static function dumpAssets(CommandEvent $event)
        $options = self::getOptions($event);
        $appDir = $options['symfony-app-dir'];
        $webDir = $options['symfony-web-dir'];
-        $env = isset($options['ezpublish-asset-dump-env']) ? $options['ezpublish-asset-dump-env'] : '';
+        // @deprecated: Use of ezpublish-asset-dump-env is deprecated, use standard SYMFONY_ENV instead.


So if ezpublish-asset-dump-env is deprecated, once it is removed, how to automatically dump assets for whatever environment is picked, without user having to confirm? (based of course, on existence and value of SYMFONY_ENV and default being dev)

You're saying SYMFONY_ENV doesn't work for you? I guess your talking install by human as opposed to deployment. I kind of had in mind there would be a sf composer setting for this we could use instead but did not find any. Alternative is to skip deprecating it for instance.

It would work in production since we use SYMFONY_ENV, but in dev we don't use SYMFONY_ENV but I would still like not to be interrupted and just use the default dev value.

But for DEV you don't dump assets anyway, do you?
As for us, we do use SYMFONY_ENV in dev as well - with containres and vms now we try to set up dev envs as close to prod as possible. We went as far as having a 'staging' env set up on our dev environment where eZ is accessed through nginx and varnish, so every single developer can troubleshoot caching problems :-)

We don't, but script still stalls and asks for it. We also use vagrant so over there, using SYMFONY_ENV=dev wouldn't be a problem, but some run eZ on local machines (I run 7 instances at all times, both in prod and dev and reinstall on daily bases), so I use ezpublish-asset-dump-env to just dump for prod, without asking.

Second proposal: I still think ezpublish-asset-dump-env should be deprecated, because it is flawed given it is not taken into account for cache clear, so if you' haven't set SYMFONY_ENV to prod, it will potentially crash as cache for prod haven't been cleared on composer updates.

Thump up: Deprecate, and as I and @bdunogier kind of intend start to adapt documentation for optionally setting SYMFONY_ENV=prod when doing install, otherwise dev is assumed.
Thumb down: Find or create another composer config that can be taken into account for all install/update commands (note: I have no idea how this is supposed to work, overriding all scripts is not really an option, maybe we need to make a composer plugin for this)

I'm okay with that too, as long as it gets rid of the question. So, using dev by default, unless SYMFONY_ENV is set.

@emodric 😄

andrerom · 2016-04-02T16:32:19Z

@emodric Updated

emodric · 2016-04-02T17:00:20Z

eZ/Bundle/EzPublishCoreBundle/Composer/ScriptHandler.php

+            $command .= ' --env=' . escapeshellarg($options['ezpublish-asset-dump-env']);
+        } else if (($env = getenv('SYMFONY_ENV')) === false || $env === 'dev') {
+            // No need to dump assets for dev
+            return self::dumpAssetsHelpText($event);


This is kinda misleading. I'd expect for assets to still be dumped, even in dev. For example, ez and sylius integration depends on assets being dumped in dev since Sylius uses them.

EDIT: Is there any reason why you wouldn't want assets being dumped in dev? I mean, it certainly doesn't hurt.

Also, what if someone disables assetic controller that dynamically dumps the assets? There's no way to configure this behaviour.

I dont have any pov on this, tought you implied this.

Well, I said I don't dump assets in dev in pure eZ Platform, but that doesn't mean that it shouldn't be done at all without being able to enable it if required (ez+sylius being an example).

andrerom · 2016-04-04T14:43:57Z

@emodric, @bdunogier, @gggeek updated code/pr taking everything into account now, so unless we talked past each-other this should be ready for voting.

emodric · 2016-04-04T15:00:59Z

Looks good to me.

+1

andrerom · 2016-04-05T18:48:41Z

Thanks @emodric :)

andrerom reviewed Mar 29, 2016
View reviewed changes

emodric reviewed Mar 30, 2016
View reviewed changes

andrerom force-pushed the asset_dump_sf_env branch from 7179dbf to 758bb0d Compare April 2, 2016 16:31

emodric reviewed Apr 2, 2016
View reviewed changes

EZP-23445: Take SYMFONY_ENV into account in asset dump composer scripts

2d4c202

andrerom force-pushed the asset_dump_sf_env branch from 758bb0d to 2d4c202 Compare April 2, 2016 20:13

andrerom added the Ready for review label Apr 4, 2016

andrerom merged commit 027897c into master Apr 5, 2016

andrerom deleted the asset_dump_sf_env branch April 5, 2016 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-23445: Take SYMFONY_ENV into account in asset dump composer scripts #1627

EZP-23445: Take SYMFONY_ENV into account in asset dump composer scripts #1627

andrerom commented Mar 29, 2016

andrerom Mar 29, 2016

bdunogier Mar 30, 2016

andrerom Mar 30, 2016

gggeek Mar 30, 2016

andrerom Mar 30, 2016

bdunogier Mar 30, 2016

andrerom Mar 30, 2016

gggeek commented Mar 29, 2016

emodric Mar 30, 2016

andrerom Mar 31, 2016

emodric Mar 31, 2016

gggeek Mar 31, 2016

emodric Mar 31, 2016

emodric Mar 31, 2016

andrerom Mar 31, 2016

emodric Mar 31, 2016

andrerom Mar 31, 2016

emodric Mar 31, 2016

andrerom commented Apr 2, 2016

emodric Apr 2, 2016

andrerom Apr 2, 2016

emodric Apr 2, 2016

andrerom commented Apr 4, 2016

emodric commented Apr 4, 2016

andrerom commented Apr 5, 2016

EZP-23445: Take SYMFONY_ENV into account in asset dump composer scripts #1627

EZP-23445: Take SYMFONY_ENV into account in asset dump composer scripts #1627

Conversation

andrerom commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gggeek commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom commented Apr 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom commented Apr 4, 2016

emodric commented Apr 4, 2016

andrerom commented Apr 5, 2016