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 🎆

carsonbot added the Status: Needs Review label

Dec 31, 2015

stof reviewed

Dec 31, 2015

View reviewed changes

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

Copy link

Member

stofDec 31, 2015

Choose a reason for hiding this comment

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

missing silencing

Copy link

Member

stofDec 31, 2015

Choose a reason for hiding this comment

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

and it cannot be removed in 3.0...

wouterj force-pushed thevalidator-move_constraint_test branch from30ea9a3 tocbf1f77Compare

December 31, 2015 17:29

Copy link

MemberAuthor

wouterj commentedDec 31, 2015

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

Copy link

Contributor

Tobion commentedJan 12, 2016

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

Copy link

Member

nicolas-grekas commentedJan 13, 2016

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

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jan 13, 2016

Copy link

Member

stof commentedJan 13, 2016

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 reviewed

Jan 13, 2016

View reviewed changes

src/Symfony/Bridge/Doctrine/composer.json Outdated

Copy link

Member

nicolas-grekasJan 13, 2016

Choose a reason for hiding this comment

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

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)

Copy link

MemberAuthor

wouterjJan 25, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

fabpotJan 25, 2016

Choose a reason for hiding this comment

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

ping@nicolas-grekas

Copy link

Member

fabpotFeb 29, 2016

Choose a reason for hiding this comment

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

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

Copy link

Contributor

webmozartMar 2, 2016

Choose a reason for hiding this comment

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

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.

Copy link

MemberAuthor

wouterjMar 12, 2016

Choose a reason for hiding this comment

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

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?

Copy link

Contributor

webmozartMar 25, 2016

Choose a reason for hiding this comment

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

I think that would be good, yes.

Copy link

Member

nicolas-grekas commentedJan 13, 2016

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

Copy link

Member

stof commentedJan 13, 2016

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

Copy link

Member

nicolas-grekas commentedJan 13, 2016

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

Copy link

Member

fabpot commentedJan 25, 2016

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

wouterj force-pushed thevalidator-move_constraint_test branch fromcbf1f77 to5c516d8Compare

January 25, 2016 14:10

Copy link

MemberAuthor

wouterj commentedJan 25, 2016

Alright, removed the class and file.

Copy link

Member

fabpot commentedFeb 29, 2016

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

wouterj force-pushed thevalidator-move_constraint_test branch from5c516d8 to5eeb754Compare

February 29, 2016 19:14

Copy link

MemberAuthor

wouterj commentedFeb 29, 2016

Rebased

Status: Needs review

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 Outdated

		UPGRADE FROM 3.0 to 3.1
		=======================

		<<<<<<< HEAD

Copy link

Member

stofFeb 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 thevalidator-move_constraint_test branch from5eeb754 tod4ff6ffCompare

February 29, 2016 20:04

Copy link

MemberAuthor

wouterj commentedFeb 29, 2016

Thanks, fixed your comments@stof.

Copy link

Member

xabbuh commentedMar 1, 2016

This seems to need a rebase again.

webmozart reviewed

Mar 2, 2016

View reviewed changes

UPGRADE-3.1.md Outdated

		// ...
		use Symfony\Component\Validator\Test\ConstraintValidatorTestCase;

		class MyCustomValidatorTest extends ConstraintValidatorTestCase

Copy link

Contributor

webmozartMar 2, 2016

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

wouterjMar 10, 2016

Choose a reason for hiding this comment

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

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

javiereguiluz added Validator Enhancement labels

Mar 3, 2016

javiereguiluz added the Deprecation label

Mar 7, 2016

Copy link

Member

fabpot commentedJun 15, 2016

@wouterj IIUC, it would be better to keepAbstractConstraintValidatorTest.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 force-pushed thevalidator-move_constraint_test branch fromd4ff6ff toed33a2dCompare

June 15, 2016 09:42

Copy link

MemberAuthor

wouterj commentedJun 15, 2016

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.

wouterj force-pushed thevalidator-move_constraint_test branch fromed33a2d to0e868eeCompare

June 15, 2016 09:44

Move Constraint validator test case to Test namespace

e938361

wouterj force-pushed thevalidator-move_constraint_test branch from0e868ee toe938361Compare

June 15, 2016 14:32

Copy link

Member

fabpot commentedJun 15, 2016

Thank you@wouterj.

fabpot merged commite938361 intosymfony:master

Jun 15, 2016

fabpot added a commit that referenced this pull request

Jun 15, 2016

feature#17203Move Constraint validator test case to Test namespace …

ba34459

…(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

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

Labels

Deprecation Enhancement Status: Needs Review Validator

Movatterモバイル変換

Uh oh!

Move Constraint validator test case to Test namespace#17203

Move Constraint validator test case to Test namespace#17203

Uh oh!

Conversation

wouterj commentedDec 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commentedDec 31, 2015

Uh oh!

Tobion commentedJan 12, 2016

Uh oh!

nicolas-grekas commentedJan 13, 2016

Uh oh!

stof commentedJan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedJan 13, 2016

Uh oh!

stof commentedJan 13, 2016

Uh oh!

nicolas-grekas commentedJan 13, 2016

Uh oh!

fabpot commentedJan 25, 2016

Uh oh!

wouterj commentedJan 25, 2016

Uh oh!

fabpot commentedFeb 29, 2016

Uh oh!

wouterj commentedFeb 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commentedFeb 29, 2016

Uh oh!

xabbuh commentedMar 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedJun 15, 2016

Uh oh!

wouterj commentedJun 15, 2016

Uh oh!

fabpot commentedJun 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants