Skip to content
This repository
Browse code

BUG 15e2efb broke the Page ListView.

  • Loading branch information...
commit 9c4e4747c9c39c5753436ec3c124156c4ea6b2d8 1 parent c070771
Hamish Friedlander authored July 28, 2012

Showing 1 changed file with 20 additions and 7 deletions. Show diff stats Hide diff stats

  1. 27  forms/gridfield/GridFieldDataColumns.php
27  forms/gridfield/GridFieldDataColumns.php
@@ -133,13 +133,14 @@ public function getColumnContent($gridField, $record, $columnName) {
133 133
 			$value = $gridField->getDataFieldValue($record, $columnName);
134 134
 		}
135 135
 
136  
-		$value = $this->formatValue($gridField, $record, $columnName, $value);
  136
+		// Turn $value, whatever it is, into a HTML embeddable string
137 137
 		$value = $this->castValue($gridField, $columnName, $value);
  138
+		// Make any formatting tweaks
  139
+		$value = $this->formatValue($gridField, $record, $columnName, $value);
  140
+		// Do any final escaping
138 141
 		$value = $this->escapeValue($gridField, $value);
139 142
 
140  
-		return (is_object($value) && $value instanceof Object && $value->hasMethod('forTemplate')) 
141  
-			? $value->forTemplate()
142  
-			: Convert::raw2xml($value);
  143
+		return $value;
143 144
 	}
144 145
 	
145 146
 	/**
@@ -200,6 +201,7 @@ protected function getValueFromRelation($record, $columnName) {
200 201
 	}
201 202
 	
202 203
 	/**
  204
+	 * Casts a field to a string which is safe to insert into HTML
203 205
 	 *
204 206
 	 * @param GridField $gridField
205 207
 	 * @param string $fieldName
@@ -207,11 +209,22 @@ protected function getValueFromRelation($record, $columnName) {
207 209
 	 * @return string 
208 210
 	 */
209 211
 	protected function castValue($gridField, $fieldName, $value) {
  212
+		// If a fieldCasting is specified, we assume the result is safe
210 213
 		if(array_key_exists($fieldName, $this->fieldCasting)) {
211  
-			return $gridField->getCastedValue($value, $this->fieldCasting[$fieldName]);
212  
-		} elseif(is_object($value) && method_exists($value, 'Nice')) {
213  
-			return $value->Nice();
  214
+			$value = $gridField->getCastedValue($value, $this->fieldCasting[$fieldName]);
214 215
 		}
  216
+		// If the value is an object, we do one of two things
  217
+		else if(is_object($value)) {
  218
+			// If it has a "Nice" method, call that & make sure the result is safe
  219
+			if (method_exists($value, 'Nice')) Convert::raw2xml($value->Nice());
  220
+			// Otherwise call forTemplate - the result of this should already be safe
  221
+			else $value = $value->forTemplate();
  222
+		}
  223
+		// Otherwise, just treat as a text string & make sure the result is safe
  224
+		else {
  225
+			$value = Convert::raw2xml($value);
  226
+		}
  227
+
215 228
 		return $value;
216 229
 	}
217 230
 	

8 notes on commit 9c4e474

jakr

I think this line is a NOP, it should have an assignment: $value = Convert::raw2xml($value->Nice());.

jakr
jakr commented on 9c4e474 July 29, 2012

Thanks for cleaning up my mess! To me, this looks like a nice solution. Some comments:

  1. I think line 219 misses an assignment (see comment there)
  2. There might be a $value that is an object which does not have a forTemplate method. I am not sure if this case is possible, but if it occured, the call to $value->forTemplate() in line 221 will fail.
  3. In my opinion the comment to setFieldFormatting (line 101, not part of this commit) should be updated. I think the example there will only work if a fieldCasting is set.
  4. The way the brackets are structured in castValue is hard to read for me. My personal preference is to have else on the same line as the closing brackets of the if (this is also what the SilverStripe coding conventions recommend). To me this way makes it clear that there is an else part. The advantage of your style is that it offers a space for the comments. Nevertheless, I would write it like:

    protected function castValue($gridField, $fieldName, $value) {
        // If a fieldCasting is specified, we assume the result is safe
        if(array_key_exists($fieldName, $this->fieldCasting)) {
            $value = $gridField->getCastedValue($value, $this->fieldCasting[$fieldName]);
        } else if(is_object($value)) {
            // If the value is an object, we do one of two things
            if (method_exists($value, 'Nice')) {
                // If it has a "Nice" method, call that & make sure the result is safe
                $value = Convert::raw2xml($value->Nice());
            } else {
                // Otherwise call forTemplate - the result of this should already be safe
                $value = $value->forTemplate();
            }
        } else {
            // Otherwise, just treat as a text string & make sure the result is safe
            $value = Convert::raw2xml($value);
        }
        return $value;
    }
    
jakr
jakr commented on 9c4e474 July 29, 2012

The third point is actually not an issue. I overlooked that formatValue operates on the original item, and not on $value, when accessing fields (e.g. $ID will access $record->ID and not $value->ID).

jakr
jakr commented on 9c4e474 July 29, 2012

See #680

Hamish Friedlander
Owner

Yeah, that's a NOP. Thanks for the catch.

Theoretically $value as object might not have a forTemplate, and somewhere in documentation we should indicate it must in order to be a valid $value. I'm not sure deliberately checking just to throw a clearer error message is useful though (there's no reasonable fallback if we have an object that doesn't have a forTemplate defined, so we can't fail gracefully - except __toString I guess, which might be missing too).

Sam Minnée
Owner

We could potentially call forTemplate from the default __toString() method and then cast as string? It would be similar to deprecating forTemplate in favour of toString... Is that appropriate?

Sam Minnée
Owner

...or call forTemplate if it exists, otherwise cast as string.

jakr

To me this depends on what the semantics of __toString should be. If we postulate that all strings be safe for HTML that would be ok, but then we would need a different method to generate strings for the commandline.

I am in favor of keeping forTemplate (or using XML, HTML, toHTML, HTMLSafe or similar), but I think it should be clarified where this method comes from in terms of the class inheritance. At the moment it is introduced in several places, but I think it should be defined in ViewableData (see my post to the dev mailinglist: Why no forTemplate in ViewableData). Saying that something is viewable and then not providing a way to display it safely is, IMO, illogical.

Stig Lindqvist
Owner

2 cents coming up: I'm in favor of forTemplate and throwing an error unless it's defined. __toString is IMHO magic that should only be used for debugging.

Please sign in to comment.
Something went wrong with that request. Please try again.