Problem/Motivation

The internal path has always been a part of front theme hook suggestions. The related issue (cause of this regression) instead, prematurely preempted detection of the front page to determine which arguments are passed to theme_get_suggestions(), which is where the actual --front suggestion addition lives.

Currently, there is an empty string in an array passed for front suggestions, which causes an improper theme hook suggestion:

<!-- FILE NAME SUGGESTIONS:
   * html--front.html.twig
   * html--.html.twig <----
   x html.html.twig
-->

<!-- FILE NAME SUGGESTIONS:
   * page--front.html.twig
   * page--.html.twig <----
   x page.html.twig
-->

Proposed resolution

system_theme_suggestions_html and system_theme_suggestions_page should simply pass the normally split internal path like every other page.

Remaining tasks

  • Create initial patch
  • Create tests

User interface changes

None

API changes

None, fixes a regression to this API

Data model changes

None

CommentFileSizeAuthor
#7 2754929-7.patch3.2 KBjoelpittet
#7 interdiff.txt587 bytesjoelpittet
#4 2754929-4.patch3.21 KBjoelpittet
#4 2754929-4--test-only.patch2.03 KBjoelpittet
#2 internal_path_s-2754929-2.patch1.59 KBmarkhalliwell

Comments

markcarver created an issue. See original summary.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new internal_path_s-2754929-2.patch1.59 KB

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: -Needs tests
StatusFileSize
new 2754929-4--test-only.patch2.03 KB
new 2754929-4.patch3.21 KB

I had to trim both right and left for the / because with a path of just '/' produced an empty result too.
Command line proof: php -r "print_r(explode('/', ltrim('/', '/')));"

This test does a node like path that is resolved to the home page (mocked with prophasize, thanks for help from @klausi).

The last submitted patch, 4: 2754929-4--test-only.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/KernelTests/Core/Theme/RegistryTest.php
@@ -157,4 +159,37 @@ public function testThemeRegistryAlterByTheme() {
+   * Tests that the theme registry can be altered by themes.

This comment doesn't describe at all what is actually being tested. Beside that the patch looks good for me.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt587 bytes
new 2754929-7.patch3.2 KB

Thanks I forgot to change that from the test copied from above.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Thanks! For me this is RTBC.

  • catch committed 2b74c93 on 8.3.x 
    Issue #2754929 by joelpittet, markcarver: Internal path(s) missing for "...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Nice that this actually simplifies the code and thanks for the test coverage.

Committed/pushed to 8.3.x, thanks!

Status: Fixed » Closed (fixed)

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