Problem/Motivation

Drupal\Core\Template\Loader\StringLoader needs to implement \Twig_SourceContextLoaderInterface. This is caught by the test infra in #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation

Proposed resolution

Implement \Twig_SourceContextLoaderInterface

Remaining tasks

User interface changes

None

API changes

We're keeping up with the Twig API (kinda - Twig is already 2.x and cantering towards 3.x where this new interface will be removed).

Data model changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
895 bytes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

\Twig_Loader_String::getSourceContext has the same exact implementation ...

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Looks like we should be removing getSource() as well.

* 1.27.0 (2016-10-25)
…
 * deprecated Twig_LoaderInterface::getSource() (implement Twig_SourceContextLoaderInterface instead)

https://github.com/twigphp/Twig/blob/1.x/CHANGELOG

joelpittet’s picture

@Cottser, we can't remove it for BC, should we just mark it as deprecated also?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.15 KB

Here's a suggestion on the BC wording in patch form.

Cottser’s picture

Status: Needs review » Needs work

Excellent point @joelpittet thanks :)

+++ b/core/lib/Drupal/Core/Template/Loader/StringLoader.php
@@ -36,6 +36,9 @@ public function exists($name) {
+   * backwards compatibility with Twig <= 1.26.0.

There is a Twig 1.26.1 release so to be really specific I think we should say < 1.27.0.

Also, this second line of the @deprecated needs to be indented by two spaces. https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

"Lines after tags such as @param, @return, etc. that contain documentation connected with that tag are indented two spaces. See syntax examples for each tag."

And in the odd chance someone is calling this, we could say "Use getSourceContext() instead." That's what the Twig deprecation docs say anyway, this is from a compiled template:

    /** @deprecated since 1.27 (to be removed in 2.0). Use getSourceContext() instead */
    public function getSource()
joelpittet’s picture

I tried this patch with an older Twig and it failed because that Twig_SourceContextLoaderInterface interface didn't exist before then (should have been obvious I suppose).

Do we need to put in a copy of the interface if it doesn't exist or maybe just bump the min version of twig so we don't have to deal with it?

Is there precedent set for 3rd party interface additions?

joelpittet’s picture

Thanks for the doc notes, btw. I'll address them in the next patch.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

Quick IRC ask and going to go with bumping the Twig version to 1.27.0 minimum. Leaving the old getSource() method there in case someone is using that by extending this class, it's not @internal so that's why the extra caution.

The version bump is to avoid copying and maintaining the interface for old versions of twig.

alexpott’s picture

Looking at the Twig_Loader_String class it says...

@trigger_error('The Twig_Loader_String class is deprecated since version 1.18.1 and will be removed in 2.0. Use Twig_Loader_Array instead or Twig_Environment::createTemplate().', E_USER_DEPRECATED);

Maybe this change should go further and remove our usage of \Drupal\Core\Template\Loader\StringLoader completely - basically do part of #2572605: Update to Twig 2.0.0 in a more scoped way.

joelpittet’s picture

I'll dig in on that, it didn't sound as clear cut as stringloader but may be better than chasing the interface of something they are dropping but probably still needs a version bump and this new interface regardless?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.