Another small follow-up from #2068471: Normalize Controller/View-listener behavior with a Page object. That patch introduced renderers for HtmlFragments and HtmlPages, but made the silly mistake of making them the same service. This patch corrects that mistake by splitting them into two separate services, which is what we should have done in the first place. No other changes.

This was already discussed in the course of #2028749: Explore addressable block rendering and we decided it was a good idea, as we expect modules to override the page renderer WAY more often than the fragment renderer; it's also nice and simple so let's get that in first and keep that issue's patch size small.

Patch when I have a nid.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Needs review » Needs work

The last submitted patch, 1: 2166195-split-renderer.patch, failed testing.

The last submitted patch, 1: 2166195-split-renderer.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.92 KB
4.56 KB

I really like the cleanup as it makes the two services straightforward at least in the core implementation.
Just some simple cleanup to get it green.

The last submitted patch, 4: htmlpage-2166195.patch, failed testing.

dawehner’s picture

FileSize
16.59 KB
2.68 KB

The last submitted patch, 6: htmlpage-2166195.patch, failed testing.

dawehner’s picture

FileSize
18.25 KB

Ha, unit tests sometimes should help.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Once again, I post a half-arsed patch and dawehner cleans up my mess. Story of my life...

Thanks, dawehner!

catch’s picture

+++ b/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php
@@ -7,59 +7,15 @@
-  public function render(HtmlFragment $fragment, $status_code = 200) {
-    $page = new HtmlPage('', $fragment->getTitle());
-
-    $page_content['main'] = array(
-      '#markup' => $fragment->getContent(),
-    );
-    $page_content['#title'] = $page->getTitle();
-
-    $page_array = drupal_prepare_page($page_content);
-
-    $page = $this->preparePage($page, $page_array);
-
-    $page->setBodyTop(drupal_render($page_array['page_top']));
-    $page->setBodyBottom(drupal_render($page_array['page_bottom']));
-    $page->setContent(drupal_render($page_array));
-
-    $page->setStatusCode($status_code);
-
-    return $page;

It's really nice to see this code disappearing, but I don't get how we managed to just nuke that in this patch.

Crell’s picture

Hrm. I think that's Git's diff weirdness. That code is not going away, it's just moving from DefaultHtmlPageRenderer to DefaultHtmlFragmentRenderer.

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
diff --git a/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
index 6543fbc..35f6a81 100644

index 6543fbc..35f6a81 100644
--- a/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php

--- a/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php
+++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php

+++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
+++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -2,7 +2,7 @@

Git sees this patch as renaming this class, then removing stuff from it, then adding a new DefaultHtmlPageRenderer.

webchick’s picture

Assigned: Crell » catch

Since catch looked at this before, and committed the parent patch, shooting over to him.

catch’s picture

8: wscci-2166195.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: wscci-2166195.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.23 KB

Rerolled

Crell’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll

error: core/core.services.yml: does not exist in index
error: core/lib/Drupal/Core/Controller/ExceptionController.php: does not exist in index
error: core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php: does not exist in index
error: core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php: does not exist in index
error: core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php: does not exist in index
error: core/lib/Drupal/Core/Page/HtmlPageRendererInterface.php: does not exist in index
error: core/lib/Drupal/Core/Page/HtmlPageRendererInterface.php: does not exist in index
error: core/modules/system/lib/Drupal/system/Controller/BatchController.php: does not exist in index
error: core/tests/Drupal/Tests/Core/Controller/ExceptionControllerTest.php: does not exist in index
dawehner’s picture

Status: Needs work » Needs review
FileSize
18.82 KB

Rerolled.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Sigh

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture