Refactor field render() functions to accept $value, allowing reuse

Better title.

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

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

Here's an example usage, in efq_views, my custom date handler, instead of:

  function render($values) {
    $wrapper = entity_metadata_wrapper($this->query->entity_type, $values);
    $value = $wrapper->{$this->real_field}->value();

    $format = $this->options['date_format'];
    if (in_array($format, array('custom', 'raw time ago', 'time ago', 'raw time span', 'time span'))) {
      $custom_format = $this->options['custom_date_format'];
    }

    if (!$value) {
      return theme('views_nodate');
    }
    else {
      $time_diff = REQUEST_TIME - $value; // will be positive for a datetime in the past (ago), and negative for a datetime in the future (hence)
      switch ($format) {
        case 'raw time ago':
          return format_interval($time_diff, is_numeric($custom_format) ? $custom_format : 2);
        case 'time ago':
          return t('%time ago', array('%time' => format_interval($time_diff, is_numeric($custom_format) ? $custom_format : 2)));
        case 'raw time span':
          return ($time_diff < 0 ? '-' : '') . format_interval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2);
        case 'time span':
          return t(($time_diff < 0 ? '%time hence' : '%time ago'), array('%time' => format_interval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2)));
        case 'custom':
          if ($custom_format == 'r') {
            return format_date($value, $format, $custom_format, null, 'en');
          }
          return format_date($value, $format, $custom_format);
        default:
          return format_date($value, $format);
      }
    }

I just do:

  function render($values) {
    $wrapper = entity_metadata_wrapper($this->query->entity_type, $values);
    $value = $wrapper->{$this->real_field}->value();

     return parent::render($values, value);
  }

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.

Note 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".

Discussed 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?

I think it's ready now.

Maybe this one will apply better.

Included 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).

diff -u -p -r1.1.6.1 views_handler_field_url.inc
--- handlers/views_handler_field_url.inc	4 Feb 2011 12:29:01 -0000	1.1.6.1
+++ handlers/views_handler_field_url.inc	4 Feb 2011 12:53:58 -0000
@@ -28,12 +28,12 @@ class views_handler_field_url extends vi
   }

   function render($values) {
-    $value = $values->{$this->field_alias};
+    $value = $this->get_value($values);
     if (!empty($this->options['display_as_link'])) {
-      return l(check_plain($value), $value, array('html' => TRUE));
+      return l($this->sanitize_value($value), $value, array('html' => TRUE));
     }
     else {
-      return check_url($value);
+      return $this->sanitize_value($value);
     }
   }
 }

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.

I think it's better to keep it here, so we can keep track of what we need to update for the 7.x reroll.

Here'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!

Made a stupid mistake. Updated patches.

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