Skip to content
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

[3.x] Add validator related deprecation notices #6633

Merged

Conversation

tambait
Copy link
Contributor

@tambait tambait commented Nov 27, 2020

Subject

I am targeting this branch, because BC.

Changelog

### Deprecated

- `AbstractAdmin::$validator` property
- `AdminInterface::getValidator` method specification
- `AdminInterface::setValidator` method specification
- `AbstractAdmin::getValidator` method implementation
- `AbstractAdmin::setValidator` method implementation
- `AddDependencyCallsCompilerPass` - validator related keys
- `DependencyInjection/Configuration` - validator related config option


@tambait tambait force-pushed the validator-add-deprecation-notices branch from 6b18d22 to 02784c1 Compare November 27, 2020 12:45
src/Admin/AbstractAdmin.php Show resolved Hide resolved
src/Admin/AbstractAdmin.php Show resolved Hide resolved
public function setValidator($validator)
{
@trigger_error(sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

This method is automatically called by the CompilerPass, so I think we can't trigger an error inside (or everybody will have a deprecation everytime). @sonata-project/contributors ?

@tambait tambait force-pushed the validator-add-deprecation-notices branch 4 times, most recently from 95a5692 to b6e0708 Compare November 27, 2020 14:01
VincentLanglet
VincentLanglet previously approved these changes Nov 27, 2020
@VincentLanglet
Copy link
Member

Didn't see the tests ; do you know how to fix them ?

  • For the setValidator, I think you should remove the trigger_error
  • For the getValidator, Use @group legacy, NEXT_MAJOR: remove this test and add $this->expectDeprecation

@tambait tambait force-pushed the validator-add-deprecation-notices branch 8 times, most recently from e8f916f to c6b040b Compare November 27, 2020 16:53
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

One last thing, you have to update the UPGRADE note file, and the PR will be perfect

@tambait
Copy link
Contributor Author

tambait commented Nov 28, 2020

Didn't see the tests ; do you know how to fix them ?

Thanks. I wouldn't know how to fix tests without your help.

@tambait tambait force-pushed the validator-add-deprecation-notices branch from c6b040b to 6c3dbcc Compare November 28, 2020 11:35
VincentLanglet
VincentLanglet previously approved these changes Nov 28, 2020
@VincentLanglet VincentLanglet requested a review from a team November 28, 2020 11:36
@VincentLanglet VincentLanglet marked this pull request as ready for review November 28, 2020 11:36
@tambait tambait force-pushed the validator-add-deprecation-notices branch from 6c3dbcc to f71e220 Compare November 28, 2020 11:38
VincentLanglet
VincentLanglet previously approved these changes Nov 28, 2020
Copy link
Member

@franmomu franmomu left a comment

Choose a reason for hiding this comment

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

I think there is also this one:

$validator = $this->createMock(ValidatorInterface::class);
$validator->method('getMetadataFor')->willReturn($this->createStub(MemberMetadata::class));
$admin->setValidator($validator);

UPGRADE-3.x.md Show resolved Hide resolved
src/Admin/AbstractAdmin.php Show resolved Hide resolved
src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
@tambait tambait force-pushed the validator-add-deprecation-notices branch 4 times, most recently from 7912b82 to 5296b8f Compare December 5, 2020 13:05
@tambait tambait force-pushed the validator-add-deprecation-notices branch from 5296b8f to c143ddf Compare December 5, 2020 13:23
VincentLanglet
VincentLanglet previously approved these changes Dec 5, 2020
@SonataCI
Copy link
Collaborator

SonataCI commented Dec 5, 2020

Could you please rebase your PR and fix merge conflicts?

VincentLanglet
VincentLanglet previously approved these changes Dec 5, 2020
@VincentLanglet VincentLanglet requested a review from a team December 5, 2020 14:21
franmomu
franmomu previously approved these changes Dec 5, 2020
@franmomu franmomu added the minor label Dec 5, 2020
@franmomu franmomu mentioned this pull request Dec 5, 2020
@SonataCI
Copy link
Collaborator

SonataCI commented Dec 5, 2020

Could you please rebase your PR and fix merge conflicts?

@tambait tambait dismissed stale reviews from franmomu and VincentLanglet via 89ea0e1 December 5, 2020 17:07
@VincentLanglet VincentLanglet merged commit a4829f3 into sonata-project:3.x Dec 5, 2020
@tambait tambait deleted the validator-add-deprecation-notices branch December 6, 2020 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants