Skip to content
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

Use AUTRORUN_LARAVEL_OPTIMIZE to run Laravel's artisan optimize command. #511

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

aSeriousDeveloper
Copy link

As discussed in #510 .

This draft PR will allow for an additional, optional environment variable that will run php artisan optimize instead of the usual separate cache commands.

The purpose of this is that other dependencies hook into this command which cannot be easily accounted for in the autorun. However, the fact that the individual cache functions can be configured to be enabled or disabled means we can't simply switch to optimize.

Instead, we can check if the new environment variable AUTRORUN_LARAVEL_OPTIMIZE is set to true. If it is, we simply run artisan optimize. If the variable is not set (or is set to false), then call the cache_laravel function, which performs the same logic as always.

By default, AUTRORUN_LARAVEL_OPTIMIZE will be resolved to false if not specified, so the original & default configuration for the Dockerfile will be preserved.

@aSeriousDeveloper
Copy link
Author

Huh, found out The optimize function has a new argument that'll be useful here from. v11.38.

The PR is here.

You can use --except to specify specific cache functions you want to skip when running optimize. You could simply call optimize instead of the individual commands, and if one of them is set to false include them in the except.

Caveat here is using older versions of Laravel, they wouldn't support this.

@aSeriousDeveloper aSeriousDeveloper marked this pull request as ready for review January 16, 2025 13:00
@aSeriousDeveloper
Copy link
Author

aSeriousDeveloper commented Jan 16, 2025

I think this might be ready for review / discussion now.
As mentioned in the previous comment, Laravel v11.38.0 and above support the use of --except when optimizing. This lets you decide which optimizations to skip.

I've added this into the script so that the AUTORUN_LARAVEL_{type}_CACHE variables can still be respected. However, if you're using a version of Laravel < 11.38.0, this command will fail and optimize without exceptions is ran as a fallback. I've made sure to mention this in the docs.

I will mention, I'm not too well-versed in bash shell, so someone else may need to review & give this some checking.

I'd say a future development of this could be that the optimization skips can be specified by a single environment variable, such as AUTORUN_LARAVEL_OPTIMIZE_EXCEPT. You can specify this as a comma-separated list of any optimizations to skip, including any that may come from external dependencies. However, this is a little out of scope for this PR, so I'll leave that for a later date.

@jaydrogers
Copy link
Member

Thanks! I will take a peek once I get a moment.

The challenging part is we cannot use Bash (/bin/bash), we must use Shell (/bin/sh) 🙃

I'll review and see what we have going on. I want to make sure the user experience is great for existing users (especally ones before Laravel 11) and the upgrade is seamless.

I'll keep you posted!

@aSeriousDeveloper
Copy link
Author

Made a few changes to make sure it has better compatibility with Shell (it does use POSIX right?), and with the script getting a bit bigger from adding the extra logic I decided to refactor the artisan calls into their own functions. I can reverse the refactoring if preferred.

Co-authored-by: Paul <[email protected]>
@jaydrogers
Copy link
Member

Thanks! Yes, it should be /bin/sh and POSIX compliant.

I'll review sometime (likely next week) as this week has been a busy client week. I greatly appreciate you tackling this!

I also just approved the workflow, so hopefully it will build some preliminary images to test with on DockerHub

aSeriousDeveloper and others added 3 commits February 6, 2025 10:13
Co-authored-by: Erik Gaal <[email protected]>
Co-authored-by: Erik Gaal <[email protected]>
Co-authored-by: Erik Gaal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants