Remove PHPTemplate, and add test coverage for multiple theme engine support

Discussed with @alexpott, @stefan.r, @lauriii, @joelittet, and @dawehner. At a minimum, clearly documenting that theme engines are expected to sanitize non-safe text in templates is critical, because module code now often relies on autoescape.

Talked with @dawehner and @tim.plunkett and we'll add a hook_requirements() as well as the deprecation in test error handler for the specific test.

+++ b/core/themes/engines/phptemplate/phptemplate.engine
@@ -3,6 +3,15 @@
+ * it, makes it really hard to provide safe output. Instead its highly

Should be "Instead it is highly..." or "Instead it's highly..."

Personally I think it's confusing to still include phptemplate in core. Earlier in the D8 cycle, when I spoke to people about using Twig now, I had at least one person say "Sure, but you can still use phptemplate for themes." In reality that's going to be a bucket of pain. We can try to educate people to not use phptemplate through documentation, etc., but I think it would be much better to not provide that in core. If somebody really wants to make use of phptemplate, then taking that on could be a contrib task.

It seems wise to remove PHPTemplate if it's missing all these sanitization tools, but really this should have already been decided a couple years ago.

I think given how much is will admit bad ports from 7 if we leave it, we should remove it.

pwolanin++

I'm all for removing deprecated options. There are already enough "please don't do this" options in the theming system.

I am also leaning toward removing it entirely, FWIW. As I mentioned to @dawehner in IRC, I feel a PHPTemplate theme is practically guaranteed to have an XSS vulnerability at this point, more so than even some of the other things we've made it critical to remove. And while this would totally break any PHPTemplate theme already ported... they are most likely already broken.

It is a big BC break though if you were expecting it to work still in D8. I'd suggest doing the thing we did with checkPlain() where we deprecate and announce the removal because of the late beta BC break, except if we do have an RC soon then it wouldn't be much forewarning anyway. We should have deprecated it a year ago. Oops. =/

Tweeted about the issue to try to get some visibility: https://twitter.com/xjmdrupal/status/647483540104478720

I agree it would be hard to even get it to work properly and you'd be in a world of hurt trying to use it to the same degree as Twig. Let that be handled outside of core, if really necessary. And if we leave it, +1 to what pwolanin said. People are going to try to port themes directly, which we don't want. We want them putting the effort into converting to Twig.

Yet another vote for removing it before the RC, even with the BC break @xjm describes in #14. Because if we don't now, we'll be supporting it until 9.0.0. Does anyone want to do that?

A little pain now vs. lots of pain over the next few years...

The most awesome test in progress.

Adding to what Mike said, the real question is are we going to support phptemplate? If a critical problem is found with it, who is going to fix it or care? If it stays we have a responsibility to support it, and I don't think anyone wants any part of that. And if you've been porting a theme to phptemplate in 8, you've literally been ignoring anything and everything everyone has been saying about theming in 8.

Sending first time for the test bot. Removes phptemplate templating engine and replaces it with awesome Nyan cat templating engine!!

Discussed with @alexpott, @stefan.r, @dawehner, @Berdir, @pfrenssen, @lauriii.

One of the primary reasons that we focused on removing the ! t() placeholder is because we want to live by the mantra that the theme system auto-escapes for you. Leaving support for PHPTemplate in core leaves a glaring hole in this strategy, and if we don't remove it before RC1, we're stuck supporting it in D8 until Drupal 10.

What we did agree on is that it definitely is required to support multiple theme engines in core (Smarty, et al). So we can remove PHPTemplate as long as we have test coverage to confirm that theme engine support persists.

@stefan.r also brought up the point from @davidhernandez that we don't have the resources to commit to supporting two theme engines in core. I agree; much better to whole-ass Twig. :)

So consider it decided. :) Dawehner and lauriii are working on a patch.

We should also document... somewhere that any other templating engine would also need to handle sanitization of variables. And that should probably also get added to the change records, too. Speaking of. :)

Does #21 intentionally include the other patch from the other issue?

Yes, its for testing purposes. So this is blocked by that

Does this need more than a change record? Its own core announcement or anything?

Just fixing very small nitpicks.

Added change record draft.

Fixing that error. It was a copy/paste issue.

I agree on removing PHPTemplate. Is the reason to add nyan cat just to allow testing of multiple theme engines? If so, we should document that a bit more clearly.

Its under testing module section so I dont personally see reason to add extra documentation beside what we already have. Anyway if you disagree, maybe you could add some docs?

@hussainweb: I think that strlen check was there for a reason; consider the case where #markup is '0'.

@lauriii, @dawehner: I just wanted to understand this myself first, that's all. It's true that this being under tests directory is an indication enough. I didn't notice that earlier.

@longwave: You're right. I built a table in my head to see where it could be affected, but missed '0'. Reverting that change now.

___━*━___━━___━__*___━_*___┓━╭­­­­­━━━━╮
__*_━━___━━___━━*____━━___┗┓|:­­­­­:::::^-------^
___━━___━*━___━━____━━*___━┗|:­­­­­:::|｡◕‿‿­­­­◕｡|
___*━__━━_*___━━___*━━___*━━╰O­­­­­--O---O--O

Change record updated appropriately.

This is so awesome!

We should also document... somewhere that any other templating engine would also need to handle sanitization of variables.

There's an older issue for that (linked) mentioning at least one more place where it needs documenting.

+++ b/core/modules/language/language.admin.inc
@@ -201,5 +201,5 @@ function template_preprocess_language_content_settings_table(&$variables) {
 function theme_language_content_settings_table($variables) {
...
+  return '<h4>' . theme_escape_and_render($variables['build']['#title']) . '</h4>' . drupal_render($variables['build']);

I think this is asking for trouble - can't the theme system/engine do this for us - i.e. it already knows which theme hooks are functions and which use a template - can't it call theme_escape_and_render on non templates automatically? We don't want to leave it to chance - it will be easy to forget.

Realised this is from #2572929: Document lack of auto-escape in theme functions and add a theme autoescape helper function, copying my comment over there.

Looks like there is a lot of confusion about these two patches' overlap and dependency -- let's postpone this one explicitly.

Thanks for the CR. I'll draft an announcement for g.d.o/core.

ok, here's a clean patch just for phptemplate removal and addition of the test engine.

I don't think we need to postpone this since is no overlap?

sorry, I didn't realize it uses the new function.

Posted on g.d.o: https://groups.drupal.org/node/482888

This needs a change record but otherwise it looks really good.

There is a change record draft. They are always listed in the sidebar.

this is überawesome :)

Looks great

RTBC++ & Sorry Drupal, Nyan Cats gonna be in your HEAD till forever!

So long and thanks for all the kittens, PHPTemplate.

 ___________________________________________________________________________________________________________________
< Committed 1648376 and pushed to 8.0.x. Thanks! >
 -------------------------------------------------------------------------------------------------------------------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||