Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
In Text::defineOptions() the tokenize option is declared, though the form is in AreaPluginBase.
Let's move the defineOptions() entry to AreaPluginBase
Comment | File | Size | Author |
---|---|---|---|
#28 | drupal-1910700-28.patch | 14.23 KB | dawehner |
#28 | interdiff.txt | 3.35 KB | dawehner |
#26 | drupal-1910700-26.patch | 14.7 KB | dawehner |
#26 | interdiff.txt | 855 bytes | dawehner |
#24 | tokenize-1910700-24.patch | 14.7 KB | dawehner |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedHmm, 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?
Comment #2
dawehnerOr 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.
Comment #3
derhasi CreditAttribution: derhasi commentedI'd like to implement a TokenizeAreaPluginBase(), that could provide the relevant parts, not even for text. I'll give it a try now ;)
Comment #4
derhasi CreditAttribution: derhasi commentedSo, I guess I made it work , so here's the patch.
Comment #5
dawehnerThat'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.
1354 tells you that it should be "Contains \..." now. I'm sorry that we don't have converted all the other places.
That's one empty line too much.
Aren't we overriding AreaPluginBase::defineOptions here? The same for buildOptionsForm?
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.
Is there a reason why globalTokenForm should not set a global token form element?
This also seems to be not needed to be public
Comment #6
derhasi CreditAttribution: derhasi commentedI 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.
Comment #7
dawehner@todo: Convert the entity area handler as well.
Comment #8
derhasi CreditAttribution: derhasi commentedReworked patch.
Comment #9
dawehnerThanks for this nice work!
Comment #10
damiankloip CreditAttribution: damiankloip commentedSorry, a couple o' things....
Not sure why that has been added? this is inherited from the TokenizeAreaPluginBase class now?
Not just the first view result, so maybe we can remove this or say it provides views and global token support or soemthing.
s/to/too.?
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.
Comment #11
derhasi CreditAttribution: derhasi commented@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 ;)
Comment #12
damiankloip CreditAttribution: damiankloip commentedOh, 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!
Comment #13
derhasi CreditAttribution: derhasi commentedDamian, 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.
Comment #14
damiankloip CreditAttribution: damiankloip commentedWell, none of the others are, but I guess this makes sense. Plus, I trust Daniel :)
Comment #15
dawehnerLOL
Comment #16
xjm#11: views-tokenizerAreaPluginBase-1910700-11.patch queued for re-testing.
Comment #18
dawehner#11: views-tokenizerAreaPluginBase-1910700-11.patch queued for re-testing.
Comment #19
damiankloip CreditAttribution: damiankloip commented#11: views-tokenizerAreaPluginBase-1910700-11.patch queued for re-testing.
Comment #21
dawehnerRerolled the test.
Comment #22
dawehner#21: durpal-1910700-21.patch queued for re-testing.
Comment #24
dawehnerLet's rerole.
Comment #26
dawehnerIt's green now.
Comment #27
damiankloip CreditAttribution: damiankloip commentedMaybe the first part should be "Enable tokenize by default, ..."
{@inheritdoc} all the things
Maybe this should be 'Adds tokenization to form elements'
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.
Comment #28
dawehnerGreat
Comment #29
damiankloip CreditAttribution: damiankloip commentedNice.
Comment #30
alexpottCommitted c7a11cc and pushed to 8.x. Thanks!