Move Constraint validator test case to Test namespace #17203

wouterj · 2015-12-31T16:41:57Z

Q	A
Bug fix?	no
New feature?	kinda
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

The Validator component has a very usefull test utility to test constraint validators. It's only hidden deep in the Tests namespace. Just as was done with the TypeTestCase of the Form component some years ago, I think it's usefull to move the constraint test case to the Test namespace as well.

Btw, my last PR of the year and the first deprecation of 3.1 🎆

stof · 2015-12-31T16:45:15Z

src/Symfony/Component/Validator/Tests/Constraints/AbstractConstraintValidatorTest.php

-            $this->constraint,
-            $this->cause
-        );
+        trigger_error('The '.__NAMESPACE__.'/AbstractConstraintValidatorTest is deprecated since version 3.1 and will be removed in 3.0. Use Symfony\Component\Validator\Test\ConstraintValidatorTestCase instead.', E_USER_DEPRECATED);


missing silencing

and it cannot be removed in 3.0...

wouterj · 2015-12-31T17:29:47Z

Thanks for your quick review, @stof! I've fixed the PR now.

Tobion · 2016-01-12T12:51:25Z

We do not maintain BC for test files in Tests namespace. So these deprecations are not needed.

nicolas-grekas · 2016-01-13T09:39:44Z

I agree with @Tobion, no need to preserve BC for tests
Status: needs work

stof · 2016-01-13T09:59:33Z

Well, given that some tests are using this class in other components, we would have to preserve BC (and people might use it elsewhere too already). The point is that this base testcase is meant to be extended when writing tests. We just messed up its location when introducing it (and this is the drawback of the inclusion of tests inside the runtime, as our testsuite is autoloadable by any code installing the component as a dependency)
We were maintaining BC for similar base testcases in the Form component in 2.x

nicolas-grekas · 2016-01-13T10:07:46Z

src/Symfony/Bridge/Doctrine/composer.json

@@ -28,7 +28,7 @@
        "symfony/property-info": "~2.8|3.0",
        "symfony/security": "~2.8|~3.0",
        "symfony/expression-language": "~2.8|~3.0",
-        "symfony/validator": "~2.8|~3.0",
+        "symfony/validator": "~3.1",


This is an issue: we won't test the bridge with symfony/validator anymore, which could hide BC breaks.
We need to find a way to keep ~2.8 here.
(same comment for the other cases below)

Why wouldn't this test the bridge with symfony/validator anymore? It would simply be tested against 3.1, or am I missing something?

ping @nicolas-grekas

@nicolas-grekas Can you give us more insights about this?

I think the point is that the test suite tests both against the highest and lowest possible dependencies. Since 3.1 is the minimum boundary, even the --prefer-lowest tests will run against 3.1 and not against 2.8, which might hide compatibility issues.

So the solution would be to keep both classes (new and old) and use the old one in the core libraries, so we can still properly support and test 2.8 and 3.0?

I think that would be good, yes.

nicolas-grekas · 2016-01-13T10:08:59Z

@stof, then I suggest to NOT deprecate anything in this PR

stof · 2016-01-13T10:37:29Z

@nicolas-grekas I don't understand what you mean. Do you suggest moving the class without deprecating the old class name ?

nicolas-grekas · 2016-01-13T10:44:24Z

@stof yes, that's the best idea I have for now to still allow ~2.8 in composer json files...

fabpot · 2016-01-25T07:50:59Z

I think we are going too far with BC. Files under Tests are internal and private. Let's move the file and not care about BC here.

wouterj · 2016-01-25T14:10:35Z

Alright, removed the class and file.

fabpot · 2016-02-29T17:04:57Z

@nicolas-grekas The only remaining issue is the composer deps changes
@wouterj Can you rebase?

wouterj · 2016-02-29T19:15:01Z

Rebased

Status: Needs review

stof · 2016-02-29T19:37:28Z

UPGRADE-3.1.md

@@ -1,6 +1,7 @@
 UPGRADE FROM 3.0 to 3.1
 =======================

+<<<<<<< HEAD


Conflict resolution issue

wouterj · 2016-02-29T20:05:02Z

Thanks, fixed your comments @stof.

xabbuh · 2016-03-01T20:27:28Z

This seems to need a rebase again.

webmozart · 2016-03-02T08:52:15Z

UPGRADE-3.1.md

+   // ...
+   use Symfony\Component\Validator\Test\ConstraintValidatorTestCase;
+
+   class MyCustomValidatorTest extends ConstraintValidatorTestCase


Did we move away from prefixing abstract classes with Abstract (in general)?

Hmm, I'm not sure actually. But it seems to be more common to not add the prefix for abstract classes in Symfony.

fabpot · 2016-06-15T08:38:24Z

