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.
Field render() functions have two parts:
1) Getting the value:
$value = $values->{$this->field_alias};
2) Doing the actual rendering (check options, sanitize, make links, whatever).
If a views contrib module (efq_views, search_api, whatever) needs to find $value differently, then it also needs to copy the whole rendering part.
Changing the render functions to look something like this:
function render($values, $value = NULL) {
if (!isset($value)) {
$value = $values->{$this->field_alias};
}
would allow contrib modules to reuse rendering logic and supply their own $value.
Prompted by: http://drupal.org/node/971122#comment-3725120
Thoughts?
Comment | File | Size | Author |
---|---|---|---|
#25 | 975400_6x_followup.patch | 5.07 KB | bojanz |
#25 | 975400_7x.patch | 55.82 KB | bojanz |
#24 | 975400_6x_followup.patch | 5.07 KB | bojanz |
#24 | 975400_7x.patch | 55.82 KB | bojanz |
#20 | 975400-fix_profile_date.patch | 1.08 KB | teliseo |
Comments
Comment #1
bojanz CreditAttribution: bojanz commentedBetter title.
Comment #2
dawehnerWhat about adding a render_field method.
* Changing the signature of render blows up the notices.
Comment #3
bojanz CreditAttribution: bojanz commentedI meant field as in views fields (in handlers/), the concrete use case is views_handler_field_date::render(), we don't need to touch views_handler_field_field at all.
Comment #4
bojanz CreditAttribution: bojanz commentedThis is what I had in mind.
Didn't touch the custom handlers in views/modules/* -> no need to.
Same for views_handler_field_field, it's a very different beast.
Also didn't touch views_handler_field_prerender_list::render() since it's marked deprecated (time to kill it?)
It almost applies to 6.x-3.x too, if it needs to go that way first.
Comment #5
dawehnerJust to document this. This doesn't produce notices.
Comment #6
bojanz CreditAttribution: bojanz commentedHere's an example usage, in efq_views, my custom date handler, instead of:
I just do:
All my handlers drop significant weight. And this is not just my module. Most custom backends change the "how to get data" part, while leaving the "how to render it" part alone.
Assigning to merlinofchaos, so that he can tell if he agrees with it, and if it should go through 6.x-3.x first.
Comment #7
bojanz CreditAttribution: bojanz commentedNote that I only changed the render() functions that actually do something. There are a few simple render() functions that I didn't touch, but they should probably be changed as well for consistnecy -> to avoid the question "why do some render() functions accept two params, and some only one".
Comment #8
fagoThis makes much sense to me, as it reduce the amount of code-needed to override a handler dramatically. Thus, it would help all the modules implementing other query backends a lot.
Comment #9
bojanz CreditAttribution: bojanz commentedDiscussed this with merlinofchaos.
New patch (against 6.x-3.x), new approach.
I tried declaring get_value($values) and sanitize_value($values).
Did an initial pass, patch attached.
However, when going through views/modules, I saw a bunch of handlers doing $values->{$this->aliases['uid']} and $values->{$this->aliases['nid']} etc etc.
This makes the existing solution not so consistent.
My thought is to have get_value($values, $field) -> where $field is $this->field_alias, or any other alias..., so that $values is never accessed directly. Thoughts?
Comment #10
dawehnerContinued work.
Grepped for "function render"
Comment #11
bojanz CreditAttribution: bojanz commentedI think it's ready now.
Comment #12
dawehnerThe interdiff is looking fine.
Comment #13
bojanz CreditAttribution: bojanz commentedMaybe this one will apply better.
Comment #14
merlinofchaos CreditAttribution: merlinofchaos commentedCommitted to D6.x-3.x -- marking for D7 porting, as I'm not sure I trust just blindly applying and committing.
Comment #15
drunken monkeySubscribing — looks like a really great DX feature!
Comment #16
dawehnerHere is a first patch for drupal7
Comment #17
bojanz CreditAttribution: bojanz commentedIncluded the fix from #1025696: SQL error in views/modules/user/views_handler_field_user_roles.inc on line 26..
Also, made the get_value() function match 6.x-3.x (Earl included a notice fix before committing).
This is where our sanitize_value function falls apart, it assumes only one way of sanitizing per handler, and in this case it has two. Perhaps define sanitize_value as check_url, use it in the ELSE, but leave the check_plain in IF intact? Or extend sanitize_value to support another argument like get_value?
Still haven't finished reviewing & testing, just wanted to upload progress before I leave the devdays.
Comment #18
tim.plunkettsub
Comment #19
teliseo CreditAttribution: teliseo commentedThe patch as committed to 6.x-3.x-dev (comment #14) breaks user profile fields of type date (they always render empty). Attached patch fixes this, and also changes user profile list fields to call
$this->get_value()
instead of indexing by$this->field_alias
.Comment #20
teliseo CreditAttribution: teliseo commentedSorry, my last patch had a stray character—please use this one instead.
Comment #21
dawehnerPlease create a new issue for the new bugs.
It's the only way to keep track of them.
Comment #22
bojanz CreditAttribution: bojanz commentedI think it's better to keep it here, so we can keep track of what we need to update for the 7.x reroll.
Comment #23
Shadlington CreditAttribution: Shadlington commentedSubscribing
Comment #24
bojanz CreditAttribution: bojanz commentedHere's a followup 6.x patch (addressing my #17, as well as including teliseo's fixes), as well as what should be the final 7.x patch (which includes all fixes done so far). The 7.x patch also applies to the new UI branch.
Let's get this in!
Comment #25
bojanz CreditAttribution: bojanz commentedMade a stupid mistake. Updated patches.
Comment #26
drunken monkeyRan the tests and surfed through a few views, and everything seems to be alright.
Great work, Bojan, thanks!
Comment #27
Shadlington CreditAttribution: Shadlington commentedI'm not sure what specifically I can do to test this but I applied the patch and checked out a few views on my D7 test site and all was well...
If there's anything specific that needs testing before this can be committed, let me know. Seems good though!
Comment #28
dawehnerYou could basically test all fields which are changed here.
Comment #29
bojanz CreditAttribution: bojanz commentedI think this one's good to go. The 7.x patch comes a long time after the 6.x one and includes fixes for all the problems spotted since then. Plus I reviewed it multiple times. Even if it does need a followup, better than reviewing a 55kb for the 20th time.
Comment #30
Shadlington CreditAttribution: Shadlington commented+1 for #29
Comment #31
dawehnerSo reviewed the patch (d6 changes) again. Seems really fine especially the $type parameter for sanitize_value.
Commited both patches: 6.x-3.x and 7.x-3.x and pushed it.