Allow Views token matching for validation (and remove dead code)

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.

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.

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?

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.

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

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)));

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?

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 :)

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

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.

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. :)

#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.

Fixes typo in #39.

+++ 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.

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.

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.

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

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.

#59 no longer applies....

Just rerolling for now.

Implemented the missing function from the implemented interface.

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

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

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

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.

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

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

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.

Not sure what happened to the interdiff...

Ack, missed the ViewExecutableFactory class...

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.

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.

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.

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

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

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