@wouterj IIUC, it would be better to keep AbstractConstraintValidatorTest.php for now along side the new one and keep 2.8 as the min requirement. Is that correct @nicolas-grekas? If yes, let's do this and merge.

wouterj · 2016-06-15T09:44:04Z

Updated the PR to keep both classes and deprecate the old one. We can't trigger a deprecation warning, as we can't update all core packages to use the new class.

fabpot · 2016-06-15T16:13:17Z

Thank you @wouterj.

…(WouterJ) This PR was merged into the 3.2-dev branch. Discussion ---------- Move Constraint validator test case to Test namespace | Q | A | ------------- | --- | Bug fix? | no | New feature? | kinda | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - The Validator component has a very usefull test utility to test constraint validators. It's only hidden deep in the Tests namespace. Just as was done with the TypeTestCase of the Form component some years ago, I think it's usefull to move the constraint test case to the Test namespace as well. *Btw, my last PR of the year and the first deprecation of 3.1* 🎆 Commits ------- e938361 Move Constraint validator test case to Test namespace

carsonbot added the Status: Needs Review label Dec 31, 2015

stof reviewed Dec 31, 2015
View reviewed changes

wouterj force-pushed the validator-move_constraint_test branch from 30ea9a3 to cbf1f77 Compare December 31, 2015 17:29

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 13, 2016

nicolas-grekas reviewed Jan 13, 2016
View reviewed changes

wouterj force-pushed the validator-move_constraint_test branch from cbf1f77 to 5c516d8 Compare January 25, 2016 14:10

wouterj force-pushed the validator-move_constraint_test branch from 5c516d8 to 5eeb754 Compare February 29, 2016 19:14

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 29, 2016

stof reviewed Feb 29, 2016
View reviewed changes

UPGRADE-3.1.md

@@ -1,6 +1,7 @@

UPGRADE FROM 3.0 to 3.1

=======================

<<<<<<< HEAD

Copy link

Member

stof Feb 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflict resolution issue

wouterj force-pushed the validator-move_constraint_test branch from 5eeb754 to d4ff6ff Compare February 29, 2016 20:04

webmozart reviewed Mar 2, 2016
View reviewed changes

javiereguiluz added Validator Enhancement labels Mar 3, 2016

javiereguiluz added the Deprecation label Mar 7, 2016

wouterj force-pushed the validator-move_constraint_test branch from d4ff6ff to ed33a2d Compare June 15, 2016 09:42

wouterj force-pushed the validator-move_constraint_test branch from ed33a2d to 0e868ee Compare June 15, 2016 09:44

Move Constraint validator test case to Test namespace

e938361

wouterj force-pushed the validator-move_constraint_test branch from 0e868ee to e938361 Compare June 15, 2016 14:32

fabpot merged commit e938361 into symfony:master Jun 15, 2016

wouterj deleted the validator-move_constraint_test branch June 15, 2016 21:42

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Constraint validator test case to Test namespace #17203

Move Constraint validator test case to Test namespace #17203

wouterj commented Dec 31, 2015

stof Dec 31, 2015

stof Dec 31, 2015

wouterj commented Dec 31, 2015

Tobion commented Jan 12, 2016

nicolas-grekas commented Jan 13, 2016

stof commented Jan 13, 2016

nicolas-grekas Jan 13, 2016

wouterj Jan 25, 2016

fabpot Jan 25, 2016

fabpot Feb 29, 2016

webmozart Mar 2, 2016

wouterj Mar 12, 2016

webmozart Mar 25, 2016

nicolas-grekas commented Jan 13, 2016

stof commented Jan 13, 2016

nicolas-grekas commented Jan 13, 2016

fabpot commented Jan 25, 2016

wouterj commented Jan 25, 2016

fabpot commented Feb 29, 2016

wouterj commented Feb 29, 2016

stof Feb 29, 2016

wouterj commented Feb 29, 2016

xabbuh commented Mar 1, 2016

webmozart Mar 2, 2016

wouterj Mar 10, 2016

fabpot commented Jun 15, 2016

wouterj commented Jun 15, 2016

fabpot commented Jun 15, 2016

Move Constraint validator test case to Test namespace #17203

Move Constraint validator test case to Test namespace #17203

Conversation

wouterj commented Dec 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Dec 31, 2015

Tobion commented Jan 12, 2016

nicolas-grekas commented Jan 13, 2016

stof commented Jan 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 13, 2016

stof commented Jan 13, 2016

nicolas-grekas commented Jan 13, 2016

fabpot commented Jan 25, 2016

wouterj commented Jan 25, 2016

fabpot commented Feb 29, 2016

wouterj commented Feb 29, 2016

Choose a reason for hiding this comment

wouterj commented Feb 29, 2016

xabbuh commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 15, 2016

wouterj commented Jun 15, 2016

fabpot commented Jun 15, 2016