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 deprecation notices related to validate methods #6618

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

tambait
Copy link
Contributor

@tambait tambait commented Nov 22, 2020

Subject

I am targeting this branch, because BC.

Closes #6233

Changelog

### Deprecated
- `AdminInterface::validate` method specification
- `AbstractAdmin:validate` method implementation
- `AbstractAdmin::attachInlineValidator()` method
- `AdminExtensionInterface::validate()` method specification
- `AbstractAdminExtension::validate()` method implementation

@tambait tambait force-pushed the add-depreciation-notice branch 6 times, most recently from ca84630 to 71f528b Compare November 22, 2020 19:14
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.

Can you add consign for the master branch ?

We generally add NEXT_MAJOR comment for this.

For instance

/**
     * Attach the inline validator to the model metadata, this must be done once per admin.
     *
     * NEXT_MAJOR: Remove this method.
     *
     * @deprecated since sonata-admin/admin-bundle 3.81
     */
    protected function attachInlineValidator()

Same can be done for validate, prevalidate, ... (all the methods you're deprecating).
And then, I think the PR will be ready.

@tambait
Copy link
Contributor Author

tambait commented Nov 22, 2020

Same can be done for validate, prevalidate, ... (all the methods you're deprecating). And then, I think the PR will be ready.

Thanks. I wanted to ask about prevalidate should it be depreciated also, I saw it only now. Its used in buildForm method in AbstractAdmin.

@tambait tambait force-pushed the add-depreciation-notice branch 2 times, most recently from 5a8407e to f6d2e82 Compare November 22, 2020 20:07
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.

You should also deprecate AbstractAdmin::validate

And AbstractAdmin::preValidate I think (and remove it from LifecycleHookProviderInterface.)

@tambait tambait force-pushed the add-depreciation-notice branch 2 times, most recently from 2419837 to 217caa5 Compare November 23, 2020 12:02
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.

The phpstan errors are normal ; I recommend to ignore them with a @phpstan-ignore-next-line

@tambait tambait marked this pull request as ready for review November 23, 2020 13:06
@VincentLanglet VincentLanglet requested a review from a team November 23, 2020 13:20
@core23 core23 changed the title [3.x] Add depreciation notices related to validate methods [3.x] Add deprecation notices related to validate methods Nov 23, 2020
@VincentLanglet VincentLanglet merged commit eddec26 into sonata-project:3.x Nov 23, 2020
@VincentLanglet
Copy link
Member

Thanks tambait

@tambait tambait deleted the add-depreciation-notice branch November 23, 2020 16:15
franmomu added a commit to franmomu/SonataAdminBundle that referenced this pull request Nov 23, 2020
They were missing in sonata-project#6618
and also the deprecated version has been set to 3.x since there
has not been a release yet.
@franmomu
Copy link
Member

@tambait thanks for your contribution!

Since in this PR we're introducing some deprecations, can you please update the issue description adding a changelog with these deprecations (like #6510 for example)?

I've created #6621 to fix some comments.

@tambait
Copy link
Contributor Author

tambait commented Nov 23, 2020

@franmomu

Since in this PR we're introducing some deprecations, can you please update the issue description adding a changelog with these deprecations (like #6510 for example)?

I've created #6621 to fix some comments.

Thanks. Does it make sense to list both method specification and implementation when they are in different classes like I did?

@franmomu
Copy link
Member

Thanks, I think it does, for some reason we can remove a method from the specification but leave it in the implementation, I prefer being explicit.

VincentLanglet pushed a commit that referenced this pull request Nov 24, 2020
They were missing in #6618
and also the deprecated version has been set to 3.x since there
has not been a release yet.
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.

[NEXT_MAJOR] Remove AbstractAdmin::validate method and all validation-related methods.
5 participants