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

Allow to autoconfigure validation groups generators #3346

Merged

Conversation

norkunas
Copy link
Contributor

@norkunas norkunas commented Jan 16, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tickets -
License MIT
Doc PR api-platform/docs#1023

When having many validation groups generators for resources, you need to make every generator as a public service, and this doesn't work with _instanceof configuration where you could add a custom marker and make those services public, because symfony doesn't detect that those services are used and removes from container.

This adds ApiPlatform\Core\Validator\ValidationGroupsGeneratorInterface so apps can leverage the autoconfiguration + autowiring of those generators. Generators are collected and registered in a custom service locator, but consumers won't need anymore any configuration for them if will implement the interface. For not breaking backwards compatibility left the full service container as a fallback.

TODO:

  • Add more tests if needed?
  • Make docs PR

@norkunas norkunas requested review from dunglas, soyuka and alanpoulain and removed request for dunglas and soyuka January 16, 2020 06:55
@norkunas norkunas force-pushed the validation-groups-generators-dx branch 4 times, most recently from 5ec89da to a067974 Compare January 16, 2020 07:20
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

nice job, we should add a unit test implementing a validationgroupsgenerator

@norkunas norkunas force-pushed the validation-groups-generators-dx branch from a067974 to 573bb2e Compare January 18, 2020 10:31
@norkunas
Copy link
Contributor Author

Also updated return typehint from generator interface not to be strict and allow return GroupSequence for consistency.

@norkunas norkunas force-pushed the validation-groups-generators-dx branch from 573bb2e to f56d598 Compare January 18, 2020 10:34
@norkunas
Copy link
Contributor Author

ping :)

@norkunas norkunas force-pushed the validation-groups-generators-dx branch 3 times, most recently from b7450e4 to 0a6a90f Compare February 26, 2020 06:54
@norkunas
Copy link
Contributor Author

Updated, but I don't understand what's wrong with the tests. How a single definition can affect another definitions 🤔

@teohhanhui
Copy link
Contributor

Error: Call to a member function setPublic() on null

Looks like there's a missing mock?

@norkunas
Copy link
Contributor Author

Error: Call to a member function setPublic() on null

Looks like there's a missing mock?

Which mock?

@teohhanhui
Copy link
Contributor

teohhanhui commented Feb 26, 2020

Which mock?

The ChildDefinition prophecy must return the same instance on addTag(...), e.g. phpspec/prophecy#190 (comment)

My suggestion is to add:

$this->childDefinitionProphecy->addTag(Argument::type('string'))->willReturn($this->childDefinitionProphecy);

in getBaseContainerBuilderProphecy method.

@norkunas norkunas force-pushed the validation-groups-generators-dx branch from 0a6a90f to d6ac314 Compare February 26, 2020 12:10
@norkunas norkunas force-pushed the validation-groups-generators-dx branch from d6ac314 to f4d76e5 Compare February 26, 2020 12:14
@norkunas
Copy link
Contributor Author

Which mock?

The ChildDefinition prophecy must return the same instance on addTag(...), e.g. phpspec/prophecy#190 (comment)

My suggestion is to add:

$this->childDefinitionProphecy->addTag(Argument::type('string'))->willReturn($this->childDefinitionProphecy);

in getBaseContainerBuilderProphecy method.

Didn't help 🤦‍♂️ 😁

@teohhanhui
Copy link
Contributor

Let me fix this...

@teohhanhui teohhanhui force-pushed the validation-groups-generators-dx branch 5 times, most recently from 62a5f85 to 5a4484c Compare February 26, 2020 16:53
@teohhanhui teohhanhui force-pushed the validation-groups-generators-dx branch from 5a4484c to 08a10f5 Compare February 26, 2020 17:08
Co-authored-by: Teoh Han Hui <teohhanhui@gmail.com>
@teohhanhui teohhanhui force-pushed the validation-groups-generators-dx branch from 08a10f5 to 39fc0a4 Compare March 2, 2020 16:57
@teohhanhui teohhanhui merged commit b5370a0 into api-platform:master Mar 2, 2020
@teohhanhui
Copy link
Contributor

Thanks @norkunas! 🎉 🚀

@norkunas norkunas deleted the validation-groups-generators-dx branch March 3, 2020 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants