Posted by damiankloip on October 10, 2012 at 2:13pm
7 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs reroll, Novice, VDC |
Issue Summary
Currently the tokenize code live in the Text area handler, we should think about moving this (or atleast a helper) into AreaPluginBase to use in Text, TextCustom, and the new title handler in #1808542: Allow area handlers to override the page title when used in an empty area.. TextCustom should then extend AreaPluginBase and not Text.
So do we just want this in the buildOptionsForm method or have this code in a helper method?
Comments
#1
tagging
#2
I think the best way to do this would be probably to actually move this into a global helper method because token form help might be helpful on:
#3
I was talking to tim.plunkett yesterday and agreed this would be good to just move into AreaPluginBase first, then move this later if we need to. I am also working on actual token integration too, so we will see. Either way, I think this should move :)
I was thinking something like this...
#4
#5
+++ b/lib/Drupal/views/Plugin/views/area/AreaPluginBase.phpundefined
@@ -92,6 +91,60 @@ abstract class AreaPluginBase extends HandlerBase {
+ * Form Helper function to add tokenization form elements.
+++ b/lib/Drupal/views/Plugin/views/area/TextCustom.phpundefined
@@ -18,21 +18,26 @@ use Drupal\Core\Annotation\Plugin;
- unset($form['content']['#wysiwyg']);
I guess Helper should be lowercase here, maybe the description could make it clear that the form is altered and not returned, but i don't care that much about that.
+++ b/lib/Drupal/views/Plugin/views/area/AreaPluginBase.phpundefined@@ -92,6 +91,60 @@ abstract class AreaPluginBase extends HandlerBase {
+ $output = '<p>' . t('The following tokens are available. If you would like to have the characters \'[\' and \']\' please use the html entity codes \'%5B\' or \'%5D\' or they will get replaced with empty space.' . '</p>');
...
+ $output .= theme('item_list',
...
+ '#value' => $output,
I know this is sort of out of scope, but couldn't we all this just replace by proper render arrays?
#6
This is untested, but how about this?
#7
+++ b/lib/Drupal/views/Plugin/views/area/AreaPluginBase.phpundefined@@ -92,6 +91,59 @@ abstract class AreaPluginBase extends HandlerBase {
+ $form['token_help']['markup'] = array(
That's nice, what about help instead?
#8
Fine! ;) How about this?
#9
Awesome!
#10
#11
I attached one patch with the reroll.
#12
This is fine, it was RTBC before....
#13
.
#14
All of this looks very straight-forward, apart from this:
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/TextCustom.phpundefined@@ -18,21 +18,26 @@
-class TextCustom extends Text {
+class TextCustom extends AreaPluginBase {
Like the review I did this weekend, the old code makes a ton more sense to me. I have no idea what an "AreaPluginBase" is, and the PHPDoc for that class tells me:
/*** Base class for area handlers.
*
* @ingroup views_area_handlers
*/
abstract class AreaPluginBase extends HandlerBase {
Not. Helpful. :)
I asked damian on IRC and he said:
"An area plugin is the base plugin class we use for Header, footer, empty areas in views."
That's much more helpful, though I'm not sure I would call a thing with such a description an "area plugin."
ANYWAY.
None of that is related to the patch at hand, which looks fine to me now that I understand what an AreaBasePlugin is (this is just moving the token integration into the base class so all derivations can use it), so...
Committed and pushed to 8.x.
But let's get a follow-up to both document and possibly rename AreaPluginBase cos that shit is whack. :)
#15
Yep, properly documenting the base plugin and handler types is our next priority for documentation. It might be after Dec. 1 but it's also one of the few documentation tasks I'm okay with doing before then and it's my next priority. :) There isn't really a better name for it I don't think, but it does need docs to say WTF it is.
#16
Automatically closed -- issue fixed for 2 weeks with no activity.