Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Bugfix: ClassMethods strategies tied to original name #45

Conversation

BreyndotEchse
Copy link
Contributor

@BreyndotEchse BreyndotEchse commented Nov 17, 2016

All hydrators hydrate values by calling $this->hydrateValue($this->hydrateName($originalName, $data), $value, $data) (simplified).

Only ClassMethods calls hydrateValue with the original array key passed to hydrate: $this->hydrateValue($originalName, $value, $data) (simplified)

  • First commit: Demonstration test case
  • Second commit: Bugfix

@BreyndotEchse BreyndotEchse force-pushed the bugfix/classmethods-hydrate-name branch 2 times, most recently from d718ebd to 5f2d156 Compare November 17, 2016 15:20
@BreyndotEchse
Copy link
Contributor Author

Waiting for opinions on #46

@froschdesign
Copy link
Member

@BreyndotEchse
Can you provide your bugfix? Please do not change existing tests. (Example)

Thanks!

@BreyndotEchse
Copy link
Contributor Author

Cannot help you at the moment (vacation). You might take a look on @boesing's PR BreyndotEchse#1 in the meantime.

@froschdesign
Copy link
Member

@BreyndotEchse

at the moment (vacation)

Then I will not disturb you!

You might take a look on @boesing's PR BreyndotEchse#1

Great. Thank you!

@boesing
Copy link
Member

boesing commented Mar 30, 2017

@froschdesign Within my PR, I'd to change the HydratorTestTrait provided by @BreyndotEchse but the other UnitTests still passed.
Dunno why travis actually arguing about javascript.

Bugfix for strategies used in hydrators with naming strategy
@boesing
Copy link
Member

boesing commented Apr 11, 2017

The PHPUnit dependency should be raised to at least v5.2 even if this does not affect the problem.
Obviously zendframework/zend-servicemanager#07d53d35fdeb89ade8d6f72fe5ca2497f89b57f1 dropped PHPUnit 4 support.

@BreyndotEchse
Copy link
Contributor Author

@boesing there is already a PR: #55

@boesing
Copy link
Member

boesing commented Apr 11, 2017

Yah, until then, the unit tests may fail. I dont expect this to get merged with failed unit tests.
Just wanted to say.

@kokspflanze
Copy link
Contributor

these fails are no problem of that PR,

if #55 get merged before that PR, we have to change that PR, or otherwise change #55

i dont see a problem with that.

@boesing
Copy link
Member

boesing commented Apr 12, 2017

I dont have any problems with that either. Just wanted to say that former PR got declined because tests failed which didn't caused by the changes of those PR.
Just my 2 cents.

@weierophinney weierophinney merged commit 78b8a81 into zendframework:master May 17, 2017
weierophinney added a commit that referenced this pull request May 17, 2017
…-name

Bugfix: ClassMethods strategies tied to original name
weierophinney added a commit that referenced this pull request May 17, 2017
weierophinney added a commit that referenced this pull request May 17, 2017
weierophinney added a commit that referenced this pull request May 17, 2017
@weierophinney
Copy link
Member

Thanks, @BreyndotEchse

@BreyndotEchse
Copy link
Contributor Author

Thanks, @boesing :)

@boesing boesing mentioned this pull request Jul 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants