babenkoivan / elastic-migrations

Elasticsearch migrations for Laravel
MIT License
188 stars 32 forks source link

Setting blank `ELASTIC_HOST` causes exception during `composer dump-autoload` #30

Closed stevebauman closed 3 years ago

stevebauman commented 3 years ago
Software Version
PHP 7.4.21
Elasticsearch 7.12.1
Laravel 8.52.0

Describe the bug If you set ELASTIC_HOST to a blank string or null, the FreshCommand asks for an instance of IndexManagerInterface, which in turn attempts to create an ElasticSearch client with configuration.

https://github.com/babenkoivan/elastic-client/blob/9d4eab0ce97c48e60cf63c8fee7a547d41045129/config/elastic.client.php#L5

This is a problem due to running application tests using <env> supplied by a phpunit.xml file, rather than an actual .env file:

[2021-07-28 13:46:24] local.ERROR: Could not parse URI: "http://" {"exception":"[object] (Elasticsearch\\Common\\Exceptions\\InvalidArgumentException(code: 0): Could not parse URI: \"http://\" at /app/vendor/elasticsearch/elasticsearch/src/Elasticsearch/ClientBuilder.php:809)
[stacktrace]
#0 /app/vendor/elasticsearch/elasticsearch/src/Elasticsearch/ClientBuilder.php(768): Elasticsearch\\ClientBuilder->extractURIParts('http://')
#1 /app/vendor/elasticsearch/elasticsearch/src/Elasticsearch/ClientBuilder.php(715): Elasticsearch\\ClientBuilder->buildConnectionsFromHosts(Array)
#2 /app/vendor/elasticsearch/elasticsearch/src/Elasticsearch/ClientBuilder.php(667): Elasticsearch\\ClientBuilder->buildTransport()
#3 /app/vendor/elasticsearch/elasticsearch/src/Elasticsearch/ClientBuilder.php(217): Elasticsearch\\ClientBuilder->build()
#4 /app/vendor/babenkoivan/elastic-client/src/ServiceProvider.php(41): Elasticsearch\\ClientBuilder::fromConfig(Array)
#5 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(869): ElasticClient\\ServiceProvider::ElasticClient\\{closure}(Object(Illuminate\\Foundation\\Application), Array)
#6 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(754): Illuminate\\Container\\Container->build(Object(Closure))
#7 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(841): Illuminate\\Container\\Container->resolve('Elasticsearch\\\\C...', Array, true)
#8 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(692): Illuminate\\Foundation\\Application->resolve('Elasticsearch\\\\C...', Array)
#9 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(826): Illuminate\\Container\\Container->make('Elasticsearch\\\\C...', Array)
#10 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(1027): Illuminate\\Foundation\\Application->make('Elasticsearch\\\\C...')
#11 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(947): Illuminate\\Container\\Container->resolveClass(Object(ReflectionParameter))
#12 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(908): Illuminate\\Container\\Container->resolveDependencies(Array)
#13 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(754): Illuminate\\Container\\Container->build('ElasticAdapter\\\\...')
#14 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(841): Illuminate\\Container\\Container->resolve('ElasticAdapter\\\\...', Array, true)
#15 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(692): Illuminate\\Foundation\\Application->resolve('ElasticAdapter\\\\...', Array)
#16 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(826): Illuminate\\Container\\Container->make('ElasticAdapter\\\\...', Array)
#17 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(1027): Illuminate\\Foundation\\Application->make('ElasticAdapter\\\\...')
#18 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(947): Illuminate\\Container\\Container->resolveClass(Object(ReflectionParameter))
#19 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(908): Illuminate\\Container\\Container->resolveDependencies(Array)
#20 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(754): Illuminate\\Container\\Container->build('ElasticMigratio...')
#21 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(841): Illuminate\\Container\\Container->resolve('ElasticMigratio...', Array, false)
#22 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(294): Illuminate\\Foundation\\Application->resolve('ElasticMigratio...', Array, false)
#23 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(869): Illuminate\\Container\\Container->Illuminate\\Container\\{closure}(Object(Illuminate\\Foundation\\Application), Array)
#24 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(754): Illuminate\\Container\\Container->build(Object(Closure))
#25 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(841): Illuminate\\Container\\Container->resolve('ElasticMigratio...', Array, true)
#26 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(692): Illuminate\\Foundation\\Application->resolve('ElasticMigratio...', Array)
#27 /app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(826): Illuminate\\Container\\Container->make('ElasticMigratio...', Array)
#28 /app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(175): Illuminate\\Foundation\\Application->make('ElasticMigratio...')
#29 /app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(124): Illuminate\\Container\\BoundMethod::addDependencyForCallParameter(Object(Illuminate\\Foundation\\Application), Object(ReflectionParameter), Array, Array)
#30 /app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(36): Illuminate\\Container\\BoundMethod::getMethodDependencies(Object(Illuminate\\Foundation\\Application), Array, Array)
#31 /app/vendor/laravel/framework/src/Illuminate/Container/Util.php(40): Illuminate\\Container\\BoundMethod::Illuminate\\Container\\{closure}()
#32 /app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(93): Illuminate\\Container\\Util::unwrapIfClosure(Object(Closure))
#33 /app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(37): Illuminate\\Container\\BoundMethod::callBoundMethod(Object(Illuminate\\Foundation\\Application), Array, Object(Closure))
#34 /app/vendor/laravel/framework/src/Illuminate/Container/Container.php(651): Illuminate\\Container\\BoundMethod::call(Object(Illuminate\\Foundation\\Application), Array, Array, NULL)
#35 /app/vendor/laravel/framework/src/Illuminate/Console/Command.php(136): Illuminate\\Container\\Container->call(Array)
#36 /app/vendor/symfony/console/Command/Command.php(299): Illuminate\\Console\\Command->execute(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Illuminate\\Console\\OutputStyle))
#37 /app/vendor/laravel/framework/src/Illuminate/Console/Command.php(121): Symfony\\Component\\Console\\Command\\Command->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Illuminate\\Console\\OutputStyle))
#38 /app/vendor/symfony/console/Application.php(978): Illuminate\\Console\\Command->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#39 /app/vendor/symfony/console/Application.php(295): Symfony\\Component\\Console\\Application->doRunCommand(Object(ElasticMigrations\\Console\\FreshCommand), Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#40 /app/vendor/symfony/console/Application.php(167): Symfony\\Component\\Console\\Application->doRun(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#41 /app/vendor/laravel/framework/src/Illuminate/Console/Application.php(92): Symfony\\Component\\Console\\Application->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#42 /app/vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php(129): Illuminate\\Console\\Application->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#43 /app/artisan(37): Illuminate\\Foundation\\Console\\Kernel->handle(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#44 {main}
"} 

Resolution

Console type-hinted dependencies should be moved to the handle methods so they are only retrieved when the commands are ran, instead of during every single request. Ex:

Before:

public function __construct(
    Migrator $migrator,
    MigrationRepository $migrationRepository,
    IndexManagerInterface $indexManager
) {
    // ...
}

After:

public function handle(
    Migrator $migrator,
    MigrationRepository $migrationRepository,
    IndexManagerInterface $indexManager
) {
    // ...
}

https://laravel.com/docs/8.x/artisan#command-structure

Note that we are able to request any dependencies we need via the command's handle method. The Laravel service container will automatically inject all dependencies that are type-hinted in this method's signature:

To Reproduce

  1. Make a fresh Laravel application
  2. Run composer require babenkoivan/elastic-migrations
  3. Remove ELASTIC_HOST default value
  4. Run composer dump-autoload
stevebauman commented 3 years ago

I can write a PR to move all of these to the handle method and adjust the tests to follow suit, just let me know if you're okay with that 👍

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days

babenkoivan commented 3 years ago

Hey @stevebauman, sorry for the late response, I was sure I've answered on this one. Please go ahead with the PR if it's still relevant.

stevebauman commented 3 years ago

No worries @babenkoivan! Appreciate the response. I've just submitted the PR 👍