Skip to content

Commit

Permalink
API Make SSViewer#process return HTMLText not string
Browse files Browse the repository at this point in the history 
This means that you dont have to worry about casting it
as HTMLText again when using the result in a template or other context

However in some situations code might be assuming it can
check with is_string, in which case you now need to use instanceof HTMLText
  • Loading branch information
Hamish Friedlander authored and Sam Minnee committed Mar 13, 2013
1 parent 9bd6dd9 commit 743a186
Show file tree
Hide file tree
Showing 7 changed files with 24 additions and 14 deletions.
4 changes: 2 additions & 2 deletions control/Controller.php
View file
Expand Up @@ -157,9 +157,9 @@ public function handleRequest(SS_HTTPRequest $request, DataModel $model) {
. "returning it without modification.");
}
$this->response = $body;

} else {
if(is_object($body)) {
if($body instanceof Object && $body->hasMethod('getViewer')) {
if(isset($_REQUEST['debug_request'])) {
Debug::message("Request handler $body->class object to $this->class controller;"
. "rendering with template returned by $body->class::getViewer()");
Expand Down
2 changes: 1 addition & 1 deletion control/HTTPResponse.php
View file
Expand Up @@ -159,7 +159,7 @@ public function isError() {
* @return SS_HTTPRequest $this
*/
public function setBody($body) {
$this->body = $body;
$this->body = $body ? (string)$body : $body; // Don't type-cast false-ish values, eg null is null not ''
}

/**
Expand Down
3 changes: 2 additions & 1 deletion control/PjaxResponseNegotiator.php
View file
Expand Up @@ -79,7 +79,8 @@ public function respond(SS_HTTPRequest $request, $extraCallbacks = array()) {
// Execute the fragment callbacks and build the response.
foreach($fragments as $fragment) {
if(isset($callbacks[$fragment])) {
$responseParts[$fragment] = call_user_func($callbacks[$fragment]);
$res = call_user_func($callbacks[$fragment]);
$responseParts[$fragment] = $res ? (string)$res : $res;
} else {
throw new SS_HTTPResponse_Exception("X-Pjax = '$fragment' not supported for this URL.", 400);
}
Expand Down
11 changes: 11 additions & 0 deletions dev/SapphireTest.php
View file
Expand Up @@ -510,6 +510,17 @@ public function tearDown() {
$controller->response->removeHeader('Location');
}
}

public static function assertContains($needle, $haystack, $message = '', $ignoreCase = FALSE, $checkForObjectIdentity = TRUE) {
if ($haystack instanceof DBField) $haystack = (string)$haystack;
parent::assertContains($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity);
}

public static function assertNotContains($needle, $haystack, $message = '', $ignoreCase = FALSE, $checkForObjectIdentity = TRUE) {
if ($haystack instanceof DBField) $haystack = (string)$haystack;
parent::assertNotContains($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity);
}

/**
* Clear the log of emails sent
*/
Expand Down
5 changes: 4 additions & 1 deletion model/fieldtypes/DBField.php
View file
Expand Up @@ -295,5 +295,8 @@ public function debug() {
</ul>
DBG;
}


public function __toString() {
return $this->forTemplate();
}
}
11 changes: 3 additions & 8 deletions view/SSViewer.php
View file
Expand Up @@ -866,7 +866,7 @@ protected function includeGeneratedTemplate($cacheFile, $item, $overlay, $underl
* @param ViewableData $item
* @param SS_Cache $cache Optional cache backend.
*
* @return String Parsed template output.
* @return HTMLText Parsed template output.
*/
public function process($item, $arguments = null) {
SSViewer::$topLevel[] = $item;
Expand Down Expand Up @@ -909,12 +909,7 @@ public function process($item, $arguments = null) {
$subtemplateViewer->includeRequirements(false);
$subtemplateViewer->setPartialCacheStore($this->getPartialCacheStore());

$underlay[$subtemplate] = DBField::create_field(
'HTMLText',
$subtemplateViewer->process($item, $arguments),
$subtemplate,
array('shortcodes' => false)
);
$underlay[$subtemplate] = $subtemplateViewer->process($item, $arguments);
}
}

Expand All @@ -939,7 +934,7 @@ public function process($item, $arguments = null) {
}
}

return $output;
return DBField::create_field('HTMLText', $output, null, array('shortcodes' => false));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion view/ViewableData.php
View file
Expand Up @@ -318,7 +318,7 @@ public function buildCastingCache(&$cache) {
*
* @param string|array|SSViewer $template the template to render into
* @param array $customFields fields to customise() the object with before rendering
* @return string
* @return HTMLText
*/
public function renderWith($template, $customFields = null) {
if(!is_object($template)) {
Expand Down

3 comments on commit 743a186

@dhensby
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sminnee This appears to have caused a slight issue with doing something like:

$jsonEncode = array();
foreach (Page::get() as $page) {
    $jsonEncode[$page->ID] = $page->forTemplate();
}
return Convert::raw2json($jsonEncode);

Previously, that would have returned a JSON object of

"ID" : "<html template>"

But now it returns

"ID" : { "class" : "HTMLText" }

My reading of the docs is, that the json_encode function turns the object into a json object based on it's public variables (class being the only one in this instance).

I'm not sure of a way to make json_encode use some other function to determine how it should be represented (in 5.3, you can in 5.4 - see http://www.php.net/manual/en/class.jsonserializable.php).

Not sure how best to patch this? Either the docs need to make it very clear that you need to be casting as string in these circumstances (I think it's very big deal for updgrades) or the Convert::array2json() needs to walk the entire array making sure that all items are strings and casting them as such - but equally, that could break other code that relies on encoding an object and not a string...

@hafriedlander
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you now need to cast as strings, like $jsonEncode[$page->ID] = (string)$page->forTemplate(); - it's a difficult change to catch, as it's only needed where the behaviour for objects is different than the behavior for strings. It is briefly mentioned in the 3.1 upgrade docs but that should probably be expanded.

@dhensby
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with that position, if it is the way it's being put now, then sure. But i do think it's a big deal and needs to be made very clear in the docs.

Please sign in to comment.