Fix deprecated ValidatorInterface usage #491
Conversation
not needed. The service id does not change. there is nothing to configure |
| throw new \InvalidArgumentException('Argument 1 of '.__METHOD__.' must be an instance of Symfony\Component\Validator\Validator\ValidatorInterface or Symfony\Component\Validator\ValidatorInterface.'); | ||
| } | ||
|
|
||
| if ($validator instanceof LegacyValidatorInterface) { |
There was a problem hiding this comment.
check for !$validator instanceof ValidatorInterface instead. Otherwise you will get the deprecation warning all the time (the class implements both interfaces in Symfony for BC reasons)
|
you should add tests for this class |
soullivaneuh
force-pushed
the
deprecate-validator
branch
2 times, most recently
from
4f76cf0
to
b8a5d44
Compare
I agree, but tests are already failing on master. BTW, Travis should be set up. @schmittjoh What should I do for this? |
|
@soullivaneuh Travis is running. It is just unable to set the commit status. See #495 |
|
@stof yes, and as you can see, test are already failing: https://travis-ci.org/schmittjoh/serializer/builds/80216387 They need to be fixed before writing my own. |
|
@soullivaneuh this does not prevent writing new tests for these class, which would be passing (only 2 tests are failing in the testsuite, not the whole testsuite) |
|
Ideed. Was thinking travis broke before ending all the test. Seems not. |
soullivaneuh
force-pushed
the
deprecate-validator
branch
from
b8a5d44
to
68dc3e8
Compare
|
@stof @schmittjoh done. |
|
@soullivaneuh this is not actually testing the BC layer: the only way to test it is to force installing an old Symfony version. |
|
Okay @stof, it would be better to copy the class and make it legacy. |
|
well, you could indeed have 2 test cases rather than having 2 series of tests in the same testcase, but it would make it harder to notice that we have 2 series of tests for someone editing tests of the class later |
soullivaneuh
force-pushed
the
deprecate-validator
branch
from
68dc3e8
to
4ea4a1f
Compare
Note sure if we should update legacy tests in this case. Done for double test cases. |
soullivaneuh
force-pushed
the
deprecate-validator
branch
from
4ea4a1f
to
0c697aa
Compare
IMO we should: the tests are there to ensure that the feature works fine against both APIs, to ensure compatibility with Symfony 2.3 too |
|
@stof Thinking of your words:
Actually, with a single class like I did before, both will be tested if Travis is correctly configured with different symfony versions AND lowest deps. |
|
@soullivaneuh but not locally, making it easier to miss it. and it would also be a pain to configure tests as legacy, as your test would be tested the legacy case only in some environments but not in others |
I don't think it's needed too. We are testing our usage case, not if legacy BTW, actually it's spitted onto two classes, should be ok now. |
|
@soullivaneuh we are testing whether the serializer code works againt both interfaces, which is the job of the serializer library. Symfony maintainers cannot check whether the interface works btw: an interface never works as it has no implementation for its methods. You can only write tests for concrete code, not for interfaces. Tests are covering the implementation. |
A agree, but we are taking an interface as an argument, not writing one. Am I wrong? |
|
@soullivaneuh you are writing code against 2 different interfaces. So you should have tests covering both. This is why I vote for having a testsuite using both interfaces when they are both available, rather than only the newer one. |
|
Okay, that is done so. |
|
Is this still valid? if not i'm going to close it in some weeks |
|
replaced by #491 |
Fixes #438.
ping @stof for review.
@schmittjoh: In which configuration file this service is configured?