Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
theme system
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
27 Jan 2013 at 22:55 UTC
Updated:
29 Jul 2014 at 21:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
steveoliver commentedTitle.
Comment #2
steveoliver commentedComment #3
tstoecklerYou can roll the patch with
git diff -C -Mwhich reduces the patch size and makes it easier to review.Comment #5
star-szrFixed one of the tests, but the test that's failing in
Drupal\system\Tests\Theme\ThemeTestTwig->testTemplateOverride()is failing in the sandbox as well.Patch re-rolled with
git diff -C -Mas suggested in #3.Comment #6
webchickCan you explain why we're doing this? I'm looking at the official documentation at http://twig.sensiolabs.org/pdf/Twig.pdf and all I see are references to foo.twig, not foo.html.twig. Why are we inventing a Drupalism here?
Comment #7
star-szr@webchick - I just updated the issue summary to include a reference to #1804698: Consider using *.html.twig filename convention
Comment #8
webchickOk, awesome. Thanks for the reference! :)
Comment #10
steveoliver commentedThanks, Cottser. I know you just signed off -- gonna work on getting this passing.
Comment #11
star-szrOkay, managed to track this down. The failing test is only passing on HEAD because the template file is named .twig, not .html.twig. To really fix this, a hardcoded ".tpl" needs to be removed, and a couple strpos() need to be changed to strrpos().
#1678808-1: Defining a new theme engine with templates without .tpl.[template extension] breaks template suggestions (committed as part of #1696786: Integrate Twig into core: Implementation issue) removed one instance of hardcoded .tpl, and in the sandbox @Fabianx removed the other hardcoded .tpl instance with the same technique.
Previously strpos was searching for
.tplso it had a definite boundary and worked fine. Now that it's just searching for "." it should use strrpos instead to allow for template files with extra periods in them - liketheme_test.template_test.tpl.phportheme_test.template_test.html.twig.I'm not sure if we should add a test somewhere to make sure periods can be used in template suggestions. See also #1886448: Rewrite the theme registry into a proper service
@Fabianx should be credited as well on this commit since it incorporates his commit from the sandbox.
Comment #12
steveoliver commentedThanks, Cottser (and Fabianx). You rock.
Comment #13
star-szrIf you apply only the interdiff from #11 (without any .twig -> .html.twig changes) then the test will fail because the template file doesn't have an "extra" extension as in .tpl.php and .html.twig, so something like
theme_test.template_test.hamlwould fail because the last part gets stripped away mistakenly. Here's a slightly better idea that just replaces the whole extension in the filename based on the $extension passed into drupal_find_theme_templates(), no strpos() of any sort needed.Interdiff is from #5.
Happy to move this change into a separate issue if that makes more sense.
Comment #15
star-szrFailure in Drupal\openid\Tests\Upgrade\OpenIDAuthmapUpgradePathTest - looks like #1893800: Something is very, very wrong with update.php / upgrade tests... demons suspected, sending for a re-test.
Comment #16
star-szr#13: 1900458-13.patch queued for re-testing.
Comment #17
catchThis is apparently blocking Twig conversion, so bumping priority.
Comment #18
tim.plunkettLooks good, and the diffstat is nice since we're removing special casing.
Comment #19
webchickCommitted and pushed to 8.x. Thanks!
Comment #20
jenlamptonwoot :)
Comment #21.0
(not verified) commentedAdding reference to .html.twig issue