In Text::defineOptions() the tokenize option is declared, though the form is in AreaPluginBase.

Let's move the defineOptions() entry to AreaPluginBase

Files: 
CommentFileSizeAuthor
#28 drupal-1910700-28.patch14.23 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,077 pass(es).
[ View ]
#28 interdiff.txt3.35 KBdawehner
#26 drupal-1910700-26.patch14.7 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,044 pass(es).
[ View ]
#26 interdiff.txt855 bytesdawehner
#24 tokenize-1910700-24.patch14.7 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#21 durpal-1910700-21.patch14.69 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch durpal-1910700-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 views-tokenizerAreaPluginBase-1910700-11.patch14.6 KBderhasi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-tokenizerAreaPluginBase-1910700-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 interdiff.txt1.62 KBderhasi
#8 views-tokenizerAreaPluginBase-1910700-8.patch14.36 KBderhasi
PASSED: [[SimpleTest]]: [MySQL] 53,044 pass(es).
[ View ]
#8 interdiff.txt6.69 KBderhasi
#6 views-tokenizerAreaPluginBase-1910700-6.patch9.75 KBderhasi
PASSED: [[SimpleTest]]: [MySQL] 53,131 pass(es).
[ View ]
#6 interdiff.txt2.03 KBderhasi
#4 views-tokenizerAreaPluginBase-1910700-4.patch9.79 KBderhasi
PASSED: [[SimpleTest]]: [MySQL] 52,579 pass(es).
[ View ]

Comments

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?

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.

Issue tags:+#SprintWeekend

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

Status:Active» Needs review
StatusFileSize
new9.79 KB
PASSED: [[SimpleTest]]: [MySQL] 52,579 pass(es).
[ View ]

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

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

StatusFileSize
new2.03 KB
new9.75 KB
PASSED: [[SimpleTest]]: [MySQL] 53,131 pass(es).
[ View ]

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.

@todo: Convert the entity area handler as well.

StatusFileSize
new6.69 KB
new14.36 KB
PASSED: [[SimpleTest]]: [MySQL] 53,044 pass(es).
[ View ]

Reworked patch.

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

Status:Needs review» Reviewed & tested by the community

Thanks for this nice work!

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.

Status:Needs work» Needs review
StatusFileSize
new1.62 KB
new14.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-tokenizerAreaPluginBase-1910700-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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.

Status:Needs review» Reviewed & tested by the community

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

LOL

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review

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

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

Status:Needs work» Needs review
StatusFileSize
new14.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch durpal-1910700-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled the test.

Issue tags:-Novice, -VDC, -#SprintWeekend

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

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

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

Status:Needs work» Needs review
StatusFileSize
new14.7 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's rerole.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new855 bytes
new14.7 KB
PASSED: [[SimpleTest]]: [MySQL] 56,044 pass(es).
[ View ]

It's green now.

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

StatusFileSize
new3.35 KB
new14.23 KB
PASSED: [[SimpleTest]]: [MySQL] 56,077 pass(es).
[ View ]

Great

Status:Needs review» Reviewed & tested by the community

Nice.

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.