jolicode / elastically

🔍 JoliCode's Elastica wrapper to bootstrap Elasticsearch PHP integrations
248 stars 37 forks source link

Clean how the project deal with DI #78

Closed lyrixx closed 3 years ago

lyrixx commented 3 years ago

TODO

lyrixx commented 3 years ago

Looks good. Not a fan of all the constants being in the Factory class, if this class is only supposed to help peoples that don't use DIC? Feels likes unnecessary move, could you elaborate on the motivations behind this change?

To me it's odd to define constants in a class, and use theme in another class (less context switch). More over, only one use statement is needed with all in the same place. And finally, Why would we set these constant in the Client? It has nothing to do with the client.

The original code made sure to never override Elastica __construct methods, to be as much compatible as possible and compatible with future versions of Elastica. Do you think that was unnecessary precaution?

I agree with you. I'm not very confortable with that either. But since elastically extends elastica, we should not avoid to override constructor : 1/ it blocks us for future developments 2/ we write more complexe code. So I override constructor, and I pretty confident because 1/ semver exists, and I hope elastica respect it 2/ we have tests (that are run each week whatever happen).

I'm not able to test it right now, but I'm looking forward the README change to see the impact ^^

I'll do that right now :)

lyrixx commented 3 years ago

The PR is ready ✅