Remove command registration console requirement

[blhylton]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Related Issue	N/A
Need Doc update	not likely

Describe your change

Removed the conditional if ($this->app->runningInConsole()) from the command registration. This allows for a Developer to utilize Artisan::call() in the edge cases where that is useful.

What problem is this fixing?

There will be times when the console commands need to be run from a UI, such as via a button in an administration panel. Developers should take care to handle their own errors in these cases, but preventing them from doing this is limiting the usefulness of this package. This is especially true with newer tools like Laravel Vapor and Nova working in conjunction since you don't readily have access to a CLI environment.

If limitation of these commands is actually desired and warranted, then it would probably be better to keep this change and add a condition checking for a --force flag if the app is not run from the console rather than preventing them from registering.

[blhylton]

I may have misunderstood how the automated testing works for this project. Let me know if I need to change anything. The tests all pass locally, so I feel like it's just due to the CircleCI tests not having my Algolia credentials.

[DevinCodes]

Hi @blhylton,

Thank you for opening this PR. Don't worry about the failing tests: we're currently experiencing an issue with the test suite when triggered from forks.

To register the commands, we're following the documented way of doing this, meaning we only register commands when your app is running in the console. I don't think we should change this behavior for everyone to support the use case of running the commands programmatically through a UI. Therefore, I'm closing this PR.

That being said, there's still a way to register the commands in your application when it's not running in the console. You can manually register the commands in your AppServiceProvider, like so:

    // AppServiceProvider.php

    public function boot(): void
    {
        // Your other code here...

        // You can remove any commands you won't use
        if (!$this->app->runningInConsole()) {
            $this->commands([
                \Algolia\ScoutExtended\Console\Commands\MakeAggregatorCommand::class,
                \Algolia\ScoutExtended\Console\Commands\ImportCommand::class,
                \Algolia\ScoutExtended\Console\Commands\FlushCommand::class,
                \Algolia\ScoutExtended\Console\Commands\OptimizeCommand::class,
                \Algolia\ScoutExtended\Console\Commands\ReImportCommand::class,
                \Algolia\ScoutExtended\Console\Commands\StatusCommand::class,
                \Algolia\ScoutExtended\Console\Commands\SyncCommand::class,
            ]);
        }
    }

I know this is not ideal, but I hope you understand why this is not something we want to change for all our users.

Happy coding!

[blhylton]

Preface: Tone isn't a thing on when talking via text, and I tend to be terse with words. The comment below is in no way intended to be adversarial.

I understand, but with the release of environments like Laravel Vapor where we can't reasonably run a command on CLI on-demand this is a bit of an odd one. Ultimately, it is your decision if this is how you want to approach it, but it just feels like this is a limitation without benefit or cause. Additionally, I'm not certain that this is documented anywhere, and, even though it follows the documented means, I've not encountered another 3rd party package that does this. This has caused issues before for others.

The workaround itself is not awful, but the reason I was hoping to do this at the "source" was so that it would not require additional maintenance at the application level should those commands need to change for some reason. I'll implement your suggestion for now, but I would like you to reconsider.

[DevinCodes]

Thanks for the reply @blhylton 😄

Another important reason that we register commands this way (and Laravel Scout still does it this way even after the issue you're linking to) is because running these commands can take a long time: depending on your dataset, it can take several minutes for, for example, an import to fully finalize. This is why we don't register the commands unless the app is running in the console, because the connection timeouts of a browser don't apply here.

For this reason, I'm afraid we won't change our strategy regarding command registration for the time being. If this is an issue that keeps coming up repeatedly we might reconsider in the future.

I'm sure this isn't a very satisfying answer for you, so I'd like to propose another way to run the commands through a web app which may reduce the additional maintenance if we would decide to change the commands. Does your app use queues? You could, through the UI, schedule a custom job that calls the Artisan command you want to call. Since queues run through the CLI they are not affected by timeouts, and they get registered as expected. It also ensures your users won't encounter a timeout, since scheduling a job is a lot quicker than reindexing, for example.

[blhylton]

@DevinCodes That's fair, and it should be queued regardless. This actually makes me wonder if something is keeping the command from queueing correctly on our end if that should've worked, because we were already enqueueing it via Laravel Nova's queued actions, or if those have something particular about them.

Either way, thanks for your time. It's not really a huge concern in the long run, just a minor annoyance.