New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct Put class for updating settings. Fixes #1295 #1296

Merged
merged 1 commit into from May 5, 2017

Conversation

Projects
None yet
5 participants
@nickygerritsen
Contributor

nickygerritsen commented May 4, 2017

This fixes #1295 and adds a test that verifies this.

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa May 4, 2017

Collaborator

Strange, that another tests in this SettingsTest class are written in a same way, but they do not fail...

I haven't ability to check it right now, but did your test fail, if you revert changes in "lib/Elastica/Index.php" back?

Maybe there is wrong tests if it is not fail...

Collaborator

ewgRa commented May 4, 2017

Strange, that another tests in this SettingsTest class are written in a same way, but they do not fail...

I haven't ability to check it right now, but did your test fail, if you revert changes in "lib/Elastica/Index.php" back?

Maybe there is wrong tests if it is not fail...

@nickygerritsen

This comment has been minimized.

Show comment
Hide comment
@nickygerritsen

nickygerritsen May 4, 2017

Contributor

Yeah my test failed before. The other tests in this class are different: they use getSetting and then set on that object instead of directly using setSettings on the index. The later is what was broken (and not tested at all yet).

Contributor

nickygerritsen commented May 4, 2017

Yeah my test failed before. The other tests in this class are different: they use getSetting and then set on that object instead of directly using setSettings on the index. The later is what was broken (and not tested at all yet).

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa May 4, 2017

Collaborator

@nickygerritsen Indeed 👍

ping @ruflin

Collaborator

ewgRa commented May 4, 2017

@nickygerritsen Indeed 👍

ping @ruflin

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin May 5, 2017

Owner

@nickygerritsen Could you put a note in the CHANGELOG.md file?

Owner

ruflin commented May 5, 2017

@nickygerritsen Could you put a note in the CHANGELOG.md file?

@ruflin

ruflin approved these changes May 5, 2017

@nickygerritsen

This comment has been minimized.

Show comment
Hide comment
@nickygerritsen

nickygerritsen May 5, 2017

Contributor

Done!

Contributor

nickygerritsen commented May 5, 2017

Done!

@ruflin ruflin merged commit 4b5781f into ruflin:master May 5, 2017

3 checks passed

codecov/patch Coverage not affected when comparing e69243a...e53d130
Details
codecov/project 84.4% (+0.19%) compared to e69243a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin May 5, 2017

Owner

@nickygerritsen Thanks for the fix
@nickygerritsen @ewgRa Do we need some follow up PR's to fix more stuff?

Owner

ruflin commented May 5, 2017

@nickygerritsen Thanks for the fix
@nickygerritsen @ewgRa Do we need some follow up PR's to fix more stuff?

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa May 5, 2017

Collaborator

@ruflin I don't think so, it was kind of typo, and fully fixed and covered by test now.

Collaborator

ewgRa commented May 5, 2017

@ruflin I don't think so, it was kind of typo, and fully fixed and covered by test now.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion May 31, 2017

Collaborator

@ruflin can you make a patch release as this bug could affect many people?

Collaborator

Tobion commented May 31, 2017

@ruflin can you make a patch release as this bug could affect many people?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jun 2, 2017

Owner

@Tobion Yes, that is possible. Put it on my TODO list for next week. Hope that works.

Owner

ruflin commented Jun 2, 2017

@Tobion Yes, that is possible. Put it on my TODO list for next week. Hope that works.

@mweibel

This comment has been minimized.

Show comment
Hide comment
@mweibel

mweibel Jun 7, 2017

Contributor

@ruflin +1 on the patch release. Affects us right now :) Thanks! 👍

Contributor

mweibel commented Jun 7, 2017

@ruflin +1 on the patch release. Affects us right now :) Thanks! 👍

@ruflin

This comment has been minimized.

Show comment
Hide comment
@mweibel

This comment has been minimized.

Show comment
Hide comment
@mweibel

mweibel Jun 7, 2017

Contributor

Thanks! I noticed 5.2.1 has a BC break if you override Elastica\Client::request()

see: FriendsOfSymfony/FOSElasticaBundle#1258

Contributor

mweibel commented Jun 7, 2017

Thanks! I noticed 5.2.1 has a BC break if you override Elastica\Client::request()

see: FriendsOfSymfony/FOSElasticaBundle#1258

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jun 12, 2017

Owner

Oh, good point :-( Should we add this to the CHANGELOG? Thanks for fixing it in the bundle.

Owner

ruflin commented Jun 12, 2017

Oh, good point :-( Should we add this to the CHANGELOG? Thanks for fixing it in the bundle.

mhernik pushed a commit to mhernik/Elastica that referenced this pull request Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment