Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Service constructor arguments validating #39678

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Service constructor arguments validating #39678

wants to merge 1 commit into from

Conversation

fu22ybear
Copy link
Contributor

@fu22ybear fu22ybear commented Jan 1, 2021 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #31882
License MIT
Doc PR n/a

proof of concept, caused by #31882.

these changes give the possibility to validate services constructor arguments using Validation component constraints.
validation implemented in ValidateConstructorArgumentsPass() and runs with optimizationPasses, but could be moved to cli-command like CheckTypeDeclarationsPass (#27825).

constraints definition example for .yaml files, .xml and .php files are skipped because the implementation has not been approved yet.

    App\Service\Service:
        arguments:
            $int: 1
            $array: [1, 2, 3]
            $email: 'test@email.com'
            $datetime: '2020-12-31 23:59:59'
            $ipAddresses: ['8.8.4.4', '8.8.8.8']
            $noConstraints: 'no constraints for this argument'
        constraints:
            $int:
                EqualTo: 1
            $array:
                Count:
                    min: 1
                    max: 5
            $email:
                Email: ~
            $datetime:
                NotBlank: ~
                DateTime: ~
            $ipAddresses:
                All:
                    NotBlank: ~
                    Ip:
                        version: !php/const Symfony\Component\Validator\Constraints\Ip::V4_ONLY_PUBLIC

@carsonbot
Copy link

Hey!

I did a quick review of this PR, I think most things looks good.

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@fu22ybear fu22ybear force-pushed the issue-31882-validate-constructor-argument branch from 76ec3b8 to cbdf2b2 Compare January 1, 2021 12:37
@fu22ybear fu22ybear marked this pull request as ready for review January 1, 2021 12:47
@nicolas-grekas nicolas-grekas added this to the 5.x milestone Jan 1, 2021
OskarStark
OskarStark reviewed Jan 2, 2021
View reviewed changes
...ymfony/Component/DependencyInjection/Tests/Compiler/ValidateConstructorArgumentsPassTest.php
'$email' => 'test@email.com',
'$datetime' => '2020-12-31 23:59:59',
'$ipAddresses' => ['8.8.4.4', '8.8.8.8'],
'$noConstraints' => 'no constraints for this argument',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OskarStark, sorry, didn't understand what do you mean

@nicolas-grekas
Copy link
Member

Hi, thanks for working on this.

I'm sorry but I'm 👎 with this approach: this adds complexity and adds some coupling with the validator component.

I don't think this code infrastructure is worth the maintenance effort.
Today, I would go as far as rejecting the linked issue.

Another reason is that validating arguments at compile time conflicts with env vars.
Also: the constraints could give the illusion that they are enforced somehow at runtime.
This isn't the case obviously.

@fu22ybear
Copy link
Contributor Author

@nicolas-grekas, hi.

Thanks for the detailed answer.
Basically, I agree with arguments and I'm ok with closing PR if linked issue will be rejected. Otherwise, I would try another implementation

@nicolas-grekas nicolas-grekas mentioned this pull request Jan 6, 2021
Closed
@nicolas-grekas
Copy link
Member

Thanks for your understanding, closing here for now, let's discuss on the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

Assignees
No one assigned
Labels
Feature Status: Needs Review
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

[DI] Validate constructor arguments
4 participants
@fu22ybear @carsonbot @nicolas-grekas @OskarStark
Morty Proxy This is a proxified and sanitized view of the page, visit original site.