[FrameworkBundle] Dont wire "annotations.cached_reader" before removing passes #21381

Merged
merged 1 commit into from Jan 24, 2017

Projects

None yet

4 participants

@nicolas-grekas
Member
nicolas-grekas commented Jan 23, 2017 edited
Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #21339
License MIT
Doc PR -

This PR basically reverts #20601 and wires "annotations.cached_reader" later, so that any compiler passes needing "annotation_reader" at compile time don't get any cache - anyway, it's useless at compile time.

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone Jan 23, 2017
@fabpot
Member
fabpot commented Jan 23, 2017

Is it possible to some tests? Because I fear we can get regressions on this one.

@nicolas-grekas nicolas-grekas [FrameworkBundle] Dont wire "annotations.cached_reader" before removi…
…ng passes
e59f0e0
@nicolas-grekas
Member

Test added. Without the patch, the test suite fails with many Constructing service "cache.annotations" from a parent definition is not supported at build time.

@fabpot
Member
fabpot commented Jan 24, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit e59f0e0 into symfony:3.2 Jan 24, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot added a commit that referenced this pull request Jan 24, 2017
@fabpot fabpot bug #21381 [FrameworkBundle] Dont wire "annotations.cached_reader" be…
…fore removing passes (nicolas-grekas)

This PR was merged into the 3.2 branch.

Discussion
----------

[FrameworkBundle] Dont wire "annotations.cached_reader" before removing passes

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21339
| License       | MIT
| Doc PR        | -

This PR basically reverts #20601 and wires "annotations.cached_reader" later, so that any compiler passes needing "annotation_reader" at compile time don't get any cache - anyway, it's useless at compile time.

Commits
-------

e59f0e0 [FrameworkBundle] Dont wire "annotations.cached_reader" before removing passes
99d29c0
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:no-annot branch Jan 24, 2017
@stof
Member
stof commented Jan 24, 2017

this is wrong, as it changes the object graph after optimizations, which may change the allowed inlining.

Instead it should stay a pass registered before optimizations, but with a very low priority

@stof
Member
stof commented Jan 24, 2017

in practice, adding more compiler passes after optimization should be quite rare, as changes done in them may break previous optimizations

@nicolas-grekas
Member

Not sure to get what you mean. To me, the graph is computed in removing passes, by AnalyzeServiceReferencesPass, so this won't be an issue.
But if you spotted a broken case, could you please tell me how to reproduce/test it?

@fabpot fabpot added a commit that referenced this pull request Feb 12, 2017
@fabpot fabpot bug #21556 [FrameworkBundle] Wire ArrayCache for annotation reader at…
… bootstrap (nicolas-grekas)

This PR was merged into the 3.2 branch.

Discussion
----------

[FrameworkBundle] Wire ArrayCache for annotation reader at bootstrap

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21549
| License       | MIT
| Doc PR        | -

Related to #21381 which disabled any cache at bootstrap.
Instead, this wires ArrayCache during bootstrapping, then swaps the cache provider for the real one.

Commits
-------

f90f53e [FrameworkBundle] Wire ArrayCache for annotation reader at bootstrap
50be214
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment