Spin-off from #2341357: Views entity area config is not deployable and missing dependencies

Problem/Motivation

Views supports two types of tokens, argument placeholders that correspond to View arguments (e.g. %1 or !3), and global tokens. The use of these tokens affects what is valid input for the View, however, Views sometimes checks for only the opening character of the token, and other times does not validate for token input at all. See #2392823: [meta] Much Views UI input is not validated.

Additionally, the existing method StylePluginBase::usesTokens() is never used or tested in D8 HEAD.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it does not fix any bug by itself.
Issue priority Normal because it is a simple API addition to make Views input validation simpler, but the related issues are not blocked by this addition.
Prioritized changes The main goal of this issue is reducing fragility by providing a single API for matching tokens in Views and removing related unused, untested code. Also, the patch simplifies a critical and a major. Therefore, this could be considered a prioritized change during the beta.
Disruption Not disruptive for core or contributed modules as it is only an API addition, and only requires limited changes within core for consistency.

Proposed resolution

Add API methods for matching tokens in Views.

Remaining tasks

  • There are still several strpos($path, '%') (without !) about in path-checking code.
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

None.

API changes

  • New methods on PluginBase.
  • DisplayPluginBase::getArgumentsToken() renamed to DisplayPluginBase::getArgumentToken()
Files: 
CommentFileSizeAuthor
#91 2396607-91-88reroll.interdiff.txt607 bytesmikeker
#91 2396607-91-vdc-token-cleanup.patch45.94 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#88 2396607-88-83.interdiff.txt6.28 KBmikeker
#88 2396607-88-vdc-token-cleanup.patch44.7 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#83 2396607-83-78.interdiff.txt4.08 KBmikeker
#83 2396607-83-vdc-token-cleanup.patch38.42 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#81 2396607-81-78.interdiff.txt1.01 KBmikeker
#81 2396607-81-vdc-token-cleanup.patch37.64 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#79 2396607-78-74.interdiff.txt6.94 KBmikeker
#78 2396607-78-74.interdiff.txt0 bytesmikeker
#78 2396607-78-vdc-token-cleanup.patch37.67 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#74 2396607-74-70.interdiff.txt793 bytesmikeker
#74 2396607-74-vdc-token-cleanup.patch37.91 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#70 2396607-70-69.interdiff.txt1.08 KBmikeker
#70 2396607-70-vdc-token-cleanup.patch37.35 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#68 allow_views_token-2396607-68.patch36.67 KBAjitS
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#68 interdiff.txt654 bytesAjitS
#65 allow_views_token-2396607-65.patch36.74 KBAjitS
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/display/DisplayPluginBase.php. View
#65 interdiff.txt2.41 KBAjitS
#64 allow_views_token-2396607-64.patch34.88 KBAjitS
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#64 interdiff-62-64.txt655 bytesAjitS
#59 vdc-token_2396607-59.patch33.61 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,298 pass(es), 111 fail(s), and 82 exception(s). View
#57 vdc-token_2396607-57.patch32.66 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,306 pass(es), 112 fail(s), and 82 exception(s). View
#52 interdiff-serialization-trait.txt581 bytesxjm
#51 vdc-token-2396607-50.patch33.72 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,083 pass(es), 272 fail(s), and 61 exception(s). View
#45 interdiff.txt607 bytesxjm
#42 vdc-token-interdiff.txt605 bytesmikeker
#42 vdc-tokens_2396607-41.patch33.63 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,566 pass(es), 569 fail(s), and 289 exception(s). View
#39 vdc-token-2396607-39.patch33.63 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/area/Title.php. View
#33 interdiff.txt15.89 KBxjm
#33 vdc-token-2396607-33.patch12.01 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,180 pass(es). View
#32 interdiff.txt10.37 KBxjm
#32 vdc-token-2396607-30.patch8.17 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,190 pass(es). View
#22 interdiff-20-22.txt7.58 KBxjm
#22 vdc-token-2396607-22.patch12.44 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,782 pass(es), 43 fail(s), and 229 exception(s). View
#20 interdiff-18-20.txt4.3 KBxjm
#20 vdc-token-2396607-20.patch8.64 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,802 pass(es). View
#18 interdiff-0-18.txt5.5 KBxjm
#18 vdc-token-2396607-18.patch5.37 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,788 pass(es). View
vdc-hastoken.patch4.76 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/style/StylePluginBase.php. View

