Skip to content

[FrameworkBundle] Fix auto-discovering validator constraints #49850

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
Merged
nicolas-grekas merged 1 commit into from
Mar 30, 2023
Merged

[FrameworkBundle] Fix auto-discovering validator constraints #49850

nicolas-grekas merged 1 commit into from
Mar 30, 2023

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 28, 2023 •
edited
Loading

Q A
Branch? 6.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets #49397
License MIT
Doc PR -

Alternative to #49785

Note that this kind of container-assisted auto-discovery of non-service classes looks like an interesting pattern for many other subsystems. /cc @symfony/mergers @mtarld

@carsonbot carsonbot added this to the 6.3 milestone Mar 28, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 28, 2023
Closed
@mtarld
Copy link
Contributor

mtarld commented Mar 28, 2023

Indeed, it could be great to use this pattern for internal stuff that doesn't need to be manipulated by the container such as form types, or even to load user land stuff like entities, serializable items, API resources, etc...

nfragnet
nfragnet reviewed Mar 29, 2023
View reviewed changes
Copy link

@nfragnet nfragnet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this is indeed the way to go

A solution for the edge case UniqueEntity?

As it's in the doctrine bridge and the validator is not a service, this one does not get extraction.

Also: I see that you chose 6.3 branch: does that mean that 6.2 will stay with translation extraction not working as expected?

@chalasr
Copy link
Member

chalasr commented Mar 29, 2023

See #49785 (comment)

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.2 Mar 29, 2023
@nicolas-grekas nicolas-grekas changed the base branch from 6.3 to 6.2 March 29, 2023 13:05
@nicolas-grekas nicolas-grekas force-pushed the validator-discovery branch 2 times, most recently from bcc8dca to f9ac1e3 Compare March 29, 2023 13:40
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 29, 2023

PR updated to target 6.2

A solution for the edge case UniqueEntity? As it's in the doctrine bridge and the validator is not a service, this one does not get extraction.

To me, the corresponding validator is a service, so there is nothing to do.

@stof
Copy link
Member

stof commented Mar 29, 2023

Reading this PR makes me realize that findTaggedServiceIds does not filter out definitions tagged as being excluded. This means that our change that creates excluded definition instead of skipping the definition of services is actually a BC break for all bundles that collect tagged services to inject them somewhere (the primary usage of this method) as it would make them inject excluded services into their dependencies while they were not injected before.

Including excluded services should be opt-in IMO.

And btw, we should keep throwing in case of non-excluded abstract services being tagged as constraint validators, to provide a better DX (which is why the check was used in the first place). Removing this check in your PR in favor of silently skipping them decreases DX.

@nicolas-grekas nicolas-grekas mentioned this pull request Mar 30, 2023
Merged
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 30, 2023

PR updated, needs #49867

@nicolas-grekas nicolas-grekas mentioned this pull request Mar 30, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Mar 30, 2023
@nicolas-grekas
bug #49867 [DependencyInjection] Filter "container.excluded" services…
3b9fa3c 
… when using `findTaggedServiceIds()` (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[DependencyInjection] Filter "container.excluded" services when using `findTaggedServiceIds()`

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

As spotted by `@stof` in #49850 (comment)

Commits
-------

cf3d67e [DependencyInjection] Filter "container.excluded" services when using `findTaggedServiceIds()`
@nicolas-grekas nicolas-grekas force-pushed the validator-discovery branch 2 times, most recently from d84f845 to c05e655 Compare March 30, 2023 12:40
@nicolas-grekas nicolas-grekas merged commit ad2b68b into symfony:6.2 Mar 30, 2023
@nicolas-grekas nicolas-grekas deleted the validator-discovery branch March 30, 2023 13:20
@marien-probesys marien-probesys mentioned this pull request Mar 30, 2023
Closed
@fabpot fabpot mentioned this pull request Mar 31, 2023
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 1, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Apr 1, 2023
@nicolas-grekas
bug #49889 [FrameworkBundle] Fix registering ExpressionValidator (nic…
1753ad9 
…olas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[FrameworkBundle] Fix registering ExpressionValidator

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #49886
| License       | MIT
| Doc PR        | -

Introduced in #49850, my bad.

Commits
-------

e11de1f [FrameworkBundle] Fix registering ExpressionValidator
@AymLaf AymLaf mentioned this pull request Apr 3, 2023
Closed
@xabbuh xabbuh mentioned this pull request Apr 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof stof approved these changes

@welcoMattic welcoMattic welcoMattic approved these changes

@mtarld mtarld mtarld approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh

@lyrixx lyrixx Awaiting requested review from lyrixx

@dunglas dunglas Awaiting requested review from dunglas

@yceruto yceruto Awaiting requested review from yceruto

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr

@OskarStark OskarStark Awaiting requested review from OskarStark

@jderusse jderusse Awaiting requested review from jderusse

+1 more reviewer

@nfragnet nfragnet nfragnet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug FrameworkBundle Status: Reviewed

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

7 participants

@nicolas-grekas @mtarld @chalasr @stof @welcoMattic @nfragnet @carsonbot