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

[FrameworkBundle] Move compiler passes to their component #21284

Closed
chalasr opened this issue Jan 13, 2017 · 10 comments

Comments

Projects
None yet
6 participants
@chalasr
Copy link
Member

commented Jan 13, 2017

Q A
Bug report? no
Feature request? no
BC Break report? no
RFC? yes
Symfony version 3.3

Helps when using them independently with the DI component (also giving it visibility).
The most part are unlikely to be extended as is, they can be deprecated/moved in 3.3 (wired only if the component is installed) and removed from the framework in 4.0.

  • AddConsoleCommandPass => Console (#19443)
  • FormPass => Form (#21283)
  • SerializerPass => Serializer (#21293)
  • ConfigCachePass => Config (#21375)
  • RoutingResolverPass => Routing (#21835)
  • PropertyInfoPass => PropertyInfo (#21806)
  • ControllerArgumentValueResolverPass => HttpKernel (#21815)
  • AddConstraintsValidatorPass, AddValidatorInitializersPass => Validator (#22081)
  • ValidateWorkflowsPass => Workflow (#22313)
  • AddCacheWarmerPass, AddCacheClearerPass => HttpKernel (#22620)
  • TranslationDumperPass, TranslationExtractorPass, TranslatorPass => Translation (#22619)
@dunglas

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

It has already been discussed several time and I think most people agree that it's a good idea. It's in the spirit of what we try to do with 3.3 too. 👍 for me.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

👍 it makes a lot of sense to me and I can't see any drawback so far.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2017

See also #20250 where the approach is to make special names configurable (imo. this is nice for the component part of it).

However the console passes were merged anyway (#19443 (comment)), so im curious about the general approach here..

Not doing it, means the pass is pretty much useless for people wanting to do different naming. Which raises the question; is it worth it?

Besides, the console pass practically only sets console.command.ids as a parameter; is it really component worthy?

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

I think it makes sense. Let's do it.

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2017

@fabpot See #21283 for the Form and #21293 for the Serializer.

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2017

@ro0NL Outside of the special name handling, having passes in the components make the DI component and the whole tagging feature discoverable/available when using the component standalone.

About the special name handling, the feature could be added in a next time (sure, not passed through the constructor), moving passes will at least give the idea of using the DI component for achieving such needs if the built-in passes do not fit, btw avoiding registering passes when components are not there. So yes, I do think it's worth it.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2017

👍 for as is moving to the components... just mentioning different approaches already :)

There was also discussion about some generic approach, i can imagine we can simplify some passes with a

new DI\TagCollectingPass('target.def', ['tagA', 'tagB'], function (Def $targetDef, Def $taggedDef, array $tagAttr) {
   $targetDef->addMethodCall('addTagAorB', $taggedDef);
});

Perhaps something to look into.

@Simperfit

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2017

👍

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

@ro0NL Applied your suggestion to make service id/tag names configurable through the constructor in #21283 and #21293

fabpot added a commit that referenced this issue Feb 16, 2017

feature #21293 [FrameworkBundle][Serializer] Move SerializerPass to t…
…he Serializer (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[FrameworkBundle][Serializer] Move SerializerPass to the Serializer

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Part of #21284

Commits
-------

95cf508 [FrameworkBundle][Serializer] Move SerializerPass to the Serializer

fabpot added a commit that referenced this issue Feb 16, 2017

feature #21283 [Form][FrameworkBundle] Move FormPass to the Form comp…
…onent (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Form][FrameworkBundle] Move FormPass to the Form component

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

So that anyone using only Form and DI can use it for registering form types/type guessers.
Follows #19443, related to #21284

Commits
-------

e68a6d9 [FrameworkBundle][Form] Move FormPass to the Form component
@chalasr

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

@fabpot I'm going to propose the remaining passes and I wonder: would you prefer one huge PR or several ones?

edit: opted for several ones.

fabpot added a commit that referenced this issue Feb 28, 2017

feature #21375 [FrameworkBundle][Config] Move ConfigCachePass from Fr…
…ameworkBundle to Config (Deamon)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[FrameworkBundle][Config] Move ConfigCachePass from FrameworkBundle to Config

| Q             | A
| ------------- | ---
| Branch?       | master<!--see comment below-->
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes/no
| Fixed tickets | - <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | <!--highly recommended for new features-->

This MR is part of the #21284 todo list
<!--
- Bug fixes must be submitted against the lowest branch where they apply
  (lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the master branch.
- Please fill in this template according to the PR you're about to submit.
- Replace this comment by a description of what your PR is solving.
-->

Commits
-------

bce445f Move ConfigCachePass from FrameworkBundle to Config

fabpot added a commit that referenced this issue Apr 14, 2017

feature #22313 [Workflow] Move ValidateWorkflowsPass to the Workflow …
…component (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Workflow] Move ValidateWorkflowsPass to the Workflow component

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Continues #21284

Commits
-------

8fe122f Move ValidateWorkflowsPass to the Workflow component

nicolas-grekas added a commit that referenced this issue May 21, 2017

feature #22749 Remove deprecated container injections and compiler pa…
…sses (chalasr)

This PR was merged into the 4.0-dev branch.

Discussion
----------

Remove deprecated container injections and compiler passes

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~~#21451 #21625 #21284 #22010~~ #22805
| License       | MIT
| Doc PR        | n/a

Commits
-------

16a2fcf Remove deprecated container injections and compiler passes

fabpot added a commit that referenced this issue Jul 6, 2017

feature #22620 [FrameworkBundle][HttpKernel] Move httpkernel pass (le…
…piaf)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle][HttpKernel] Move httpkernel pass

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | part of #21284
| License       | MIT
| Doc PR        | n/a

Move addcachearmer, addcacheclearer compiler pass to httpkernel

Commits
-------

83727c7 [FrameworkBundle][HttpKernel] Move addcachearmer, addcacheclearer compiler pass

@fabpot fabpot closed this Jul 6, 2017

fabpot added a commit that referenced this issue Jul 6, 2017

feature #22619 [FrameworkBundle][Translation] Move translation compil…
…er pass (lepiaf)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle][Translation] Move translation compiler pass

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | part of #21284
| License       | MIT
| Doc PR        | n/a

move TranslationDumperPass, TranslationExtractorPass, TranslatorPass to Translation component.

Commits
-------

74c951f [FrameworkBundle][Translation] Move translation compiler pass

@chalasr chalasr changed the title [FrameworkBundle] Move compiler passes in their component [FrameworkBundle] Move compiler passes to their component Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.