Skip to content
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

Feature/number parse option for number form element #30

Open
weierophinney opened this issue Dec 31, 2019 · 0 comments
Open

Feature/number parse option for number form element #30

weierophinney opened this issue Dec 31, 2019 · 0 comments

Comments

@weierophinney
Copy link
Member

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
zendframework/zendframework#7084 (comment).


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:

  • Add a Form subcomponent in the Zend\I18n namespace, and have an i18n-specific extension there.
  • Offer it in a personal library/package

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.



Originally posted by @GeeH at zendframework/zend-form#96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant