-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[WIP] Update library/Zend/Stdlib/Hydrator/ClassMethods.php #3754
Conversation
ucfirst at the previous point forces you to have lowercase first properties whereas if you are dealing with an existing table that already has Uppercase first, then the Hydrator wont work. The fix above maintains everything you had before but also allows for the use of uppercase first property e.g. protected $FirstName; MSSQL tables usually have this naming convention
} else { | ||
$attribute = lcfirst($attribute); | ||
$attributes[$attribute] = $this->extractValue($attribute, $object->$method()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code MUST use 4 spaces for indenting, see https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md
I think this make tests failure in tests\ZendTest\Stdlib\HydratorStrategyTest and tests\ZendTest\Stdlib\HydratorTest |
Hi @mabuzagu |
Formated according to formatting standars
Thanks for the comments samsonasik and ralph. I have made the changes and it looks like the tests are running ok |
@mabuzagu Please add a unit test for the new behavior, to ensure we don't have a future regression that removes this capability. Thanks! |
|
||
if(property_exists($object, $attribute)) { | ||
$attributes[$attribute] = $this->extractValue($attribute, $object->$method()); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you perhaps correct the indenting here (and the line above this, and the block below as well)? If it all looks okay in your editor, please make sure you use spaces. Not tabs. (we try to adhere to PSR2, which uses 4 spaces per tab: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md )
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will do
@mabuzagu Any progress on the unit tests? If you need help, jump into #zftalk.dev on Freenode IRC, and ask for assistance -- lots of folks who will be able to help you there! |
Sorry about the delay. Pass couple week have been hell :). But I think this week I should be able to get to it. I started on a some time back but seemed my solution was still causing the UT to fail. So I need to look at it more closely. You will have something from me soon. Sorry for the delay |
@mabuzagu Will you be able to finish this? If not, I'll close, and you can re-submit when you have tests. At this point, your branch is getting more and more out-of-date, and I worry that the merge will be difficult. |
I gave it a try and of course the unit tests failed :) Give me until the Monday. I should be able to lock myself somewhere and do it. I just need to different approach and some thorough testing. Thanks for your patience |
@weierophinney. Its ready now. Tests are good to go. Thanks for your patience |
CS fixes for this are on #4229. |
[WIP] Update library/Zend/Stdlib/Hydrator/ClassMethods.php
As this provides a new feature, I've merged it to the develop branch for release with 2.2.0. Thanks! |
…ch-1 [WIP] Update library/Zend/Stdlib/Hydrator/ClassMethods.php
ucfirst at the previous point forces you to have lowercase first properties
whereas if you are dealing with an existing table that already has
Uppercase first, then the Hydrator wont work. The fix above maintains everything
you had before but also allows for the use of uppercase first property e.g.
protected $FirstName; MSSQL tables usually have this naming convention