Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug: Views tokens available in D7 are now missing
Issue priority Major: Followup from #2342287: Allow Twig in Views token replacement
Prioritized changes "Follow-up from a recent critical or major change"
Disruption Minimal: Few modules provide Views tokens. The change is simply replacing a hyphen with a double-underscore.

Problem/Motivation

When using the views overrides placeholders in fields, they don't render the correct values.
The reason is that we are using inline templates for rendering them (actually we allow to use twig there), but the subtokens are formed by using dashes ('-'). This makes them invalid twig variable names, which instead of throwing an error makes twig understand them as a mathematical expression.

Proposed resolution

Replace the field - subfield separator. Instead of using a dash we should use a double underscore.
E.g.: {{ langcode-value}} becomes {{ langcode__value}}

Remaining tasks

Review. Commit.

User interface changes

The tokens available change when overriding a field change.

API changes

The tokens available change when overriding a field change.

Data model changes

None.

Original report:

When using the {{ langcode }} placeholder, it renders the language name not the code. That is a bit obscure but one would expect, its the field rendered. Maybe the field should have settings to be able to render as the langcode. But then there is the {{ langcode-value }} token which was promising because it should definitely render the raw value. That renders numbers though.

Files: 
CommentFileSizeAuthor
#45 2546210-43-45.interdiff.txt1.18 KBmikeker
#45 2546210-views-tokens-45.patch22.28 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,026 pass(es). View
#43 2546210-41-43.interdiff.txt7.48 KBmikeker
#43 2546210-views-tokens-43.patch22.21 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,219 pass(es). View
#41 2546210-40-41.interdiff.txt5.57 KBmikeker
#41 2546210-views-tokens-41.patch16.21 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,192 pass(es). View
#40 2546210-38-40.interdiff.txt1.5 KBmikeker
#40 2546210-views-tokens-40.patch10.64 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,192 pass(es). View
#38 2546210-28-35.interdiff.txt11.2 KBmikeker
#38 2546210-views-tokens-35.patch10.66 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,248 pass(es). View
#28 2546210-views-tokens-28.patch9.79 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,448 pass(es). View

Comments

Gábor Hojtsy created an issue. See original summary.

Gábor Hojtsy’s picture

Title: Langcode filter raw value is converted to a number in views » Langcode field raw value is converted to a number in views
Gábor Hojtsy’s picture

Title: Langcode field raw value is converted to a number in views » Langcode field raw placeholder value (for rewriting output) is converted to a number in views
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content, +sprint
penyaskito’s picture

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.

Gábor Hojtsy’s picture

[2:39pm] GaborHojtsy: penyaskito: well, its in the replacement values for the **langcode field itself**
[2:39pm] GaborHojtsy: penyaskito: so you can rewrite the output of teh langcode field with placeholders
[2:39pm] GaborHojtsy: penyaskito: THAT offers up that placeholder
[2:39pm] GaborHojtsy: penyaskito: for some reason not other places
[2:42pm] GaborHojtsy: penyaskito: I wanted to use it in URLs of a link on another field to pass on language result information in a link for example

penyaskito’s picture

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

penyaskito’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.25 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,370 pass(es), 84 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 8: 2546210-views-tokens-8.patch, failed testing.

penyaskito’s picture

FileSize
3.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,969 pass(es), 164 fail(s), and 5 exception(s). View
760 bytes

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

penyaskito’s picture

Status: Needs work » Needs review

Go testbot!

The last submitted patch, 10: 2546210-views-tokens-10.patch, failed testing.

penyaskito’s picture

FileSize
1.23 KB
3.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,462 pass(es). View

And now for real, sorry for the noise.

penyaskito’s picture

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 }}

Gábor Hojtsy’s picture

Well, not {{ langcode|raw }} because langcode is the rendered version of the language field, so will be the NAME of the language. It would need to be {{ langcode-raw|raw }} because langcode-raw is the raw value and |raw makes it not escaped in Twig. (This one is not an issue for the langcode itself because it will not be able to include things to escape, but for other raw values, yeah).

penyaskito’s picture

FileSize
7.83 KB
7.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,466 pass(es). View

Thanks for clarifying.

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

penyaskito’s picture

Title: Langcode field raw placeholder value (for rewriting output) is converted to a number in views » Views subtokens are broken since the introduction of inline templates for replacements
Issue summary: View changes
dawehner’s picture

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

As we already know, we need tests.

penyaskito’s picture

Issue tags: -Needs tests
FileSize
2.12 KB
1.63 KB
9.33 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,445 pass(es), 9 fail(s), and 0 exception(s). View

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

penyaskito’s picture

[duped]

Status: Needs review » Needs work

The last submitted patch, 19: 2546210-views-tokens-19.patch, failed testing.

penyaskito’s picture

+++ 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 :(

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
1.81 KB
9.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,446 pass(es). View

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

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks good to me. Made minor adjustments to issue summary. Thanks for sticking to it and resolving it.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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.

penyaskito’s picture

Issue tags: -Needs change record

Drafted the change record.

Gábor Hojtsy’s picture

penyaskito’s picture

Status: Needs work » Needs review
FileSize
789 bytes
9.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,448 pass(es). View

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.

Cottser’s picture

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.

penyaskito’s picture

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.

dawehner’s picture

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

mikeker’s picture

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.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags: +Twig

Merged tags.

dawehner’s picture

Status: Needs review » Needs work
mikeker’s picture

mikeker’s picture

mikeker’s picture

Status: Needs work » Needs review
FileSize
10.66 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,248 pass(es). View
11.2 KB

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.

penyaskito’s picture

Status: Needs review » Needs work
  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?

mikeker’s picture

Status: Needs work » Needs review
FileSize
10.64 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,192 pass(es). View
1.5 KB

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!

mikeker’s picture

FileSize
16.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,192 pass(es). View
5.57 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for me! Great that we could merge efforts. Let's get it in then!

mikeker’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
22.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,219 pass(es). View
7.48 KB

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.

stefan.r’s picture

  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

mikeker’s picture

FileSize
22.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,026 pass(es). View
1.18 KB

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

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

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.

Gábor Hojtsy’s picture

Yay, thanks for expanding coverage!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed f71a375 on 8.0.x
    Issue #2546210 by penyaskito, mikeker, Gábor Hojtsy, dawehner: Views...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, amazing, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.