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.

Sign up for GitHub

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

NEW Handle WithinRangeFilter in searchable_fields and SearchContext #11602

Merged
merged 4 commits into from
Feb 18, 2025
+1,071 −105
Merged

NEW Handle WithinRangeFilter in searchable_fields and SearchContext #11602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
842d328
API Provide minimum and maximum comparison values for numeric DBFields
GuySartorelli Jan 31, 2025
d6751ae
NEW Handle WithinRangeFilter in searchable_fields and SearchContext
GuySartorelli Jan 31, 2025
c0880fa
FIX Pass filter fields through the form before using them.
GuySartorelli Feb 17, 2025
d047b6c
ENH Update scaffolded search fields to be more sensible
GuySartorelli Feb 17, 2025
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
22 changes: 5 additions & 17 deletions src/Core/Validation/FieldValidation/BigIntFieldValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,14 @@

use RunTimeException;
use SilverStripe\Core\Validation\ValidationResult;
use SilverStripe\ORM\FieldType\DBBigInt;

/**
* A field validator for 64-bit integers
* Will throw a RunTimeException if used on a 32-bit system
*/
class BigIntFieldValidator extends IntFieldValidator
{
/**
* The minimum value for a signed 64-bit integer.
* Defined as string instead of int otherwise will end up as a float
* on 64-bit systems
*
* When this is cast to an int in IntFieldValidator::__construct()
* it will be properly cast to an int
*/
protected const MIN_INT = '-9223372036854775808';

/**
* The maximum value for a signed 64-bit integer.
*/
protected const MAX_INT = '9223372036854775807';
Comment on lines -14 to -27
Copy link
Member Author

Choose a reason for hiding this comment

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

These are moved to DBBigInt so they can be used with the new getMinValue() and getMaxValue().


public function __construct(
string $name,
mixed $value,
Expand All @@ -37,6 +23,8 @@ public function __construct(
if ($bits === 32) {
throw new RunTimeException('Cannot use BigIntFieldValidator on a 32-bit system');
}
$minValue ??= (int) DBBigInt::getMinValue();
$maxValue ??= (int) DBBigInt::getMaxValue();
}
$this->minValue = $minValue;
$this->maxValue = $maxValue;
Expand All @@ -51,10 +39,10 @@ protected function validateValue(): ValidationResult
// int values that are too large or too small will be cast to float
// on 64-bit systems and will fail the validation in IntFieldValidator
if (is_string($this->value)) {
if (!is_null($this->minValue) && bccomp($this->value, static::MIN_INT) === -1) {
if (!is_null($this->minValue) && bccomp($this->value, DBBigInt::getMinValue()) === -1) {
$result->addFieldError($this->name, $this->getTooSmallMessage());
}
if (!is_null($this->maxValue) && bccomp($this->value, static::MAX_INT) === 1) {
if (!is_null($this->maxValue) && bccomp($this->value, DBBigInt::getMaxValue()) === 1) {
$result->addFieldError($this->name, $this->getTooLargeMessage());
}
}
Expand Down
17 changes: 3 additions & 14 deletions src/Core/Validation/FieldValidation/IntFieldValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,35 +3,24 @@
namespace SilverStripe\Core\Validation\FieldValidation;

use SilverStripe\Core\Validation\ValidationResult;
use SilverStripe\ORM\FieldType\DBInt;

/**
* Validates that a value is a 32-bit signed integer
*/
class IntFieldValidator extends NumericNonStringFieldValidator
{
/**
* The minimum value for a signed 32-bit integer.
* Defined as string instead of int because be cast to a float
* on 32-bit systems if defined as an int
*/
protected const MIN_INT = '-2147483648';

/**
* The maximum value for a signed 32-bit integer.
*/
protected const MAX_INT = '2147483647';
Comment on lines -12 to -22
Copy link
Member Author

Choose a reason for hiding this comment

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

These are moved to DBInt so they can be used with the new getMinValue() and getMaxValue().


public function __construct(
string $name,
mixed $value,
?int $minValue = null,
?int $maxValue = null
) {
if (is_null($minValue)) {
$minValue = (int) static::MIN_INT;
$minValue = (int) DBInt::getMinValue();
}
if (is_null($maxValue)) {
$maxValue = (int) static::MAX_INT;
$maxValue = (int) DBInt::getMaxValue();
}
parent::__construct($name, $value, $minValue, $maxValue);
}
Expand Down
1 change: 1 addition & 0 deletions src/Forms/CurrencyField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public function setValue($value, $data = null)
. number_format((double)preg_replace('/[^0-9.\-]/', '', $value ?? ''), 2);
return $this;
}

/**
* Overwrite the datavalue before saving to the db ;-)
* return 0.00 if no value, or value is non-numeric
Expand Down
20 changes: 17 additions & 3 deletions src/Forms/GridField/GridFieldFilterHeader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,13 @@ public function handleAction(GridField $gridField, $actionName, $arguments, $dat
$state->Columns = [];

if ($actionName === 'filter') {
if (isset($data['filter'][$gridField->getName()])) {
foreach ($data['filter'][$gridField->getName()] as $key => $filter) {
$state->Columns->$key = $filter;
$filterValues = $data['filter'][$gridField->getName()] ?? null;
if ($filterValues !== null) {
$form = $this->getSearchForm($gridField);
$this->removeSearchPrefixFromFields($form->Fields());
$form->loadDataFrom($filterValues);
foreach ($filterValues as $fieldName => $rawFilterValue) {
$state->Columns->$fieldName = $form->Fields()->dataFieldByName($fieldName)?->dataValue() ?? $rawFilterValue;
}
}
}
Expand Down Expand Up @@ -498,6 +502,16 @@ private function addSearchPrefixToFields(FieldList $fields): void
}
}

private function removeSearchPrefixFromFields(FieldList $fields): void
{
foreach ($fields as $field) {
$field->setName(str_replace('Search__', '', $field->getName()));
if ($field instanceof CompositeField) {
$this->removeSearchPrefixFromFields($field->getChildren());
}
}
}

/**
* Update CSS classes for form fields, including nested inside composite fields
*/
Expand Down
148 changes: 112 additions & 36 deletions src/ORM/DataObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use SilverStripe\Core\Validation\ValidationResult;
use SilverStripe\Dev\Debug;
use SilverStripe\Dev\Deprecation;
use SilverStripe\Forms\FieldGroup;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\FormField;
use SilverStripe\Forms\FormScaffolder;
Expand Down Expand Up @@ -46,6 +47,7 @@
use SilverStripe\Security\Security;
use SilverStripe\View\SSViewer;
use SilverStripe\Model\ModelData;
use SilverStripe\ORM\Filters\WithinRangeFilter;
use stdClass;

/**
Expand Down Expand Up @@ -2438,40 +2440,7 @@ public function scaffoldSearchFields($_params = null)
continue;
}

// If a custom fieldclass is provided as a string, use it
$field = null;
if ($params['fieldClasses'] && isset($params['fieldClasses'][$fieldName])) {
$fieldClass = $params['fieldClasses'][$fieldName];
$field = new $fieldClass($fieldName);
// If we explicitly set a field, then construct that
} elseif (isset($spec['field'])) {
// If it's a string, use it as a class name and construct
if (is_string($spec['field'])) {
$fieldClass = $spec['field'];
$field = new $fieldClass($fieldName);

// If it's a FormField object, then just use that object directly.
} elseif ($spec['field'] instanceof FormField) {
$field = $spec['field'];

// Otherwise we have a bug
} else {
user_error("Bad value for searchable_fields, 'field' value: "
. var_export($spec['field'], true), E_USER_WARNING);
}

// Otherwise, use the database field's scaffolder
} elseif ($object = $this->relObject($fieldName)) {
if (is_object($object) && $object->hasMethod('scaffoldSearchField')) {
$field = $object->scaffoldSearchField();
} else {
throw new Exception(sprintf(
"SearchField '%s' on '%s' does not return a valid DBField instance.",
$fieldName,
get_class($this)
));
}
}
$field = $this->scaffoldSingleSearchField($fieldName, $spec, $params);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a lot of code to parse through just to see that it's all for this singular purpose. Made sense to abstract into its own method here to encapsulate this functionality, reducing cognitive load for anyone working in this area in the future.


// Allow fields to opt out of search
if (!$field) {
Expand All @@ -2483,6 +2452,24 @@ public function scaffoldSearchFields($_params = null)
}
$field->setTitle($spec['title']);

// If we're using a WithinRangeFilter, split the field into two separate fields (from and to)
if (is_a($spec['filter'] ?? '', WithinRangeFilter::class, true)) {
$fieldFrom = $field;
$fieldTo = clone $field;
$originalTitle = $field->Title();
$originalName = $field->getName();

$fieldFrom->setName($originalName . '_SearchFrom');
Copy link
Member Author

Choose a reason for hiding this comment

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

The _Search part of this suffix is to help prevent clashes, in case someone has something like a Price and a PriceFrom DB field - this won't clash with it if they add a range filter for Price.

$fieldFrom->setTitle(_t(__CLASS__ . '.FILTER_WITHINRANGE_FROM', 'From'));
$fieldTo->setName($originalName . '_SearchTo');
$fieldTo->setTitle(_t(__CLASS__ . '.FILTER_WITHINRANGE_TO', 'To'));

$field = FieldGroup::create(
$originalTitle,
[$fieldFrom, $fieldTo]
)->setName($originalName)->addExtraClass('fieldgroup--fill-width');
}

$fields->push($field);
}

Expand All @@ -2498,6 +2485,56 @@ public function scaffoldSearchFields($_params = null)
return $fields;
}

/**
* Scaffold a single form field for use by the search context form.
*/
private function scaffoldSingleSearchField(string $fieldName, array $spec, ?array $params): ?FormField
{
// If a custom fieldclass is provided as a string, use it
$field = null;
if ($params['fieldClasses'] && isset($params['fieldClasses'][$fieldName])) {
$fieldClass = $params['fieldClasses'][$fieldName];
$field = new $fieldClass($fieldName);
// If we explicitly set a field, then construct that
} elseif (isset($spec['field'])) {
// If it's a string, use it as a class name and construct
if (is_string($spec['field'])) {
$fieldClass = $spec['field'];
$field = new $fieldClass($fieldName);

// If it's a FormField object, then just use that object directly.
} elseif ($spec['field'] instanceof FormField) {
$field = $spec['field'];

// Otherwise we have a bug
} else {
user_error("Bad value for searchable_fields, 'field' value: "
. var_export($spec['field'], true), E_USER_WARNING);
}
// Use the explicitly defined dataType if one was set
} elseif (isset($spec['dataType'])) {
$object = Injector::inst()->get($spec['dataType'], true);
$field = $this->scaffoldFieldFromObject($object, $fieldName);
$field->setName($fieldName);
// Otherwise, use the database field's scaffolder
} elseif ($object = $this->relObject($fieldName)) {
$field = $this->scaffoldFieldFromObject($object, $fieldName);
}
return $field;
}

private function scaffoldFieldFromObject(mixed $object, string $fieldName): FormField
{
if (!is_object($object) || !$object->hasMethod('scaffoldSearchField')) {
throw new LogicException(sprintf(
"SearchField '%s' on '%s' does not return a valid DBField instance.",
$fieldName,
get_class($this)
));
}
return $object->scaffoldSearchField();
}

/**
* Scaffold a simple edit form for all properties on this dataobject,
* based on default {@link FormField} mapping in {@link DBField::scaffoldFormField()}.
Expand Down Expand Up @@ -3896,28 +3933,40 @@ public function searchableFields()
$rewrite = [];
foreach ($fields as $name => $specOrName) {
$identifier = (is_int($name)) ? $specOrName : $name;
$relObject = isset($specOrName['match_any']) ? null : $this->relObject($identifier);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was only being called if the spec is an array - but that means if you're trying to search against a field that isn't valid, you only get an exception if you're using complex config! Otherwise you have to filter to get the exception, which isn't very friendly.

Note that it is possible for this to return null


if (is_int($name)) {
// Format: array('MyFieldName')
$rewrite[$identifier] = [];
} elseif (is_array($specOrName) && (isset($specOrName['match_any']))) {
$rewrite[$identifier] = $fields[$identifier];
$rewrite[$identifier]['match_any'] = $specOrName['match_any'];
} elseif (is_array($specOrName) && ($relObject = $this->relObject($identifier))) {
} elseif (is_array($specOrName)) {
Comment on lines -3906 to +3944
Copy link
Member Author

Choose a reason for hiding this comment

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

The condition would cause the following problem:

You have config like this:

[
    'MyRelation' => [
        'filter' => 'ExactMatch',
    ],
]

Because this is an array, but the relObject returns null, this condition isn't met. So we go to the else clause. This turns the config into this:

[
    'MyRelation' => [
        'filter' => [
            'filter' => 'ExactMatch',
        ],
    ],
]

// Format: array('MyFieldName' => array(
// 'filter => 'ExactMatchFilter',
// 'field' => 'NumericField', // optional
// 'title' => 'My Title', // optional
// 'dataType' => DBInt::class // optional
// These two are only required if using WithinRangeFilter with a data type that doesn't
// inherently represent a date, time, or number
// 'rangeFromDefault' => PHP_INT_MIN
// 'rangeToDefault' => PHP_INT_MAX
// ))
$rewrite[$identifier] = array_merge(
['filter' => $relObject->config()->get('default_search_filter_class')],
[
'filter' => $relObject?->config()->get('default_search_filter_class'),
'dataType' => $relObject ? get_class($relObject) : null,
Comment on lines +3957 to +3958
Copy link
Member Author

Choose a reason for hiding this comment

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

If there's no relObject, these will just default to null. If the $specOrName array doesn't fill them in, they'll get filled in further down the line.

],
(array)$specOrName
);
} else {
// Format: array('MyFieldName' => 'ExactMatchFilter')
$rewrite[$identifier] = [
'filter' => $specOrName,
];
if ($relObject !== null) {
$rewrite[$identifier]['dataType'] ??= get_class($relObject);
}
}
if (!isset($rewrite[$identifier]['title'])) {
$rewrite[$identifier]['title'] = (isset($labels[$identifier]))
Expand All @@ -3926,6 +3975,33 @@ public function searchableFields()
if (!isset($rewrite[$identifier]['filter'])) {
$rewrite[$identifier]['filter'] = 'PartialMatchFilter';
}
// When using a WithinRangeFilter we need to know what the default from and to values
// should be, so that if a user only enters one of the two fields the other can be
// populated appropriately within the filter.
if (is_a($rewrite[$identifier]['filter'], WithinRangeFilter::class, true)) {
// The dataType requirement here is explicitly for WithinRangeFilter.
// DO NOT make it mandatory for other filters without first checking if this breaks
// anything for filtering a relation, where the class on the other end of the relation
// implements scaffoldSearchField().
$dataType = $rewrite[$identifier]['dataType'] ?? null;
if (!is_a($dataType ?? '', DBField::class, true)) {
throw new LogicException("dataType must be set to a DBField class for '$identifier'");
}
if (!isset($rewrite[$identifier]['rangeFromDefault'])) {
$fromDefault = $dataType::getMinValue();
if ($fromDefault === null) {
throw new LogicException("rangeFromDefault must be set for '$identifier'");
}
$rewrite[$identifier]['rangeFromDefault'] = $fromDefault;
}
if (!isset($rewrite[$identifier]['rangeToDefault'])) {
$toDefault = $dataType::getMaxValue();
if ($toDefault === null) {
throw new LogicException("rangeToDefault must be set for '$identifier'");
}
$rewrite[$identifier]['rangeToDefault'] = $toDefault;
}
}
}

$fields = $rewrite;
Expand Down
15 changes: 15 additions & 0 deletions src/ORM/FieldType/DBBigInt.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,21 @@
*/
class DBBigInt extends DBInt
{
/**
* The minimum value for a signed 64-bit integer.
* Defined as string instead of int otherwise will end up as a float
* on 64-bit systems
*
* When this is cast to an int in IntFieldValidator::__construct()
* it will be properly cast to an int
*/
protected const MIN_INT = '-9223372036854775808';

/**
* The maximum value for a signed 64-bit integer.
*/
protected const MAX_INT = '9223372036854775807';
Comment on lines +17 to +30
Copy link
Member Author

Choose a reason for hiding this comment

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

These override consts of the same name from DBInt and are used in the new getMinValue() and getMaxValue() methods.


private static array $field_validators = [
// Remove parent validator and add BigIntValidator instead
IntFieldValidator::class => null,
Expand Down
6 changes: 6 additions & 0 deletions src/ORM/FieldType/DBCurrency.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use SilverStripe\Forms\CurrencyField;
use SilverStripe\Forms\FormField;
use SilverStripe\Forms\NumericField;
use SilverStripe\Model\ModelData;

/**
Expand Down Expand Up @@ -68,4 +69,9 @@ public function scaffoldFormField(?string $title = null, array $params = []): ?F
{
return CurrencyField::create($this->getName(), $title);
}

public function scaffoldSearchField(?string $title = null): ?FormField
{
return NumericField::create($this->getName(), $title);
}
}
Loading
Loading