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

[Admin] Remove validation related methods #6617

Closed

Conversation

tambait
Copy link
Contributor

@tambait tambait commented Nov 22, 2020

Subject

Closes #6233

Changelog

### Changed
- Changed `AbstractAdmin::buildForm()` removed line with call to `addEventListener()` which used `preValidate()`

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

@tambait tambait force-pushed the remove-validation-methods branch from 0f0e90f to 70666f2 Compare November 22, 2020 14:11
@VincentLanglet
Copy link
Member

@tambait In order to reduce conflict, you should always wait for the merge in 3.x, then for the 3.x merge into master before doing the PR in master

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@tambait tambait force-pushed the remove-validation-methods branch from c5da28f to 66a1fd1 Compare November 24, 2020 14:41
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@tambait tambait marked this pull request as ready for review November 26, 2020 12:09
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.

I would say you can also remove

  • The property protected $validator; of the AbstractAdmin
  • The set/getValidator method of the AbstractAdmin and the interface
  • All the 4 class_exists(\Sonata\Form\Validator\ErrorElement::class); occurences
  • ->scalarNode('validator')->defaultNull()->end() in DependencyInjection/Configuration
  • All the validator occurence in the doc
  • All the 'validator' occurence in AddDependencyCallsCompilerPass

@tambait tambait marked this pull request as draft November 27, 2020 11:20
@tambait
Copy link
Contributor Author

tambait commented Nov 27, 2020

* All the 4 class_exists(\Sonata\Form\Validator\ErrorElement::class); occurences

I don't see these appearing in the master branch only in 3.x, so should I just add notice in 3.x for this to be removed in next major?

Also, should Command\Validators be removed or modified in any way?

@VincentLanglet
Copy link
Member

I don't see these appearing in the master branch only in 3.x, so should I just add notice in 3.x for this to be removed in next major?

If it's already removed, nothing to do ;)

Also, should Command\Validators be removed or modified in any way?

I don't think so. These validators are used for the commands, not the admin.

@SonataCI
Copy link
Collaborator

SonataCI commented Dec 4, 2020

Could you please rebase your PR and fix merge conflicts?

@franmomu
Copy link
Member

franmomu commented Dec 4, 2020

Sorry @tambait 🙏 I didn't see this PR before creating #6644, I just wanted to remove a comment that should be there and I ended up removing other NEXT_MAJOR comments because there were fails with phpstan.

@VincentLanglet
Copy link
Member

Sorry @tambait 🙏 I didn't see this PR before creating #6644, I just wanted to remove a comment that should be there and I ended up removing other NEXT_MAJOR comments because there were fails with phpstan.

Is there still something to keep in this PR ?

@tambait
Copy link
Contributor Author

tambait commented Dec 4, 2020

Sorry @tambait pray I didn't see this PR before creating #6644, I just wanted to remove a comment that should be there and I ended up removing other NEXT_MAJOR comments because there were fails with phpstan.

Is there still something to keep in this PR ?

@VincentLanglet
I switched from working on this PR because there were things to be done on related PR #6633 on 3.x branch

@VincentLanglet
Copy link
Member

Oh yes indeed, let's ask @franmomu A review for your PR introducing deprecations :p

@tambait tambait deleted the remove-validation-methods branch December 6, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants