Views subtokens are broken since the introduction of inline templates for replacements

Trying to reproduce this. If I understood correctly, for reproducing it I need to edit the "Content" view, add the language field, add a "Global: Custom Text" field and use {{ langcode-value }} as a replacement.

@Gábor: Where did you get that you could use "{{ langcode-value }}"? I don't see it listed at the replacements section. Actually, if you add anything (e.g. {{ langcode-value-unknown }}, you get a "0". This is a bug for sure, but maybe a different one than expected.

I was able of reproducing it, working on a potential patch

The problem is that we are using inline_templates for replacements and we are composing our tokens by concatenating fields and columns with hyphens.
But a twig variable name cannot contain a hyphen, so its evaluation is failing and we are getting wrong values.

I guess it's too late in the release cycle for changing how we compose the tokens, and it will be a change from how D7 worked.
Let's see if a simple substitution works.

Ok, let's replace the token completely. But in that case, I'm wondering if we should rely on twig at all.

Go testbot!

And now for real, sorry for the noise.

Talked with Gábor about this. As the overwrite field description says that twig is allowed, people aware of twig variable naming restrictions can find out that the twig code we are forcing them to write for the overrides is invalid, so we should accept this API change, as anything else is even more problematic.

Also, in the description of the langcode-value we specify "raw value", but taking into account that twig has autoescape enabled, that's not true anymore. If we want to render the raw value we are forced to use {{ langcode|raw }}

Thanks for clarifying.

New attempt, now by replacing the tokens separator with two underscores ("__"), which makes it a valid twig variable.

Wow, yeah I think the right fix is to use __ instead of just _

As we already know, we need tests.

Added a test for token replacements.
I include test-only patch with the tokens using dashes so we can see how it fails.

[duped]

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -390,7 +390,7 @@ function ($children, $elements) {
-      return (string) $this->getRenderer()->render($build);
+      return (string) $this->getRenderer()->renderPlain($build);

No, we can't do that :(

@dawehner told me how to fix + test my issue with rendering without a render context. This should be green, and tests included should suffice.

I think we should have a CR for this and an assert() in whichever method gathers all the tokens that they don't contain a - or any other illegal twig variable characters.

Drafted the change record.

Added assertion. I couldn't find any docs on twig about how variable naming is validated, aside of that they cannot contain dashes. I added the regexp from PHP variable names validation.

Nice to see that this is further along, however I think it's a duplicate of #2371633: Views uses hyphens in many tokens, but that is not valid Twig syntax.. Might be good to compare notes before closing one as a dupe.

Nitpick:

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -913,19 +913,28 @@ protected function addSelfTokens(&$tokens, $item) {
+      else if($raw && is_object($raw)) {

Missing space after the if.

Great, now we have a test for an actual token! Here are some small bits.

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -365,6 +365,8 @@ protected function viewsTokenReplace($text, $tokens) {
   +        assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $token) === 1', 'Tokens need to be valid variables.');
   

   Could we point to some place in twig so in case they change we at least know where the regex was coming from?

2. +++ b/core/modules/views/src/Tests/Plugin/PluginBaseTest.php
   @@ -0,0 +1,68 @@
   +      $tokens = [ '{{ langcode }}' => SafeString::create('English'), '{{ langcode__value }}' => 'en'];
   

   Nitpick: Space after [

3. +++ b/core/modules/views/src/Tests/Plugin/PluginBaseTest.php
   @@ -0,0 +1,68 @@
   +
   +
   

   Two empty spaces

4. +++ b/core/modules/views/src/Tests/Plugin/PluginBaseTest.php
   @@ -0,0 +1,68 @@
   +
   +namespace Drupal\views\Plugin\views {
   +
   +  /**
   ...
   +}
   

   You don't need an extra namespace, ... you can keep the existing one of the test

I merged the test and taxonomy/role handler changes in this issue into a new patch in #2371633: Views uses hyphens in many tokens, but that is not valid Twig syntax..

As mentioned in that issue, I don't really care which issue moves forward and which is marked as a dup -- the solutions are nearly identical. I things do move forward in this issue, I would like to see the removal of the object-to-array cast and the CoreXss namespace alias merged into this issue.

@penyaskito, thank you for your work on this and scaffolding the tests! I had stalled on that in the other issue and hadn't had time to return to it.

Added beta-eval from #2371633: Views uses hyphens in many tokens, but that is not valid Twig syntax..

Referenced in dup issue.

Merged patch from #2371633-32: Views uses hyphens in many tokens, but that is not valid Twig syntax.. Interdiff is against the patch in #28 of this issue -- interdiffs against the other issue can be found in the first link.

As mentioned in #32, this patch includes the cleanup from the dup issue: removing the object-to-array cast and the extraneous CoreXss namespace alias. I would like to see more tests to cover the original bug report in the dup issue -- I'll work on those later today.

1. +++ b/core/modules/taxonomy/src/Plugin/views/field/TaxonomyIndexTid.php
   @@ -169,16 +169,16 @@ function render_item($count, $item) {
   +    $tokens['{{ ' . $this->options['id'] . '__vocabulary-vid' . ' }}'] = $this->t('The machine name for the vocabulary the term belongs to.');
   

   Is this a valid variable?

2. +++ b/core/modules/taxonomy/src/Plugin/views/field/TaxonomyIndexTid.php
   @@ -169,16 +169,16 @@ function render_item($count, $item) {
   +      $tokens['{{ ' . $this->options['id'] . '__' . str_replace('_', '-', $token) . ' }}'] = isset($item[$token]) ? $item[$token] : '';
   

   Does that str_replace add - instead of removing them?

Is this a valid variable?

Nope.

Does that str_replace add - instead of removing them?

Yup.

Thanks for the review @penyaskito. Sorry, I dorked those up when merging the two patches. The fact that the testbot gave us a green light tells me that we need better test coverage around this!

Adds tests for the all-terms field handler tokens (those from TaxonomyIndexTid).

Hate to remove an RTBC, but I wanted to get some tests in that cover the original report in #2371633: Views uses hyphens in many tokens, but that is not valid Twig syntax.. Sorry, took longer than expected to get back to this...

This patch adds tests to the Node module for the tokens __value, __summary, and __format tokens.

1. +++ b/core/modules/node/src/Tests/Views/NodeFieldTokensTest.php
   @@ -0,0 +1,65 @@
   + * @see core/modules/node/src/Tests/NodeTokenReplaceTest.php
   

   Should we refer to this by the namespaced class name?

2. +++ b/core/modules/node/src/Tests/Views/NodeFieldTokensTest.php
   @@ -0,0 +1,65 @@
   +  public static $testViews = array('test_node_tokens');
   ...
   +    $node_type = entity_create('node_type', array('type' => 'article', 'name' => 'Article'));
   

   Nit: bracket notation

3. +++ b/core/modules/node/src/Tests/Views/NodeFieldTokensTest.php
   @@ -0,0 +1,65 @@
   +  public function testViewsTokenReplacement() {
   

   Missing method doc

@stefan.r: Thanks for the review. Fixed.

Extra test coverage can only be a good thing! Haven't reviewed the complete patch but RTBCing considering #41 was RTBC and the test looks good.

This issue is a normal bug fix, and disruption is necessary to fix the bug and has a CR, so it is allowed per https://www.drupal.org/core/beta-changes. Committed f71a375 and pushed to 8.0.x. Thanks!