Problem/Motivation

Noticed while working on #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

  1. The use of the ElementInfoManager service is useless.
  2. The CacheContextsManager service is unused.

This would help #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) be simpler.

Proposed resolution

Remove both.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because unused injected services are a potential performance problem, but at least they decrease maintainability.
Issue priority Normal because just some cleanup.
Prioritized changes The main goal of this issue is to help a performance issue be easier and to cleanup unused services to help maintainability.
Disruption Likely not disruptive for contrib as overriding HtmlRenderer is not yet so common.
CommentFileSizeAuthor
#14 interdiff.txt1022 byteswim leers
#14 simplify-htmlrenderer-2493021-14.patch4.67 KBwim leers
#11 remove_unused_useless-2493021-10.patch3.86 KBfabianx
#11 interdiff.txt715 bytesfabianx
#10 interdiff.txt794 byteswim leers
#10 simplify-htmlrenderer-2493021-10.patch4.05 KBwim leers
#1 simplify-htmlrenderer-2493021-1.patch4.08 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new simplify-htmlrenderer-2493021-1.patch4.08 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -138,7 +117,6 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    $html += $this->elementInfoManager->getInfo('html');

For completeness: this is the part that neither Fabianx nor I know what purpose its serves.

To the best of my knowledge, it is related to #2352155-64: Remove HtmlFragment/HtmlPage, but I can't tell if it's truly (still) necessary.

If it passes, it isn't necessary anymore. If it doesn't pass, it is, and we can investigate :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Tests passed; this is good to go.

dawehner’s picture

dawehner
Primary language German
commented
Priority: Major » Normal
Status: Needs review » Patch (to be ported)

How the

dawehner’s picture

dawehner
Primary language German
commented
Status: Patch (to be ported) » Reviewed & tested by the community

.

fabianx’s picture

fabianx commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/core.services.yml
@@ -862,7 +862,7 @@ services:
-    arguments: ['@title_resolver', '@plugin.manager.display_variant', '@event_dispatcher', '@element_info', '@module_handler', '@renderer', '@render_cache', '@cache_contexts_manager']
+    arguments: ['@title_resolver', '@plugin.manager.display_variant', '@event_dispatcher', '@module_handler', '@renderer', '@render_cache', '@cache_contexts_manager']

Uhm, so why do we not remove the @cache_contexts_manager here as well?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

That elementInfo line is supposed to add in the defaults for the '#type' => 'html' element, see \Drupal\Core\Render\Element\Html::getInfo():

    return array(
      '#theme' => 'html',
      // HTML5 Shiv
      '#attached' => array(
        'library' => array('core/html5shiv'),
      ),
    );

Additionally, someone could alter the definition and add their own #pre_render or something...

fabianx’s picture

fabianx commented
Issue summary: View changes
fabianx’s picture

fabianx commented

#7: Correct, but Renderer::doRender() does that already for us automatically, so we should not need to special case that. We don't need to special case it anywhere else in core.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new simplify-htmlrenderer-2493021-10.patch4.05 KB
new interdiff.txt794 bytes
fabianx’s picture

fabianx commented
StatusFileSize
new interdiff.txt715 bytes
new remove_unused_useless-2493021-10.patch3.86 KB

A new patch removing the context definition as well.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Alright, Wim wins! :)

dawehner’s picture

dawehner
Primary language German
commented
Status: Reviewed & tested by the community » Needs work

... Now that we remove the usages, YEAH, let's also drop the use statements ...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new simplify-htmlrenderer-2493021-14.patch4.67 KB
new interdiff.txt1022 bytes

Done, plus one more unused use statement.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Quickfix

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: simplify-htmlrenderer-2493021-14.patch, failed testing.

Wim Leers queued 14: simplify-htmlrenderer-2493021-14.patch for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Testbot--

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 83b9aa8 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 83b9aa8 on 8.0.x 
    Issue #2493021 by Wim Leers, Fabianx, dawehner: Remove unused...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.