Add FormBuilderIteratorTest and provide php-ini setting for precision #3714
Conversation
| @@ -45,6 +45,8 @@ | |||
| "jms/di-extra-bundle": "~1.7", | |||
| "sensio/generator-bundle": "~2.3|~3.0", | |||
| "symfony/yaml": "~2.3|~3.0", | |||
| "symfony/security-acl": "~2.4|~3.0", | |||
There was a problem hiding this comment.
Why require this is require-dev ? It already is required above.
|
Maybe rebasing on the latest master would help fixing the build (since your PR is old) ? |
|
@greg0ire My current PR is a rebase of your master. That shouldnt be a problem. |
|
Indeed, it must be something else… |
|
This is what I call good commit messages! Don't anyone dare ask you to squash that! |
| @@ -45,6 +45,7 @@ | |||
| "jms/di-extra-bundle": "~1.7", | |||
| "sensio/generator-bundle": "~2.3|~3.0", | |||
| "symfony/yaml": "~2.3|~3.0", | |||
| "phpunit/phpunit": "~4.3", | |||
There was a problem hiding this comment.
Shouldn't .travis.yml be changed to use vendor/bin/phpunit ?
There was a problem hiding this comment.
In your travis:
- composer global require phpunit/phpunit:@stable fabpot/php-cs-fixer --no-update
So I think it will use this version
There was a problem hiding this comment.
Hmmm… @core23 @OskarStark do you know why we are using @stable instead of a fixed version. It's great to be stable, but we can still can get BC-breaks.
@mikemeier : you can use this in the meantime I guess.
This is needed, because some actual floating asserts in tests only work correctly with this specific precision of 8. If this setting is not provided, php will rely on the system's provided php.ini precision setting, which than will cause some failures in the tests.
More code coverage for Form components. Checks if children are accessible.
The property 'children' is not always accessible or even available because several implementations of other FormBuilders do not have this property. Accessing private properties is never a good idea. Only rely on the given FormBuilderInterface.
| @@ -45,6 +45,7 @@ | |||
| "jms/di-extra-bundle": "~1.7", | |||
| "sensio/generator-bundle": "~2.3|~3.0", | |||
| "symfony/yaml": "~2.3|~3.0", | |||
| "phpunit/phpunit": "^5.3", | |||
There was a problem hiding this comment.
Why requiring phpunit? It's installed globally on Travis.
There was a problem hiding this comment.
Because I can! (Honest: Please see conversation on top of this site, behind this line "greg0ire and 1 other commented on an outdated diff 2 days ago") Dont know how to link direct to this.
|
I'm going a little bit late, but what is the goal of this PR? A bugfix? Do you have a concrete use case? |
|
@soullivaneuh You guys accessing a private member variable on FormBuilder with Reflection, do you think thats a good idea? (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/FormBuilder.php, property "children") This Bundle breaks already with Sonata: https://github.com/egeloen/IvoryOrderedFormBundle because it replaces the default FormBuilder (thats okay, because for this case, there is a Interface to implement). But this guy doesnt provide a property "children" -> break |
|
@soullivaneuh Also I'm providing a fixed precision php.ini value for unit tests, because some tests rely on an exact precision of 8 (comparing float values...) |
|
@soullivaneuh Last but not least, I enforce a concrete phpunit version in composer.json. Because phpunit is a tool, that should run on every developers machine. Either to check if the tests are good or/and to add new tests. How should I be able to run/write Tests, if I dont even know which phpunit you rely on? |
|
Tagged as patch because this brings no new features to the user, and does so in a BC way. |
As you said, phpunit is a tool, not a library. Except for very special cases (class conflicts for example), this should not be require on project dependencies. This bring a lots of useless dependencies not related to the bundle. BTW, your system won't work on PHP < 5.5. Just updated Travis to use your phpunit and you will see. |
|
@greg0ire What means that tag exactly in your workflow now? @soullivaneuh There is a difference between require and require-dev. phpunit should not be installed globally, as already mentioned by @greg0ire -> sebastianbergmann/phpunit#1757 Unfortunately there is no cool solution to provide different dependencies for different php-versions: composer/composer#4090 |
This is the special case I was talking about. AFAIK, we don't have this issue here. And BTW, a solution of this phpunit issue is available or under working for @mikemeier can you please remove this deps so we can merge the rest now? |
It is for us to now what semver number to bump when making a release. |
|
The related link for |
|
@greg0ire We're taking about removing commit #4f71e78 only, correct? |
|
Correct |
This is a good point : the tool could be written in java, it does not have much to do with the bundle. In fact, I think it should be a platform requirement, just like |
|
LGTM @core23 @OskarStark |
|
@greg0ire What is your current tagging strategy on sonata? Do you still use 2.X.Y where X stands for the symfony version you're supporting? Because thats not how it should be used... Thanks for merge @soullivaneuh |
|
We will release 2.4.0 and then start using semver. Big changes are coming. |
We never did like this AFAIK. |
Then have a look at this: sonata-project/SonataMediaBundle#445
Answer:
by @rande |
|
@mikemeier : you are looking for this : #3053 |
|
Nice to see that something's going on. Keep up the cool work! |
Well, this was before I start contributing on it. But I can ensure version management will not be like this anymore. |
New PR for #2562