Problem/Motivation

KernelTestBase::render is passing ActiveTheme::getName return value as a parameter for BareHtmlPageRenderer::renderBarePage. BareHtmlPageRenderer::renderBarePage is actually excpecting for a theme hook instead of a theme name.

Proposed resolution

-

Remaining tasks

-

User interface changes

-

API changes

-

Data model changes

-

CommentFileSizeAuthor
#14 kerneltestbase_render-2661470-14.patch3.13 KBlauriii
#9 2661470-theme-bare-page-9.patch2.78 KBandypost
#2 2661470-theme-bare-page.patch2.74 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
FileSize
2661470-theme-bare-page.patch2.74 KB

Looking at \Drupal\Core\Render\BareHtmlPageRenderer::renderBarePage() we need to make $page_theme_property default to page

All test files I can grep are in patch

Status: Needs review » Needs work

The last submitted patch, 2: 2661470-theme-bare-page.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I don't see what the problem is in the existing code TBH.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review

The documentation for this parameter says: 

   * @param string $page_theme_property
   *   The #theme property to set on #type 'page'.

so passing in the current active theme cannot be right

Status: Needs review » Needs work

The last submitted patch, 2: 2661470-theme-bare-page.patch, failed testing.

The last submitted patch, 2: 2661470-theme-bare-page.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

IMHO we should go with 'maintenance_page', as this is the more realistic example of core.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
2661470-theme-bare-page-9.patch2.78 KB

Yep, bare page should skip a lot, let's see what bot thinks

Status: Needs review » Needs work

The last submitted patch, 9: 2661470-theme-bare-page-9.patch, failed testing.

The last submitted patch, 9: 2661470-theme-bare-page-9.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

IMHO we should go with 'maintenance_page', as this is the more realistic example of core.

+1

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2568511: Fix broken test: KernelTestBase::render
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
kerneltestbase_render-2661470-14.patch3.13 KB

Rerolling this on 8.1.x

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

the only question here why that was working before... and #9 failed

  • alexpott committed 5f65d69 on 8.2.x 
    Issue #2661470 by andypost, lauriii: KernelTestBase::render passes wrong...

  • alexpott committed dd7dbd2 on 8.1.x 
    Issue #2661470 by andypost, lauriii: KernelTestBase::render passes wrong...
alexpott’s picture

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

#9 failed because HEAD was broken.

Committed dd7dbd2 and pushed to 8.1.x and 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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