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

[11.x] Optimize commands registry #52928

Open
wants to merge 1 commit into
base: 11.x
Choose a base branch
from

Conversation

erikgaal
Copy link
Contributor

@erikgaal erikgaal commented Sep 25, 2024

Motivation

From the docs:

When deploying your application to production, there are a variety of files that should be cached, including your configuration, events, routes, and views. Laravel provides a single, convenient optimize Artisan command that will cache all of these files.

Other than the caching commands the framework uses by default, third-party packages often also have similar commands:

Of course, developers are expected to read integration guides so they can install and configure packages accordingly. However, sometimes deployment optimization steps like these are forgotten about (I'm personally the prime example of that 🙈). Packages usually just work if you forget this but are running with sub-optimal performance costing more compute time ⏱️ , energy ⚡ and money 💰 as a result.

Wouldn't it be great that every package you install does these optimization steps automatically?

Implementation

This addition surfaces an extension point packages can use to run their own optimization handlers to be automatically run when the artisan optimize and artisan optimize:clear commands run.

It works by having an event be dispatched (Optimizing and OptimizeClearing) whenever the artisan optimize command runs. Packages can then register an event listener in their service providers that run the optimization step they need to do.

A previous implementation worked by dispatching special events that package developers could hook into. After consideration, I've taken a different approach similar to how publishes are registered in Service Providers.

The ServiceProvider class now offers a registerOptimizeCommands() method that can be used to register a command that should run during the optimize and optimize:clear commands. It also allows you to provide a key which is used to show the step in the command output.

Example:

class PackageServiceProvider extends ServiceProvider
{
    public function boot(): void
    {
        $this->commands([
            MyPackage\CacheCommand::class,
            MyPackage\ClearCommand::class
        ]);

        $this->registerOptimizeCommands(
            key: 'my package',
            optimize: 'my_package:cache',
            optimizeClear: 'my_package:clear',
        );
    }
}

Running the optimize command will now also output all the optimization commands that ran:

$ php artisan optimize

   INFO  Caching framework bootstrap, configuration, and metadata.  

  config .................................................................... 39.29ms DONE
  events ..................................................................... 2.28ms DONE
  routes .................................................................... 36.46ms DONE
  views .................................................................... 647.20ms DONE
  my package ................................................................. 0.23ms DONE

Todo

  • Find a way to show progress/tasks of packages doing their caching/clearing in the output of the command.
  • The optimize and optimize:clear commands were backported to earlier versions of Laravel, do we want to do that with this PR too?

@ollieread
Copy link
Contributor

Nice idea @erikgaal! I'd be inclined to have the third-party packages have the commands register as optimisation and optimisation clearing commands, similar to how you can register publishable assets.

$this->registerOptimizationCommands(
    optimization: 'icons:cache', 
    optimizationClearing: 'icons:clear'
);

That way it's not silent, and then it opens you up to an interactive command, something like

php artisan optimize --select

That lets you select specifically which things you want to optimise.

That's my thinking on this, for whatever it's worth. Either way, good work so far and good luck with the PR.

@erikgaal erikgaal changed the title [11.x] Optimize events [11.x] Optimize commands registry Sep 26, 2024
@erikgaal
Copy link
Contributor Author

Hey @ollieread, thanks for the suggestion. I think it aligns perfectly with what I had in mind earlier. I've updated the PR with the new implementation! 🚀

Comment on lines +46 to +50
'cache' => 'cache:clear',
'compiled' => 'clear-compiled',
'config' => 'config:clear',
'events' => 'event:clear',
'routes' => 'route:clear',
'views' => 'view:clear',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be registered in the respective service providers that are responsible for adding these commands/functionalities?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be registered in the respective service providers that are responsible for adding these commands/functionalities?

Good call, but, the order may be important

@erikgaal erikgaal force-pushed the optimize-events branch 2 times, most recently from 38088eb to 62b5698 Compare September 26, 2024 19:56
@MahdiSaremi
Copy link

This is so useful

@taylorotwell
Copy link
Member

taylorotwell commented Sep 27, 2024

I would actually prefer just having the events like you originally had. 🫠 Please mark as ready for review when the requested changes have been made.

@taylorotwell taylorotwell marked this pull request as draft September 27, 2024 15:05
@erikgaal
Copy link
Contributor Author

That would mean

  • not having any output (the original implementation), or;
  • leaking the command $components through the event and having to use Artisan::callSilently manually, or;
  • providing something on the Optimizing and OptimizeClearing events that abstracts away the command $components. $event->task(key: 'my-package', fn () => Artisan::callSilently('my-package:cache'))

Let me know what option you prefer and I'll get it done.

@shaedrich
Copy link

shaedrich commented Sep 28, 2024

$ php artisan optimize

   INFO  Caching framework bootstrap, configuration, and metadata.  

  config .................................................................... 39.29ms DONE
  events ..................................................................... 2.28ms DONE
  routes .................................................................... 36.46ms DONE
  views .................................................................... 647.20ms DONE
  my package ................................................................. 0.23ms DONE

I would actually prefer something like this:

$ php artisan optimize

   INFO  Caching framework bootstrap, configuration, and metadata.  

  config .................................................................... 39.29ms DONE
  events ..................................................................... 2.28ms DONE
  routes .................................................................... 36.46ms DONE
  views .................................................................... 647.20ms DONE
  my package:config .......................................................... 0.13ms DONE
  my package:routes .......................................................... 0.09ms DONE

or

$ php artisan optimize

   INFO  Caching framework bootstrap, configuration, and metadata.  

  config .................................................................... 39.29ms DONE
  events ..................................................................... 2.28ms DONE
  routes .................................................................... 36.46ms DONE
  views .................................................................... 647.20ms DONE
  my package ................................................................. 0.22ms DONE
      config ................................................................. 0.13ms DONE
      routes ................................................................. 0.09ms DONE

Alternatively, couldn't classes just implement something like a Cachable contract (or an attribute like #[Cachable]) and all classes that have it, are collected for caching? 🤔

@taylorotwell taylorotwell marked this pull request as ready for review October 1, 2024 14:31
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.

5 participants