Posted by dawehner on
Problem/Motivation
If you look at the following services definitions you will see that there are quite of them which don't need to be public:
asset.css.collection_optimizer:
class: Drupal\Core\Asset\CssCollectionOptimizer
arguments: [ '@asset.css.collection_grouper', '@asset.css.optimizer', '@asset.css.dumper', '@state' ]
asset.css.optimizer:
class: Drupal\Core\Asset\CssOptimizer
asset.css.collection_grouper:
class: Drupal\Core\Asset\CssCollectionGrouper
asset.css.dumper:
class: Drupal\Core\Asset\AssetDumper
asset.js.collection_renderer:
class: Drupal\Core\Asset\JsCollectionRenderer
arguments: [ '@state' ]
asset.js.collection_optimizer:
class: Drupal\Core\Asset\JsCollectionOptimizer
arguments: [ '@asset.js.collection_grouper', '@asset.js.optimizer', '@asset.js.dumper', '@state' ]
asset.js.optimizer:
class: Drupal\Core\Asset\JsOptimizer
asset.js.collection_grouper:
class: Drupal\Core\Asset\JsCollectionGrouper
asset.js.dumper:
class: Drupal\Core\Asset\AssetDumper
library.discovery:
class: Drupal\Core\Asset\LibraryDiscovery
arguments: ['@library.discovery.collector']The following could be marked as public: false
- library.discovery.parser
- library.discovery.collector
- asset.js.collection_grouper
- asset.js.dumper
- asset.js.optimizer
- asset.css.collection_grouper
- asset.css.dumper
- asset.css.optimizer
Proposed resolution
Mark them as public: falseand let the testbot decide whether this is okay.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
| Issue category | Task, because there is no real problem |
|---|---|
| Issue priority | Normal because the impact is not high |
| Prioritized changes | The main goal of this issue is to improve performance. |
Files:
| Comment | File | Size | Author |
|---|---|---|---|
| #4 |  Mark-a-couple-of-asset-services-as-non-public-2354705-2.patch | 1.63 KB | yannisc |
| PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,820 pass(es). [ View ] | |||
| #1 | Mark-a-couple-of-asset-services-as-non-public-2354705.patch | 1.59 KB | yannisc |
| FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch Mark-a-couple-of-asset-services-as-non-public-2354705.patch. Unable to apply patch. See the log in the details link for more information. [ View ] | |||
Comments
Comment #1
yannisc commentedYou mean something like that?
Comment #2
dawehner commentedAwesome work!
Comment #4
yannisc commentedThanks, dawehner!
I updated the patch, as the original one does not apply any more.
Comment #5
Crell commentedAnd back.
Comment #6
Wim Leers commentedAFAICT this does not have negative consequences for contrib: they can still override all of the services, including those marked as private.
It does look like it might make alternative asset processing pipeline implementations, that use these services in a different way a bit more difficult, but AFAIK it's still possible to make private services public again by having a container rebuild event listener.
Finally, it's easy to make private services public if we want to do that later; that'd be an API addition, the reverse direction is an API break.
Assuming I've made no mistakes in the above: +1 :)
Comment #7
alexpott commentedCommitted af83168 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.
Comment #9
webchick commentedOver at #2378737: Consider not executing hook_library_(info)_alter() *per* library, but once for all this issue is cited as causing a 150ms slowdown in testbot runs. Adding as a related issue for now, but basically I think we should just roll this back.
Comment #10
Wim Leers commentedThat is actually due to a Symfony bug, this patch didn't do anything wrong.
Comment #11
alexpott commentedI agree with @webchick reverted this. We can still do this because having unnecessary public services is a performance hit. But we need to ensure that doing this does not introduce a far bigger perf regression. According to @dawehner the problem is caused by https://github.com/symfony/symfony/issues/12924.
Comment #13
alexpott commentedre #10 I agree but lets get the test performance benefits and ensure that a patch that is supposed to deliver those benefits does - which means we need to do fix upstream first.
Comment #14
Wim Leers commentedFair enough :)