zendframework / zend-form

Form component from Zend Framework
BSD 3-Clause "New" or "Revised" License
69 stars 87 forks source link

Feature/number parse option for number form element #96

Open GeeH opened 8 years ago

GeeH commented 8 years ago

This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html


Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/7084 User: @fabiocarneiro Created On: 2014-12-29T23:31:45Z Updated At: 2015-03-18T16:40:04Z Body Add a NumberParse option to number form element, plus set the default locale to "en" since the html5 number form element always return in this locale.


Comment

User: @Ocramius Created On: 2014-12-30T00:10:59Z Updated At: 2014-12-30T00:10:59Z Body As usual, asking for tests.


Comment

User: @fabiocarneiro Created On: 2014-12-30T06:49:22Z Updated At: 2014-12-30T08:52:11Z Body @Ocramius Sorry, forgot to add WIP to the title, i was still working on dat.

Don't know if that will be enough. I thought about creating one test for each NumberFormatter style option, but i'm not sure if this should be tested here.

Also there is no coverage for the lazy load ifs early return in getFilters and getValidators. I don't know how to fix it.


Comment

User: @Ocramius Created On: 2014-12-30T17:53:53Z Updated At: 2014-12-30T17:53:53Z Body @fabiocarneiro lazy-loading is usually tested with aggressive mocking


Comment

User: @fabiocarneiro Created On: 2014-12-30T19:31:33Z Updated At: 2014-12-30T19:44:37Z Body @Ocramius, please elaborate, if possible with example. I have no idea what aggressive mocking is.


Comment

User: @Ocramius Created On: 2014-12-30T19:34:29Z Updated At: 2014-12-30T19:34:29Z Body

there is no coverage for the lazy load ifs early return in getFilters and getValidators

This suggests that getFilters and getValidators are not being called unless strictly needed, right? You can eventually mock out those methods and expect them to never be called.

If this is not what you were meaning, then please comment the exact lines of code where you think any lazy loading should happen.


Comment

User: @fabiocarneiro Created On: 2014-12-30T19:39:53Z Updated At: 2014-12-30T19:40:52Z Body In my coverage report, it is complaining about the lack of coverage in https://github.com/zendframework/zf2/pull/7084/files#diff-05c525e17bf373c935a48e1e7b117c40R102 and https://github.com/zendframework/zf2/pull/7084/files#diff-05c525e17bf373c935a48e1e7b117c40R52 calling the method twice would make the coverage to pass, but that's not a real test, right? to make sure the early return is working propertly there i would have to make sure the filter/validator objects generated in the first call are the same ones returned in the second call.


Comment

User: @Ocramius Created On: 2014-12-30T19:42:36Z Updated At: 2014-12-30T19:42:36Z Body That doesn't require mocking: a test that asserts that the result of that operation is the same over multiple calls is enough.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 30 December 2014 at 20:39, Fábio Carneiro notifications@github.com wrote:

In my coverage report, it is complaining about https://github.com/zendframework/zf2/pull/7084/files#diff-05c525e17bf373c935a48e1e7b117c40R102 and https://github.com/zendframework/zf2/pull/7084/files#diff-05c525e17bf373c935a48e1e7b117c40R52 calling the method twice would make the coverage to pass, but that's not a real test, right? to make sure the early return is working propertly there i would have to make sure the filter/validator objects generated in the first call are the same ones returned in the second call.

— Reply to this email directly or view it on GitHub https://github.com/zendframework/zf2/pull/7084#issuecomment-68389597.


Comment

User: @fabiocarneiro Created On: 2014-12-30T19:47:12Z Updated At: 2014-12-30T19:47:12Z Body Yes @Ocramius, i got that. The problem is "how"? serialize?


Comment

User: @Ocramius Created On: 2014-12-30T19:53:50Z Updated At: 2014-12-30T19:53:50Z Body assertSame? :-)


Comment

User: @fabiocarneiro Created On: 2014-12-30T20:02:58Z Updated At: 2014-12-30T20:29:40Z Body $this->assertSame($this->getFilters(), $this->getFilters()); $this->assertSame(serialize($this->getFilters()), serialize($this->getFilters())); ??

EDIT: nvm, i read the assertSame internals, it uses ===

php > var_dump([new StdClass, new StdClass] == [new StdClass, new StdClass]); 
bool(true)
php > var_dump([new StdClass, new StdClass] === [new StdClass, new StdClass]);
bool(false)

Comment

User: @weierophinney Created On: 2015-02-09T22:25:07Z Updated At: 2015-02-09T22:25:07Z Body

We are trying to decouple components, whereas this is adding more coupling between them.

True, but Zend\Form is essentially an integration component already, as it has dependencies on Zend\InputFilter (and, by extension, Zend\Filter and Zend\Validator), and, to a degree, Zend\View (as it provides helpers).

That said, we've explicitly not had Zend\InputFilter depend on Zend\I18n, and I'm not understanding why the NumberParse filter is a requirement of the Number element. @fabiocarneiro : is this class unusable without the filter? or just unusable for your use case?

There are other possibilities here:

I'm not saying definitively that we won't merge it, but I'd really like to know the use case, and we definitely need to consider the dependency.


Comment

User: @fabiocarneiro Created On: 2015-02-16T00:27:38Z Updated At: 2015-02-16T00:27:38Z Body @weierophinney :D! In my use case, it made sense to me to have automatically parsed numbers, and the zf2 feature that does that is the NumberParse filter, which belongs to Zend\i18n.

Of course we could rely in php intl directly, but IMHO that's worse than relying in Zend\i18n.

After this PR, i had a talk with @Ocramius on IRC and we both agreed that it would be more inteligent to move this logic to another repo, like the Zend\i18n component, which already has Zend\InputFilter features, but i didn't have more time to work on that since i'm running with company things and then this issue is stuck here.

That said, some time ago I had another issue (which i don't remember exacly what was right now) and i came back in this same dependency from Zend\Form, and then I considered it important again.


Comment

User: @weierophinney Created On: 2015-03-18T16:40:04Z Updated At: 2015-03-18T16:40:04Z Body I'm removing the 2.4 milestone at this time. Solutions for the short term include, as noted above, creating your own i18n-specific implementation, and wiring it into your project.

Long term, we can touch base again after the repository split, and see if this should belong in the Zend\I18n repo instead.


weierophinney commented 4 years ago

This repository has been closed and moved to laminas/laminas-form; a new issue has been opened at https://github.com/laminas/laminas-form/issues/30.