Comments

xjm’s picture

Issue summary: View changes
xjm’s picture

  1. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -106,10 +106,49 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
    -      $value = $this->view->style_plugin->tokenizeValue($value, 0);
    +      $value = $this->view->getStyle()->tokenizeValue($value, 0);
    

    Note that this change is in scope because it ensures that the style plugin is initialized when this is used.

  2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -244,6 +230,57 @@ public function tokenizeValue($value, $row_index) {
    +    // If the string definitely does not contain a token, return FALSE
    +    // immediately for performance.
    +    if ((strpos($value, '!') === FALSE) && (strpos($value, '%') === FALSE)) {
    +      return FALSE;
    +    }
    ...
    +    // If the string definitely does not contain a token, return FALSE
    +    // immediately for performance.
    +    if (strpos($value, '[') === FALSE) {
    +      return FALSE;
    +    }
    

    These optimizations are added based on the removed method, but maybe they're not needed.

xjm’s picture

Status: Needs review » Needs work

NW for unit tests which I'm trying to add next. :)

dawehner’s picture

  1. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -106,10 +106,49 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
    -      $value = $this->view->style_plugin->tokenizeValue($value, 0);
    +      $value = $this->view->getStyle()->tokenizeValue($value, 0);
    

    <3

  2. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -106,10 +106,49 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
    +  public function hasViewsToken($value) {
    +    return $this->view->getStyle()->hasViewsToken($value);
    +  }
    ...
    +  public function hasGlobalToken($value) {
    +    return $this->view->getStyle()->hasGlobalToken($value);
    +  }
    ...
    +  public function hasToken($value) {
    +    return $this->view->getStyle()->hasToken($value);
    +  }
    

    Are you sure we really need this wrapper methods here? I also still don't see reason why they should be public?

  3. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -228,7 +214,7 @@ public function getRowClass($row_index) {
    -    if (strpos($value, '[') !== FALSE || strpos($value, '!') !== FALSE || strpos($value, '%') !== FALSE) {
    +    if ($this->hasGlobalToken($value) || $this->hasViewsToken($value)) {
    

    Can you explain why we have to check for global tokens, even these tokens here don't support global tokens at all? Note: At least at the moment we just support row level and arguments tokens.

xjm’s picture

Are you sure we really need this wrapper methods here? I also still don't see reason why they should be public?

With the wrapper methods I was just following the pattern of... some other method on the class. I also debated whether they were actually necessary. Probably simpler to just take them out if you don't see a reason.

Can you explain why we have to check for global tokens, even these tokens here don't support global tokens at all? Note: At least at the moment we just support row level and arguments tokens.

Hm? The code in head supports global tokens, or at least it checks for [, so I just converted it to the new methods. Also that hunk is in StylePluginBase. Maybe I'm using the words "global token" incorrectly and the method name is wrong? I was just using it to refer to the utility Token class tokens.

xjm’s picture

Issue summary: View changes
dawehner’s picture

Hm? The code in head supports global tokens, or at least it checks for [, so I just converted it to the new methods. Also that hunk is in StylePluginBase. Maybe I'm using the words "global token" incorrectly and the method name is wrong? I was just using it to refer to the utility Token class tokens.

So yeah we use the same language here.

Let's have a look at StylePluginBase::tokenizeValue():

  public function tokenizeValue($value, $row_index) {
    if (strpos($value, '[') !== FALSE || strpos($value, '!') !== FALSE || strpos($value, '%') !== FALSE) {
      // Row tokens might be empty, for example for node row style.
      $tokens = isset($this->rowTokens[$row_index]) ? $this->rowTokens[$row_index] : array();
      if (!empty($this->view->build_info['substitutions'])) {
        $tokens += $this->view->build_info['substitutions'];
      }

      if ($tokens) {
        $value = strtr($value, $tokens);
      }
    }

    return $value;
  }

"!" and "%" are used for $this->view->build_info['substitutions'], see ViewExecutable::_buildArguments.

"[" points to row level tokens aka. field tokens, so for example things like "[title]". This is coming from

      $tokens = isset($this->rowTokens[$row_index]) ? $this->rowTokens[$row_index] : array();

What we both refer as "global tokens" are just available for area plugins, meh, its kind of a mess.

xjm’s picture

Oh ah, I completely forgot the [my_other_field_value] tokens. Is that "row-level" tokens? Edit: Yes, per above. Maybe that is a third method or?

xjm’s picture

I guess we should add hasRowToken() and maybe drop hasGlobalToken(). So in FieldPluginBase:

 foreach ($this->view->display_handler->getHandlers('field') as $field => $handler) {
      if (isset($handler->last_render)) {
        $tokens["[$field]"] = $handler->last_render;
      }

Are there any hard limitations on what the field handler identifiers there ($field) can be? Alphanumeric & underscore (or is that just convention)? Does the plugin system limit that (or does Views)?

Edit: i.e., is the regex something less than /\[.*\]/.

dawehner’s picture

I guess we should add hasRowToken() and maybe drop hasGlobalToken(). So in FieldPluginBase:

Well, ideally we still use that maybe to validate the tokens of the global ones for the area?

Are there any hard limitations on what the field handler identifiers there ($field) can be? Alphanumeric & underscore (or is that just convention)? Does the plugin system limit that (or does Views)?

This is a good question! By default views uses the key in hook_views_data. In case an item already exists ViewExecutable::generateHandlerId is used,
so its pretty much [a-z_0-9]+ I would say?

xjm’s picture

Mmmm also:

 protected function getTokenValuesRecursive(array $array, array $parent_keys = array()) {
    $tokens = array();

    foreach ($array as $param => $val) {
      if (is_array($val)) {
        // Copy parent_keys array, so we don't affect other elements of this
        // iteration.
        $child_parent_keys = $parent_keys;
        $child_parent_keys[] = $param;
        // Get the child tokens.
        $child_tokens = $this->getTokenValuesRecursive($val, $child_parent_keys);
        // Add them to the current tokens array.
        $tokens += $child_tokens;
      }
      else {
        // Create a token key based on array element structure.
        $token_string = !empty($parent_keys) ? implode('_', $parent_keys) . '_' . $param : $param;
        $tokens['%' . $token_string] = strip_tags(String::decodeEntities($val));
      }
    }

    return $tokens;
  }

(Looking particularly at $tokens['%' . $token_string] = strip_tags(String::decodeEntities($val));.)

xjm’s picture

By default views uses the key in hook_views_data.

Does this actually limit it to [a-z_0-9]+? For automatic relationships this would I guess depend on the DB driver's limitations. http://dev.mysql.com/doc/refman/5.0/en/identifiers.html is a lot more permissive than one would expect. And for exposing relationships, it almost looks like there's nothing to prevent you from giving it any crazy string name...

This is turning into one of those questions I wish I hadn't thought to ask. :P

dawehner’s picture

Urgs you are right, it could be quite a lot. Let's just go with your previous regex, ... its kinda good enough :)


    // Get flattened set of tokens for any array depth in query parameters.
    $tokens += $this->getTokenValuesRecursive($this->view->getRequest()->query->all());

This is the code calling this ... so we also have all query attributes available, ... its great to learn something new every day.

On top of that, we also have the self tokens. What are self tokens, ... those are like row level tokens, but for the rendering
within a single field. For fieldapi fields it used to be the raw data and the actual rendered data, for example. This tokens could be just
used inside the field itself.

The last submitted patch, vdc-hastoken.patch, failed testing.

xjm’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2396793: Token API will match tokens with empty types or tokens, e.g. [:invalid]
FileSize
5.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php. View

The syntax error is pre-5.5 only; took me a bit of head-scratching to remember that empty() was a bit precious.

PHP Fatal error: Can't use method return value in write context in core/modules/views/src/Plugin/views/style/StylePluginBase.php on line 267

Good thing we have 5.4 bots, I guess.

FWIW the Token API only cares that we have some non-space-containing key followed by a colon followed by anything between square brackets, so apparently a permissive regex isn't too crazy:

      \[             # [ - pattern start
      ([^\s\[\]:]*)  # match $type not containing whitespace : [ or ]
      :              # : - separator
      ([^\[\]]*)     # match $name not containing [ or ]
      \]             # ] - pattern end

(That however will match even [:] which looks like a bug. Filed #2396793: Token API will match tokens with empty types or tokens, e.g. [:invalid].)

Status: Needs review » Needs work

The last submitted patch, 15: vdc-token-2396607-15.patch, failed testing.

xjm’s picture

FileSize
5.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,788 pass(es). View
5.5 KB

Let's try that again. (TIL that running all phpunit tests doesn't hit syntax errors in TokenizeAreaPluginBase.)

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

FileSize
8.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,802 pass(es). View
4.3 KB

Hey look, when we write unit tests, we find bugs in our code.

xjm’s picture

Status: Needs review » Needs work

NW for the rest of the tests.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
12.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,782 pass(es), 43 fail(s), and 229 exception(s). View
7.58 KB

Now with the rest of the tests.

Status: Needs review » Needs work

The last submitted patch, 22: vdc-token-2396607-22.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -113,6 +122,36 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, Token $token_utility) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition);
    +    $this->tokenUtility = $token_utility;
    +  }
    

    Note: the Serializer style also overrides the constructor

  2. +++ b/core/modules/views/tests/src/Unit/Plugin/style/StylePluginBaseTest.php
    @@ -0,0 +1,193 @@
    +class StylePluginBaseTest extends UnitTestCase {
    

    <3

  3. +++ b/core/modules/views/tests/src/Unit/Plugin/style/StylePluginBaseTest.php
    @@ -0,0 +1,193 @@
    +  /**
    +   * @var \Drupal\Core\Cache\CacheBackendInterface|\PHPUnit_Framework_MockObject_MockObject
    +   */
    +  protected $cache;
    +
    +  /**
    +   * @var \Drupal\Core\Language\LanguageManagerInterface|\PHPUnit_Framework_MockObject_MockObject
    +   */
    +  protected $languageManager;
    +
    +  /**
    +   * @var \Drupal\Core\Extension\ModuleHandlerInterface|\PHPUnit_Framework_MockObject_MockObject
    +   */
    +  protected $moduleHandler;
    ...
    +    $this->tokenUtility = new Token($this->moduleHandler, $this->cache, $this->languageManager);
    

    Note: ideally a unit test would mock the token class, so this test is independent of the implementation of the other one. it would also mean less mocking crazyness here

  4. +++ b/core/modules/views/tests/src/Unit/Plugin/style/StylePluginBaseTest.php
    @@ -0,0 +1,193 @@
    +  /**
    +   * @var \Drupal\Core\Language\LanguageInterface|\PHPUnit_Framework_MockObject_MockObject
    +   */
    +  protected $language;
    

    c&p? Its not used at all

dawehner’s picture

I guess my druplicon message would be too short, here is an actual example:

    $this->format = $this->getMockBuilder('\Drupal\filter\Entity\FilterFormat')
      ->disableOriginalConstructor()
      ->getMock();
    $this->format->expects($this->any())
      ->method('getFilterTypes')
      ->will($this->returnValue(array(FilterInterface::TYPE_HTML_RESTRICTOR)));
xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

So reading the Twig docs, all sorts of craziness is possible with the {{ }} operators, e.g.:
{{ "foo #{1 + 2} baz" }}

What do we actually want to support in Views? Should we restrict it to only {{ token_name }} or pass through any old code to Twig?

I'll ask some of the Twig folks to give feedback

dawehner’s picture

As said on IRC I think the code we have is fine.

So this is about converting available tokens, to be passed along to twig.
In FieldPluginBase::getRenderTokens() we generate the available tokens. For UI purposes we show tokens with "{{ }}" around them,
also see

      // Setup the tokens for fields.
      $previous = $this->getPreviousFieldLabels();
      foreach ($previous as $id => $label) {
        $options[t('Fields')]["{{ $id }}"] = substr(strrchr($label, ":"), 2 );
      }
      // Add the field to the list of options.
      $options[t('Fields')]["{{ {$this->options['id']} }}"] = substr(strrchr($this->adminLabel(), ":"), 2 );

Views simply provides this limited amount of tokens, so it never happens that you end up with {{ "foo #{1 + 2} baz" }}
in that piece of code.

Afaik though if also foo and bar are available the inline_template should be able to use it, shouldn't it?

xjm’s picture

So @dawehner, @Cottser, and I discussed this earlier. Any old Twig syntax you enter in a Views field is evaluated, e.g.:

(a) {{ title }}

(b) {{ title ~ "#{1 + 2}" ~ title_1 }}

(c) {{ title }} {{ "#{1 + 2}" }}  {{ title_1 }}

will render as:

(a) my title (b) my title 3my title (c) my title 3 my title

And if you enter invalid Twig syntax, the View will throw an exception, e.g.

[20-Jan-2015 18:26:46 Europe/Berlin] Uncaught PHP Exception Twig_Error_Syntax: "Unexpected character "#" in "{{ title }}

{{ title #{1 + 2} title_1 }}

{{ title }} {{ #{1 + 2} }}  {{ title_1 }}
" at line 3" at /Applications/MAMP/htdocs/d8git/core/vendor/twig/twig/lib/Twig/Lexer.php line 284

Cottser tested and one thing you can't do is nest double curlies, i.e.
{{ foo {{ invalid }} bar }}
So for this issue, I'll match anything-not-double-curlies in the regex, but we probably want to figure out whether we want to support any and all Twig code, or want to restrict it to simple tokens. One possibility Cottser suggested:

so I think one possibility anyway would be to just evaluate what gets put in the little text field as an inline twig template, and pass the values of the tokens in as variables for that inline twig template

Cottser’s picture

Issue tags: +Twig

Thanks for writing that up @xjm! I just want to make sure it's clear that the double/nested curlies is invalid Twig in the first place so it's not a limitation imposed by the current implementation :)

Cottser’s picture

Also we could potentially evaluate whether the input is valid Twig before saving (as part of validation):

try {
  $environment->parse($environment->tokenize($templateCode));
} catch (\Twig_Error_Syntax $e) {
  // Display validation error.
}

(via https://groups.google.com/forum/#!topic/twig-users/idoJ0tJSZKY).

The sandbox mode might be interesting to consider with inline templates in general to limit the scope of functionality: http://twig.sensiolabs.org/doc/api.html#sandbox-extension

xjm’s picture

Status: Needs work » Needs review
FileSize
8.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,190 pass(es). View
10.37 KB

Here's a patch for #29. Edit: Forgot that I was going to move these methods to PluginBase so we can use them in PluginBase::viewsTokenReplace(). Doing that now.

I think #31 might make sense as part of #2392823: [meta] Much Views UI input is not validated or whatever followup we make to sort what Twig should do. Thanks @Cottser.

xjm’s picture

Parent issue: » #2342287: Allow Twig in Views token replacement
FileSize
12.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,180 pass(es). View
15.89 KB

Okay, so upon further inspection researching #2342287: Allow Twig in Views token replacement, all of our token handling logic is even more all over the place than I thought it was before!

  1. Why do field plugins have an interface but not display plugins etc.? (Out of scope, just wondering if there's method to the madness. Pun not intended.) ;)
  2. DisplayPluginBase::getArgumentsTokens() is not quite English. Renamed to getArgumentTokens().
  3. FieldPluginBase::getRenderTokens() needlessly duplicated logic in DisplayPluginBase::getArgument[s]Tokens(). Fixed to just call that method, since it already needed to use the display handler anyway.
  4. Changed the names of getArgToken() and getTwigToken() to match those existing method names on DisplayPluginBase and FieldPluginBase.
  5. Also moved the methods to PluginBase, so that they can be used in PluginBase::viewsTokenReplace().
  6. StylePluginBase::tokenizeValue() now seems slightly redundant, misnamed, and out of place, but I haven't addressed that yet.
  7. I also removed the global token checking from my patch for now because I thought only the entity area handlers supported them... but then there are a bunch of global token methods on PluginBase too. I've left those alone for now as they are using calls to \Drupal::token() and that should be injected those places too. Constructor injection would seem kinda noisy/disruptive; I wonder if it could use setter injection or something?
xjm’s picture

xjm’s picture

Issue summary: View changes
dawehner’s picture

Why do field plugins have an interface but not display plugins etc.? (Out of scope, just wondering if there's method to the madness. Pun not intended.) ;)

You know, things are a step by step process.

DisplayPluginBase::getArgumentsTokens() is not quite English. Renamed to getArgumentTokens().

Mh? So we want to express that its getting all tokens from all arguments :) Btw. I also trust earls english.

I also removed the global token checking from my patch for now because I thought only the entity area handlers supported them... but then there are a bunch of global token methods on PluginBase too. I've left those alone for now as they are using calls to \Drupal::token() and that should be injected those places too. Constructor injection would seem kinda noisy/disruptive; I wonder if it could use setter injection or something?

Setter injection on the plugin manager level could work, but yeah you still want to make it optional.

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -319,13 +319,68 @@ public function globalTokenReplace($string = '', array $options = array()) {
+  public function hasArgumentToken($value) {

Seriously, given how much methods we have, I think we should extract them and don't rely on inheritance. We could introduce a new object, maybe make it available on the view executable and use it. I hate to place more and more and more into the base class.

xjm’s picture

Seriously, given how much methods we have, I think we should extract them and don't rely on inheritance. We could introduce a new object, maybe make it available on the view executable and use it. I hate to place more and more and more into the base class.

Agreed, that makes a lot of sense I think and will clean up the tangle. So... any suggestion on what said object is called and where it lives? ViewsTokenSomething?

mikeker’s picture

In answer to your earlier question , I think the tokens that Views supplies should be simple tokens -- strings and arrays -- that can be used in the replacement text in any way the site builder wants. I think I'm saying the same thing that @dawehner said in #28.

One issue, though, is that Views' rewrite text is passed through Xss::filterAdmin() which encodes '<' and '>', otherwise valid in in Twig blocks. See #2055597: Allow multi-value field rendering to pass through Views rewrite only once, specifically comment #11.

(TL/DR: While not (yet) currently available, multi-valued Views tokens would be hugely useful to site builders. But filterAdmin() encodes the '>' in {% if field_tags_multiple|length > 1 %} which raise a compile error in Twig and would cripple their usefulness.)

I'm pretty sure that's out of scope for this issue, but I thought I'd bring it up in case I'm wrong. :)

xjm’s picture

FileSize
20.3 KB
33.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/area/Title.php. View

Thanks @mikeker!

Here's a start at moving the token functionality into its own object with some more central documentation. I added some @todo on the class for the numerous other methods in various Views plugins that could do with some refactoring as well. No interdiff because it's not helpful with all the moved code. (Need to add back the unit test coverage in a unit test for the new class still.) Few questions:

  1. One thing I'm still not entirely clear on is the %1 vs. !1 for argument tokens. I believe @dawehner explained that the ! tokens remove markup for places that can't have markup, like a path, but still a bit confused. In String::format() etc., ! denotes the raw value and % denotes the sanitized value -- but I think Views is doing the reverse, stripping markup from the ! version:
          $tokens["!$count"] = isset($this->view->args[$count - 1]) ? strip_tags(String::decodeEntities($this->view->args[$count - 1])) : '';
    

    Also, this is what it says in the UI:

    The difference between "title" and "input" is not clear to me.

    Part of what I'd like to do is add methods specific to ! and % and trying to figure out what those methods should be called.

  2. Another thing I've not figured out yet is why the values for the array of tokens are empty, e.g. in:
          if (!isset($tokens["%$count"])) {
            $tokens["%$count"] = '';
          }
    
  3. Also unclear on FieldPluginBase::getTokenValuesRecursive(). The documentation for what it does is very clear but I don't entirely understand why we do it. :) It appears to be a feature of argument matching that I never knew about maybe? Also it's specific to argument tokens, but yet DisplayPluginBase::getArgumentTokens() doesn't use it.
  4. In working on this, I noticed that TokenizeAreaPluginBase::tokenForm() did not get updated for #2342287: Allow Twig in Views token replacement so we have a regression there I think.

Status: Needs review » Needs work

The last submitted patch, 39: vdc-token-2396607-39.patch, failed testing.

mikeker’s picture

#4 leads us into the issue I didn't want to raise in #2342287: Allow Twig in Views token replacement, but since you brought it up... :)

The header/footer (and perhaps other) areas include Drupal tokens (aka: core tokens, such as site-name), while field rewrites only include row-based tokens (aka Views' tokens). Which leads me to my secret mission: provide Twig tokens and Twig replacements for all token fields in core.

mikeker’s picture

Status: Needs work » Needs review
FileSize
33.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,566 pass(es), 569 fail(s), and 289 exception(s). View
605 bytes

Fixes typo in #39.

Status: Needs review » Needs work

The last submitted patch, 42: vdc-tokens_2396607-41.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
33.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/area/Title.php. View
607 bytes

I really seem incapable of rolling a patch this morning.

Status: Needs review » Needs work

The last submitted patch, 45: vdc-token-2396607-46.patch, failed testing.

xjm’s picture

FileSize
33.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,045 pass(es), 302 fail(s), and 82 exception(s). View

With #42...

xjm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 47: vdc-token-2396607-47.patch, failed testing.

mikeker’s picture

+++ b/core/modules/views/src/TokenParser.php
@@ -0,0 +1,249 @@
+        // @todo Do this with the regex instead.
+        $token = trim(str_replace(array('{', '}'), '', $token));
+        $twig_tokens[$token] = $replacement;

I assume the @todo suggestion is a concern about zapping valid curly brackets that are not token delimiters? Perhaps a more surgical option than str_replace would be

$token = trim($token, "{} \t\n\r\0\x0B");

(" \t\n\r\0\x0B" is the default value for the second param.)

My only objection to using preg_replace is performance, which is a minor objection at best.

xjm’s picture

Status: Needs work » Needs review
FileSize
33.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,083 pass(es), 272 fail(s), and 61 exception(s). View

@alexpott says this voodoo trait is necessary to make D8 work.

xjm’s picture

xjm’s picture

I assume the @todo suggestion is a concern about zapping valid curly brackets that are not token delimiters? Perhaps a more surgical option than str_replace would be

$token = trim($token, "{} \t\n\r\0\x0B");

(" \t\n\r\0\x0B" is the default value for the second param.)

My only objection to using preg_replace is performance, which is a minor objection at best.

Since a given view typically will not have more than 5-20 field tokens, I think it's probably not needed to micro-optimize for that. Clever solution though! In either case I'm probably going to abstract it into its own method so we can change it as needed.

Status: Needs review » Needs work

The last submitted patch, 51: vdc-token-2396607-50.patch, failed testing.

mikeker’s picture

Since a given view typically will not have more than 5-20 field tokens, I think it's probably not needed to micro-optimize for that.

True, but this is run on each field that's rewriten in each row displayed in a view. That could start to add up, though I have no numbers to back that up. Regardless,

I'm probably going to abstract it into its own method so we can change it as needed

is a good idea and let's us fiddle with it later.

mikeker’s picture

I was trying to look into the testbot errors and ran into something strange... getTokenParser() was returning NULL. Somehow ViewExecutable was being instantiated, but __construct() wasn't being called? Since ViewExecutable isn't extended, I have no idea how that can happen...

This only happens in the ViewsUI preview, not when rendering a the view for real.

mikeker’s picture

Status: Needs work » Needs review
FileSize
32.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,306 pass(es), 112 fail(s), and 82 exception(s). View
574 bytes

This will probably help with a lot of the testbot failures... :)

Status: Needs review » Needs work

The last submitted patch, 57: vdc-token_2396607-57.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
33.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,298 pass(es), 111 fail(s), and 82 exception(s). View

This should fix one more test...

I think the remaining test failures are actually related to #56. There is some way to instantiate a ViewExecutable without passing it a Token.

Status: Needs review » Needs work

The last submitted patch, 59: vdc-token_2396607-59.patch, failed testing.

mikeker’s picture

Issue tags: +Needs reroll

#59 no longer applies....

AjitS’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +#drupalgoa2015
FileSize
34.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Just rerolling for now.

Status: Needs review » Needs work

The last submitted patch, 62: allow_views_token-2396607-62.patch, failed testing.

AjitS’s picture

Status: Needs work » Needs review
FileSize
655 bytes
34.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Implemented the missing function from the implemented interface.

AjitS’s picture

FileSize
2.41 KB
36.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/display/DisplayPluginBase.php. View

And renamed getArgumentsTokens() to getArgumentTokens() as mentioned in the issue summary.

The last submitted patch, 64: allow_views_token-2396607-64.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 65: allow_views_token-2396607-65.patch, failed testing.

AjitS’s picture

Status: Needs work » Needs review
FileSize
654 bytes
36.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

The function was already present. Just needed the rename to be done. Which is why the PHP error. Retrying.

Status: Needs review » Needs work

The last submitted patch, 68: allow_views_token-2396607-68.patch, failed testing.

mikeker’s picture

FileSize
37.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.08 KB

Working my way through testbot failures. This cleans up some namespacing issues...

mikeker’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: 2396607-70-vdc-token-cleanup.patch, failed testing.

mikeker’s picture

OK, #70 seems to get us back to where we were in #59...

But the remaining failures are weird. When instantiated through the ViewsUI class, the ViewsExecutable class doesn't have a TokenParser associated with it. To repro the error: create a new view with page display and all the default settings, then click the Preview button and look at the resulting AJAX.

mikeker’s picture

Status: Needs work » Needs review
FileSize
37.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
793 bytes

What takes me hours (and gets me nowhere) takes @dawehner mere seconds in IRC...

Let's see what the testbot thinks about this one.

Status: Needs review » Needs work

The last submitted patch, 74: 2396607-74-vdc-token-cleanup.patch, failed testing.

The last submitted patch, 74: 2396607-74-vdc-token-cleanup.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
37.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
0 bytes

Yuck. The 70k patch in #2409209: Replace all _url() calls beside the one in _l() and the 38k patch in this issue both added a fourth param to ViewExecute::__construct(). So merging one with the other resulted in a silent fail -- at least until the test were run.

<rant>This is why I really hate our anachronistic, patch-based system. If there was a history to either of these changes, it would be easy to narrow down the change to a given commit (assuming the devs involved were good about committing early and often). Instead we're left with resolving the difference between two large patches.</rant> ... Sorry. Back to our topic at hand.

I think I've gotten everything straightened out. Let's see what testbot thinks.

mikeker’s picture

FileSize
6.94 KB

Not sure what happened to the interdiff...

Status: Needs review » Needs work

The last submitted patch, 78: 2396607-78-vdc-token-cleanup.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
37.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.01 KB

Ack, missed the ViewExecutableFactory class...

Status: Needs review » Needs work

The last submitted patch, 81: 2396607-81-vdc-token-cleanup.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
38.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
4.08 KB

Here's the rest of the ViewExecutableFactory that I managed to miss on my previous attempt. Interdiff against #78 since #81 was only half the fix...

This should bring us down to just a couple of test failures. Sorry about the noise -- I'm going to bed soon. Promise.

Status: Needs review » Needs work

The last submitted patch, 83: 2396607-83-vdc-token-cleanup.patch, failed testing.

Cottser’s picture

Assigned: xjm » Unassigned

I turned my comment in #31 into a sub-issue of the meta: #2457257: Use Twig parser to detect errors in Twig code entered in Views UI

I'm guessing @xjm doesn't want/needs this assigned any longer, so unassigning.

The last submitted patch, 83: 2396607-83-vdc-token-cleanup.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
44.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
6.28 KB

Adds tests for the new TokenParser class: unit tests for hasTokens() and functional test for viewsTokenReplace(). Haven't cleaned up any of the existing test failures so I don't expect this patch to pass but running against the testbot to ensure the newly added tests don't fail either.

mikeker’s picture

@Cottser: Thanks for adding the followup for #31 -- otherwise that would've gotten lost in all the refactoring!

Status: Needs review » Needs work

The last submitted patch, 88: 2396607-88-vdc-token-cleanup.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
45.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
607 bytes

Reroll of #88. Plus it adds the token parser back to the ViewExecutable object when unserializing. That should fix the BulkFormTest that was failing and maybe the remaining ones.

Let's see what the testbot says...

Status: Needs review » Needs work

The last submitted patch, 91: 2396607-91-vdc-token-cleanup.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jonathanshaw’s picture

Refactoring here should probably also consider the implications of #2665766: {% %} twig syntax is only parsed in area handler when a {{ }} token is present, and that twig can be used more than token replacement.