Task

Use Twig instead of PHPTemplate

Remaining

  • Replace all theme functions and templates with .html.twig equivalent templates
  • Add new preprocess functions for the .html.twig equivalent templates
  • Update all hook_theme definitions

Related

#1885560: [meta] Convert everything in theme.inc to Twig

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Active » Postponed

Oops, status.

star-szr’s picture

Status: Postponed » Active

As discussed on IRC, let's pick this back up again, rolling a patch combined with #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig so this is ready to go.

joelpittet’s picture

Issue tags: +Needs manual testing
FileSize
2.5 KB

First crack at this one. I didn't take much except most the docs from the sandbox. Hopefully that is ok. I used spaceless because of it's fancy little trick where it negates the newline at the end of the file and causes the tests to pass.

Could use some manual testing and review in general.

joelpittet’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1961876-twig-link.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review

Ok seeing as that failed, I rolled the same patch above with #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig to see how it fairs.

joelpittet’s picture

FileSize
35.15 KB
35.31 KB

The interdiff on this is a bit silly (because it's #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig). But this comment has a patch attached... oOooo, ahhhh!

Status: Needs review » Needs work

The last submitted patch, 1961876-6-twig-link.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
35.25 KB

Tweaks but still fails on active link because it's not using l(), maybe I should move some of that logic to theme_link?

Status: Needs review » Needs work

The last submitted patch, 1961876-9-twig-link.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
36.28 KB
35.86 KB
star-szr’s picture

Green!

tlattimore’s picture

Issue tags: -Needs manual testing

This is looking really good to. Manual tested:

Before Twig:

<a href="/node" class="custom-class" id="unique-id">Custom link</a>

After Twig:

<a href="/node" class="custom-class" id="unique-id">Custom link</a>

c4rl’s picture

Assigned: joelpittet » Unassigned
thedavidmeister’s picture

Status: Needs review » Postponed

Looks good, but surely postponed on the patch it incorporates being committed first.

star-szr’s picture

shanethehat’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, twig-link-1961876-17.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
5.62 KB
1.63 KB

oops, somehow missed a whole function

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, twig-link-1961876-19.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
Issue tags: +Twig

#19: twig-link-1961876-19.patch queued for re-testing.

Tests pass locally, I suspect random failure.

thedavidmeister’s picture

Status: Needs review » Needs work

@shanethehat - please don't use url_is_active() and please do statically cache the function calls used to detect the link's active status and please do optimise the logic of the active checks to stop comparing variables once we know there's no match.

see comments #106 through to #122 at #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig for an explanation of all of this :)

thedavidmeister’s picture

shanethehat’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
4.16 KB

As discussed on IRC, this is essentially a copy/paste of the part of l() that handles the determination of 'active'. It's an unpleasant duplication that should be resolved by #1985974: Make l() optionally return structured output

thedavidmeister’s picture

thedavidmeister’s picture

Status: Needs review » Closed (duplicate)

Theme link was removed, there's nothing to convert any more.

thedavidmeister’s picture

Issue summary: View changes

Update summary