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

[HttpKernel] Allow bundles to declare classes and annotated classes to compile using patterns #19205

Merged
merged 1 commit into from Jul 30, 2016

Conversation

Projects
None yet
6 participants
@tgalopin
Contributor

tgalopin commented Jun 28, 2016

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

This PR introduces a simple system of patterns based on wildcards for classes to cache in the HttpKernel dependency injections extensions. This system started to be implemented in #18533 but I split it up here to use it also in the classes to compile.

return $regexps;
}
private function matchAnyRegexp($class, $regexps)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 28, 2016

Member

Should be matchAnyRegexps (missing "s")

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 28, 2016

I've been working on this with @tgalopin, so here are a few more thoughts:

Currently, bundles can use addClassesToCompile to declare which FQCN should be inlined for fast bootstrapping.
This PR extends this mechanism in two ways: bundle can now declare the same using wildcards. FQCN discovery is done against composer class maps so that only composer-dumped classes can match against the patterns. **Test** classes are blacklisted by default, except when a pattern explicitly whitelists them again. Patterns that contain no wildcards at all are used directly without checking any class maps.

The second feature is adding a way for bundles to declare which classes have annotations that are going to be used in the app. This is useful for warming up annotation reader caches, but also makes it possible to inline classes that have annotations on them (and thus add the Controller class back in the list of classes to compile).

The linked PR uses these to warm up the annotations cache.

@nicolas-grekas nicolas-grekas changed the title from [HttpKernel] Create ClassMatcher to use patterns in classes and annotations to cache to [HttpKernel] Allow bundles to declare annotated classes to cache using patterns Jun 29, 2016

@nicolas-grekas nicolas-grekas changed the title from [HttpKernel] Allow bundles to declare annotated classes to cache using patterns to [HttpKernel] Allow bundles to declare annotated classes to compile using patterns Jun 29, 2016

*/
public function addClassesToCompile(array $classes)
public function addClassesToCompile(array $classes, array $annotatedClasses = array())

This comment has been minimized.

@stof

stof Jun 29, 2016

Member

rather than adding a second argument, I suggest adding a new method. It will make the code more readable

@stof

This comment has been minimized.

Member

stof commented Jun 29, 2016

I don't see any code reading the annotations.map file. Is there something missing in the diff ?

@tgalopin

This comment has been minimized.

Contributor

tgalopin commented Jun 29, 2016

@stof : The idea is to change the behavior of classes to compile and introduce the annotations.map file here and use it in the #18533 PR afterwards.

@tgalopin

This comment has been minimized.

Contributor

tgalopin commented Jun 29, 2016

I did the changes.

@stof

This comment has been minimized.

Member

stof commented Jun 29, 2016

@tgalopin then it is good to explain it, so that people reviewing the code can understand it.

and this makes me tell than the split in multiple PRs is not good, as this PR introduces dead features. This PR should probably deal only with compiled classes

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 29, 2016

I think that this really belong to its own PR: the generated annotations.map file is open to any use cases.
The same is btw for the classes.map. Tightening this with the annotation cache warmer sends the wrong signal. We're not creating an internal detail but a public artifact that people can use as they want.
Thus dedicated PR, changelog entry, etc.

@tgalopin tgalopin changed the title from [HttpKernel] Allow bundles to declare annotated classes to compile using patterns to [HttpKernel] Allow bundles to declare classes and annotated classes to compile using patterns Jul 26, 2016

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jul 28, 2016

👍

'**Bundle\\Controller\\',
'**Bundle\\Entity\\',
// Added explicitly so that we dont rely on the class map being dumped to make it work

This comment has been minimized.

@javiereguiluz

javiereguiluz Jul 28, 2016

Member

dont -> don't

This comment has been minimized.

@tgalopin

tgalopin Jul 28, 2016

Contributor

Fixed, thanks :) .

@fabpot

This comment has been minimized.

Member

fabpot commented Jul 30, 2016

Thank you @tgalopin.

@fabpot fabpot merged commit 1be7424 into symfony:master Jul 30, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jul 30, 2016

feature #19205 [HttpKernel] Allow bundles to declare classes and anno…
…tated classes to compile using patterns (tgalopin)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[HttpKernel] Allow bundles to declare classes and annotated classes to compile using patterns

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

This PR introduces a simple system of patterns based on wildcards for classes to cache in the HttpKernel dependency injections extensions. This system started to be implemented in #18533 but I split it up here to use it also in the classes to compile.

Commits
-------

1be7424 [HttpKernel] Allow usage of patterns in classes and annotations to cache

tgalopin added a commit to tgalopin/symfony that referenced this pull request Aug 1, 2016

feature symfony#19205 [HttpKernel] Allow bundles to declare classes a…
…nd annotated classes to compile using patterns (tgalopin)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[HttpKernel] Allow bundles to declare classes and annotated classes to compile using patterns

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

This PR introduces a simple system of patterns based on wildcards for classes to cache in the HttpKernel dependency injections extensions. This system started to be implemented in symfony#18533 but I split it up here to use it also in the classes to compile.

Commits
-------

1be7424 [HttpKernel] Allow usage of patterns in classes and annotations to cache

tgalopin added a commit to tgalopin/symfony that referenced this pull request Aug 2, 2016

feature symfony#19205 [HttpKernel] Allow bundles to declare classes a…
…nd annotated classes to compile using patterns (tgalopin)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[HttpKernel] Allow bundles to declare classes and annotated classes to compile using patterns

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

This PR introduces a simple system of patterns based on wildcards for classes to cache in the HttpKernel dependency injections extensions. This system started to be implemented in symfony#18533 but I split it up here to use it also in the classes to compile.

Commits
-------

1be7424 [HttpKernel] Allow usage of patterns in classes and annotations to cache

cedricziel added a commit to cedricziel/VichUploaderBundle that referenced this pull request Aug 8, 2016

Defer RegisterPropelModelsPass
The service 'cache.annotations', which is resolved when the metadata
reader is received from the container in the Pass, is not available
at that point in the container compilation process.

This changeset defers the PropelModelsPass until a point in time, where the
container is fully accessible.

Ref: symfony/symfony#19205

@fabpot fabpot referenced this pull request Oct 27, 2016

Merged

Release v3.2.0-BETA1 #20317

cedricziel added a commit to cedricziel/VichUploaderBundle that referenced this pull request Jan 17, 2017

Defer RegisterPropelModelsPass
The service 'cache.annotations', which is resolved when the metadata
reader is received from the container in the Pass, is not available
at that point in the container compilation process.

This changeset defers the PropelModelsPass until a point in time, where the
container is fully accessible.

Ref: symfony/symfony#19205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment