search

yiisoft / yii2-elasticsearch

Yii 2 Elasticsearch extension
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
429 stars 252 forks source link

Fix di #304

Closed raidkon closed 3 years ago

raidkon commented 3 years ago
Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? ?
Fixed issues Fix di
samdark commented 3 years ago

@raidkon seems to fail tests.

beowulfenator commented 3 years ago

@samdark, this fails because of https://github.com/yiisoft/yii2-elasticsearch/pull/301 Is now the time to re-revert that PR?

bizley commented 3 years ago

It's corrected already, only awaits 2.0.42 release.

samdark commented 3 years ago

Alright. @beowulfenator what do you think about this change?

beowulfenator commented 3 years ago

Alright. @beowulfenator what do you think about this change?

I'm sorry, I have zero experience with DI. Looks legit, but what do I know? :)

bizley commented 3 years ago

I'm not sure about this. While it will benefit from container definition setting for sure it will be slower now. Is there a valid use case?

samdark commented 3 years ago

@raidkon what's your use case?

samdark commented 3 years ago

@bizley it's now released. Tests still fail, it seems.

bizley commented 3 years ago

It seems to be because of https://github.com/yiisoft/yii2/commit/54f25c4b04a0c1fb24b9c9583c878555f5d1ecef#diff-78dc0b3aa7bc8f25f23245e9739f7b411ca14be977bdec098fde0d672618c48e I'll try to fix the problem.

bizley commented 3 years ago

It looks like it's better to fix it in the core. I've missed the fact that most ActiveQuery versions don't have on property.

samdark commented 3 years ago

Alright. Solved now :)

samdark commented 3 years ago

@raidkon would you please add a changelog and a test for the change?

bizley commented 3 years ago

Since @raidkon is not eager to provide test for this or even a use case I vote for closing it.