Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Hmm, If we move this, to AreaPluginBase we will have to unset this in the option defintions for view, result, and title handlers.

Maybe we should go with a TextAreaPluginBase class instead?

dawehner’s picture

Or we just mark this issue as won't fix, and people should take care of themself.

What we could do is to store nothing if no tokenize form was set.

derhasi’s picture

Issue tags: +SprintWeekend2013

I'd like to implement a TokenizeAreaPluginBase(), that could provide the relevant parts, not even for text. I'll give it a try now ;)

derhasi’s picture

Status: Active » Needs review
FileSize
9.79 KB

So, I guess I made it work , so here's the patch.

dawehner’s picture

That's totally a great idea and allows to have a clean base class. Here are some smaller points.
As a follow up we could think about the "tokens" provided by the result area handler but also some support for the other tokens in the result handler.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,122 @@
+ * Definition of Drupal\views\Plugin\views\area\TokenizeAreaPluginBase.

1354 tells you that it should be "Contains \..." now. I'm sorry that we don't have converted all the other places.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,122 @@
+

That's one empty line too much.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,122 @@
+   * Overrides \Drupal\views\Plugin\views\PluginBase::defineOptions().
...
+   * Overrides \Drupal\views\Plugin\views\HandlerBase::buildOptionsForm().

Aren't we overriding AreaPluginBase::defineOptions here? The same for buildOptionsForm?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,122 @@
+  public function tokenForm(&$form, &$form_state) {

Is there a reason why this method is not protected? I can't think of a reason that you want to call this method from outside.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,122 @@
+    if (isset($form['global_tokens'])) {

Is there a reason why globalTokenForm should not set a global token form element?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,122 @@
+  public function tokenizeValue($value) {

This also seems to be not needed to be public

derhasi’s picture

I followed most of your suggested changes and updated the patch.
The public tokenizeValue() makes sense to me, as the display plugin may use the tokenize functionality of the area handler, as well as the area handler can use the one of the style plugin.

dawehner’s picture

@todo: Convert the entity area handler as well.

derhasi’s picture

Reworked patch.

  • Converted the Entity Area Handler.
  • Small fix in entity area handler test label
  • Additional comments on edited methods.
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for this nice work!

damiankloip’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, a couple o' things....

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined
@@ -40,20 +40,23 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+    // Per default we enable tokenize, as this is the most common use case for
+    // this handler.
+    $options['tokenize']['default'] = TRUE;
...
-    $options['tokenize'] = array('default' => TRUE, 'bool' => TRUE);

Not sure why that has been added? this is inherited from the TokenizeAreaPluginBase class now?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,119 @@
+ * the tokens of the first view result. The option form to enable that

Not just the first view result, so maybe we can remove this or say it provides views and global token support or soemthing.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,119 @@
+ * functionality is automatically added to

s/to/too.?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,119 @@
+  /**
+   * Helper to tokenize a value.
+   *
+   * @param string $value
+   *   The value to eventually tokenize.
+   *
+   * @return string
+   *   Tokenized value if tokenize option is enabled.

Can this be something like "Replaces tokens for a value" instead? Not sure about helper... Also the @return isn't correct as you will still get token replacement without this option, just not views token replacements.

derhasi’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
14.6 KB

@damian, I adjusted the patch with your suggested additions.

On the first point: by default the tokenization is off, but the entity area handler had it enabled by default, so we did not want to switch that behaviour and do a direct "port". I guess the comment on the "override" should make that clear enough.

Would be great you could review it again ;)

damiankloip’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined
@@ -40,20 +40,23 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
 
+    // Per default we enable tokenize, as this is the most common use case for
+    // this handler.
+    $options['tokenize']['default'] = TRUE;

Oh, sorry, I get it. This is actually overridding the default of FALSE. Heh, brain dead moment... I still think we shouldn't have this override but I guess @dawehner thinks it's better this way?

the rest of the changes look good!

derhasi’s picture

Damian, I thought so first too, but Daniel convinced me to alter the default to TRUE, as this is the case most people will use it. So for usability's sake this should stay TRUE imho.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Well, none of the others are, but I guess this makes sense. Plus, I trust Daniel :)

dawehner’s picture

LOL

xjm’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, views-tokenizerAreaPluginBase-1910700-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
damiankloip’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +VDC, +SprintWeekend2013

The last submitted patch, views-tokenizerAreaPluginBase-1910700-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.69 KB

Rerolled the test.

dawehner’s picture

Issue tags: -Novice, -VDC, -SprintWeekend2013

#21: durpal-1910700-21.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +VDC, +SprintWeekend2013

The last submitted patch, durpal-1910700-21.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.7 KB

Let's rerole.

Status: Needs review » Needs work

The last submitted patch, tokenize-1910700-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
855 bytes
14.7 KB

It's green now.

damiankloip’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined
@@ -37,20 +36,23 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+    // Per default we enable tokenize, as this is the most common use case for

Maybe the first part should be "Enable tokenize by default, ..."

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Text.phpundefined
@@ -16,16 +16,21 @@
+   * Overrides \Drupal\views\Plugin\views\area\TokenizeAreaPluginBase::defineOptions().
...
+   * Overrides \Drupal\views\Plugin\views\area\TokenizeAreaPluginBase::buildOptionsForm().

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TextCustom.phpundefined
@@ -16,15 +16,20 @@
+   * Overrides \Drupal\views\Plugin\views\area\TokenizeAreaPluginBase::defineOptions().
...
+   * Overrides \Drupal\views\Plugin\views\area\TokenizeAreaPluginBase::buildOptionsForm().

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,114 @@
+   * Overrides \Drupal\views\Plugin\views\area\AreaPluginBase::defineOptions().
...
+   * Overrides \Drupal\views\Plugin\views\area\AreaPluginBase::buildOptionsForm().

{@inheritdoc} all the things

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,114 @@
+   * Form helper function to add tokenization form elements.

Maybe this should be 'Adds tokenization to form elements'

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.phpundefined
@@ -0,0 +1,114 @@
+      // @todo: add an option to select the row?

Not sure about this? I think we can remove it. If people could choose a row, this would lead to a world of pain. Plus the row could change at any time.

dawehner’s picture

FileSize
3.35 KB
14.23 KB

Great

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Nice.

alexpott’s picture

Title: AreaPluginBase does not define "tokenize", but Text does. » Implement TokenizeAreaPluginBase()
Status: Reviewed & tested by the community » Fixed

Committed c7a11cc and pushed to 8.x. Thanks!

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