-
-
Notifications
You must be signed in to change notification settings - Fork 586
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
Fixed test suite on master #578
Conversation
a8ab1fd
to
cd0ea65
Compare
$existsA = isset($order[$a]); | ||
$existsB = isset($order[$b]); | ||
|
||
if ( ! $existsA && ! $existsB) { | ||
return 0; | ||
return $currentSorting[$a] - $currentSorting[$b]; |
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.
this makes the sorting more "stable"
Thanks for the detailed explanations, makes this much faster to review :) |
"propel/propel1": "~1.7", | ||
"symfony/yaml": "2.*", | ||
"symfony/translation": "~2.0", | ||
"symfony/validator": "~2.0", | ||
"symfony/validator": "~2.0.0|~2.1.0", |
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.
this does not allow installing any maintainer version of the package, as it only allows 2. and 2.1. this is pretty bad
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.
as i understand this allows versions between 2.0.0 and 2.1.999 is it?
currently the test suite will fail with the validator 2.3.0
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.
I think that the best option is to allow ~2.3|~3.0
and test against against several versions with travis. And fix if it does not work. See like FOSUB does https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/.travis.yml. At least you can do --prefer-stable
and --prefer-lowest
test to not creating a matrix mess for each major dependency
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.
i agree that here at least 2.3 should be supported, but this requires fixing all the tests that are using ConstraintViolation
in https://github.com/schmittjoh/serializer/blob/master/tests/JMS/Serializer/Tests/Serializer/BaseSerializationTest.php
since ConstraintViolation
is not backward compatible from 2.1 to 2.3
ObjectWithVirtualProperties
(error become evident on php7)note on test changes regarding virtual properties
ObjectWithVirtualProperties
has 4 serializable fields (here fields are listed in order as the appear in theObjectWithVirtualProperties
class:The object has a custom order access rule:
Since "prop_name" and "foo" do not exist, the serialization order should be:
But all tests were expecting:
that is wrong since "VirtualValue" appears before "test" in the code