Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Respect explicit casting before casting arrays #11271

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Forms/Form.php
Original file line number Diff line number Diff line change
Expand Up @@ -521,13 +521,13 @@ public function setFieldMessage(
return $this;
}

public function castingHelper($field)
public function castingHelper($field, bool $useFallback = true)
{
// Override casting for field message
if (strcasecmp($field ?? '', 'Message') === 0 && ($helper = $this->getMessageCastingHelper())) {
return $helper;
}
return parent::castingHelper($field);
return parent::castingHelper($field, $useFallback);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/Forms/FormField.php
Original file line number Diff line number Diff line change
Expand Up @@ -790,13 +790,13 @@ public function securityTokenEnabled()
return $form->getSecurityToken()->isEnabled();
}

public function castingHelper($field)
public function castingHelper($field, bool $useFallback = true)
{
// Override casting for field message
if (strcasecmp($field ?? '', 'Message') === 0 && ($helper = $this->getMessageCastingHelper())) {
return $helper;
}
return parent::castingHelper($field);
return parent::castingHelper($field, $useFallback);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/Forms/ReadonlyField.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ public function Type()
return 'readonly';
}

public function castingHelper($field)
public function castingHelper($field, bool $useFallback = true)
{
// Get dynamic cast for 'Value' field
if (strcasecmp($field ?? '', 'Value') === 0) {
return $this->getValueCast();
}

// Fall back to default casting
return parent::castingHelper($field);
return parent::castingHelper($field, $useFallback);
}

public function getSchemaStateDefaults()
Expand Down
4 changes: 2 additions & 2 deletions src/ORM/DataObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -2947,7 +2947,7 @@ public function setCastedField($fieldName, $value)
/**
* {@inheritdoc}
*/
public function castingHelper($field)
public function castingHelper($field, bool $useFallback = true)
{
$fieldSpec = static::getSchema()->fieldSpec(static::class, $field);
if ($fieldSpec) {
Expand All @@ -2965,7 +2965,7 @@ public function castingHelper($field)
}
}

return parent::castingHelper($field);
return parent::castingHelper($field, $useFallback);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/ORM/FieldType/DBComposite.php
Original file line number Diff line number Diff line change
Expand Up @@ -319,14 +319,14 @@ public function dbObject($field)
return $fieldObject;
}

public function castingHelper($field)
public function castingHelper($field, bool $useFallback = true)
{
$fields = $this->compositeDatabaseFields();
if (isset($fields[$field])) {
return $fields[$field];
}

return parent::castingHelper($field);
return parent::castingHelper($field, $useFallback);
}

public function getIndexSpecs()
Expand Down
48 changes: 40 additions & 8 deletions src/View/ViewableData.php
Original file line number Diff line number Diff line change
Expand Up @@ -385,10 +385,11 @@ public function setCustomisedObj(ViewableData $object)
* for a field on this object. This helper will be a subclass of DBField.
*
* @param string $field
* @return string Casting helper As a constructor pattern, and may include arguments.
* @param bool $useFallback If true, fall back on the default casting helper if there isn't an explicit one.
* @return string|null Casting helper As a constructor pattern, and may include arguments.
* @throws Exception
*/
public function castingHelper($field)
public function castingHelper($field, bool $useFallback = true)
Comment on lines -391 to +392
Copy link
Member Author

Choose a reason for hiding this comment

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

Opted to add an extra param here instead of just never including defaults from this method because this reduces upgrade pains - any usage of this method outside of the obj() method should always fall back on the default casting.

{
// Get casting if it has been configured.
// DB fields and PHP methods are all case insensitive so we normalise casing before checking.
Expand All @@ -399,20 +400,41 @@ public function castingHelper($field)
}

// If no specific cast is declared, fall back to failover.
// Note that if there is a failover, the default_cast will always
$failover = $this->getFailover();
if ($failover) {
$cast = $failover->castingHelper($field, $useFallback);
if ($cast) {
return $cast;
}
}

if ($useFallback) {
return $this->defaultCastingHelper($field);
}

return null;
}

/**
* Return the default "casting helper" for use when no explicit casting helper is defined.
* This helper will be a subclass of DBField. See castingHelper()
*/
protected function defaultCastingHelper(string $field): string
{
// If there is a failover, the default_cast will always
// be drawn from this object instead of the top level object.
$failover = $this->getFailover();
if ($failover) {
$cast = $failover->castingHelper($field);
$cast = $failover->defaultCastingHelper($field);
if ($cast) {
return $cast;
}
}

// Fall back to default_cast
// Fall back to raw default_cast
$default = $this->config()->get('default_cast');
if (empty($default)) {
throw new Exception("No default_cast");
throw new Exception('No default_cast');
}
return $default;
}
Expand Down Expand Up @@ -559,15 +581,25 @@ public function obj($fieldName, $arguments = [], $cache = false, $cacheName = nu
$value = $this->$fieldName;
}

// Try to cast object if we have an explicit cast set
if (!is_object($value)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Keeping this condition here (mirroring the below) because if a method returns an explicit object (usually DBField), the implication is that it shouldn't be re-cast.

We could choose to change that, but if so it should be handled in a separate card with an issue for that purpose.

$castingHelper = $this->castingHelper($fieldName, false);
if ($castingHelper !== null) {
$valueObject = Injector::inst()->create($castingHelper, $fieldName);
$valueObject->setValue($value, $this);
$value = $valueObject;
}
}

// Wrap list arrays in ViewableData so templates can handle them
if (is_array($value) && array_is_list($value)) {
$value = ArrayList::create($value);
}

// Cast object
// Fallback on default casting
if (!is_object($value)) {
// Force cast
$castingHelper = $this->castingHelper($fieldName);
$castingHelper = $this->defaultCastingHelper($fieldName);
$valueObject = Injector::inst()->create($castingHelper, $fieldName);
$valueObject->setValue($value, $this);
$value = $valueObject;
Expand Down
4 changes: 4 additions & 0 deletions tests/php/View/ViewableDataTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\ORM\ArrayList;
use SilverStripe\ORM\FieldType\DBText;
use SilverStripe\View\ArrayData;
use SilverStripe\View\SSViewer;
use SilverStripe\View\Tests\ViewableDataTest\ViewableDataTestExtension;
Expand Down Expand Up @@ -59,6 +60,9 @@ public function testRequiresCasting()

$this->assertInstanceOf(ViewableDataTest\RequiresCasting::class, $caster->obj('alwaysCasted'));
$this->assertInstanceOf(ViewableDataTest\Caster::class, $caster->obj('noCastingInformation'));

$this->assertInstanceOf(DBText::class, $caster->obj('arrayOne'));
$this->assertInstanceOf(ArrayList::class, $caster->obj('arrayTwo'));
}

public function testFailoverRequiresCasting()
Expand Down
12 changes: 11 additions & 1 deletion tests/php/View/ViewableDataTest/Castable.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@

class Castable extends ViewableData implements TestOnly
{

private static $default_cast = Caster::class;

private static $casting = [
'alwaysCasted' => RequiresCasting::class,
'castedUnsafeXML' => UnescapedCaster::class,
'test' => 'Text',
'arrayOne' => 'Text',
];

public $test = 'test';
Expand All @@ -25,6 +25,16 @@ public function alwaysCasted()
return 'alwaysCasted';
}

public function arrayOne()
{
return ['value1', 'value2'];
}

public function arrayTwo()
{
return ['value1', 'value2'];
}

public function noCastingInformation()
{
return 'noCastingInformation';
Expand Down
Loading