gbprod / elastica-bundle

Really simple bundle to use Elastica within Symfony applications
Do What The F*ck You Want To Public License
4 stars 0 forks source link

Services autowiring support #25

Closed FlyingDR closed 6 years ago

FlyingDR commented 6 years ago

Added support for providing alias for default Elasticsearch client to allow services autowiring in Symfony 3.3+. Fixes #24.

Tests coverage reduces a bit and there is several skipped tests are available for versions of Symfony lower then 3.3 because autowiring by itself is introduced only in 3.3.

codecov-io commented 6 years ago

Codecov Report

Merging #25 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master    #25   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity       45     50    +5     
=======================================
  Files             4      4           
  Lines           139    151   +12     
=======================================
+ Hits            139    151   +12
Impacted Files Coverage Δ Complexity Δ
...asticaBundle/DependencyInjection/Configuration.php 100% <100%> (ø) 5 <0> (ø) :arrow_down:
...caBundle/DependencyInjection/ElasticaExtension.php 100% <100%> (ø) 14 <5> (+5) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 411a0d7...612cfea. Read the comment docs.

gbprod commented 6 years ago

Thanks for you're contribution, I'll take some times to review before merge !

gbprod commented 6 years ago

Seems that you've not updated the CHANGELOG.md file. Can you add the changes description ?

gbprod commented 6 years ago

I wonder if we really need to add another configuration parameter to enable autowiring. I think we can use the elastica.clients.default entry as the default Elastica client (and specify this in the documentation). This will simplify the code and allow not to be required to check if autowiring is available.

So th code could be:

if (isset($config['clients']['default']) {
        $container->setAlias(Client::class, $this->createClientId('default'));
}

What do you think about that ?

FlyingDR commented 6 years ago

@gbprod I've added separate configuration entry because I'm not an author of this bundle and was not able to decide if such implicit definition will be acceptable. There is no any references in current code that would point on a convention to have default connection name so I thought that explicit definition will be better.

Of course it can be changed if you will decide to go in this way, please let me know and I will update the code.

After we will came to a point when you will consider to merge this (and another one) pull requests - I will also update CHANGELOG.md

gbprod commented 6 years ago

Yes, I think it's an approach more simple ! Can you change this and add documentation in the readme ?

FlyingDR commented 6 years ago

@gbprod is there is any plans to release new version with all recently added changes? Would be great to have them to be published officially.

gbprod commented 6 years ago

Yes, I'll do it soon ;)