Laravel automations not working when not using .env file about docker-php HOT 12 CLOSED

For sure, I totally agree with this.

My proposal is I think I can eliminate this line all together:

I don't think we need to check for any of this anymore. Thoughts?

from docker-php.

I think --force flag is not a good idea for production

If we don't add --force, won't it prompt for a confirmation? Since there is no TTY, I think we need to leave it this way, correct?

Also, the user is choosing to enable this migration too... so it's not it's enabled by default.

Looking forward to hearing your thoughts!

from docker-php.

I totally agree.

The migration script is great for small apps, but for large apps we recommend running all migrations manually. 👍

from docker-php.

@jaydrogers @shinsenter Hey. Sorry for not being part of the discussion, was busy with my family. Thanks for fixing this! :-)

from docker-php.

I think the true discussion is down to this line:

This line was added when things were more automated.

Proposed change

• Maybe I just remove condition all together? I don't see a need for it now since we can explicitly enable migrations now with AUTORUN_LARAVEL_MIGRATION. Meaning if you have AUTORUN_LARAVEL_MIGRATION set to true, it should attempt to run the migrations without checking what APP_ENV you are in.

Opening up for comment

Does this sound good @dakira? @danpastori: What are your thoughts?

from docker-php.

@jaydrogers
According to the information from the official Laravel documentation, Laravel determines if either the APP_ENV environment variable has been externally provided.

Before loading your application's environment variables, Laravel determines if either the APP_ENV environment variable has been externally provided or if the --env CLI argument has been specified. If so, Laravel will attempt to load an .env.[APP_ENV] file if it exists. If it does not exist, the default .env file will be loaded.

I think it's better for letting user change the value of APP_ENV either from docker-compose.yml file or via the docker command. The default value should be set as APP_ENV_SETTING=${APP_ENV:-production}.

from docker-php.

@jaydrogers
Yes, I think you can remove that condition.

By the way, in the AUTORUN_LARAVEL_REFRESH_CACHE block, it should be only these two commands:

su - webuser -c "php $WEBUSER_HOME/artisan optimize:clear"
su - webuser -c "php $WEBUSER_HOME/artisan optimize"

Ref:

• https://github.com/laravel/framework/blob/8.x/src/Illuminate/Foundation/Console/OptimizeClearCommand.php
• https://github.com/laravel/framework/blob/8.x/src/Illuminate/Foundation/Console/OptimizeCommand.php

from docker-php.

Example of new automation script

Here's the example that I am thinking of updating it to, what does everyone think? https://github.com/serversideup/docker-php/blob/fix/adjust-automation-logic/templates/fpm/etc/cont-init.d/50-laravel-automations

AUTORUN_LARAVEL_REFRESH_CACHE

We had issues with this in the past, that's why I commented it out. I might want to circle back with @danpastori to see if it is safe to renable it. That might be done on another PR/issue though.

Let me know your thoughts on the script! 😃👍

from docker-php.

@jaydrogers
Since some migration operations are destructive, which means they may cause us to lose data.
Honestly, I think --force flag is not a good idea for production.

su - webuser -c "php $WEBUSER_HOME/artisan migrate --force"

from docker-php.

I think ${AUTORUN_LARAVEL_MIGRATION:="false"} may be a good choice for now, and it should be set to false for production.

I tried to imagine the case that I needed to run multiple instances behind a LB for production, it would be a big deal.

from docker-php.

So, the PR is LGTM

from docker-php.

Just merged this fix 🎉🚀

from docker-php.