diff --git a/composer.json b/composer.json index e71bcc6cc..4aa81241e 100644 --- a/composer.json +++ b/composer.json @@ -115,4 +115,4 @@ }, "minimum-stability": "dev", "prefer-stable": true -} +} \ No newline at end of file diff --git a/src/Core/Environment.php b/src/Core/Environment.php index 84bf2cfe7..a6b0fc7ef 100644 --- a/src/Core/Environment.php +++ b/src/Core/Environment.php @@ -187,16 +187,22 @@ class Environment */ public static function getEnv($name) { - switch (true) { - case is_array(static::$env) && array_key_exists($name, static::$env): - return static::$env[$name]; - case is_array($_ENV) && array_key_exists($name, $_ENV): - return $_ENV[$name]; - case is_array($_SERVER) && array_key_exists($name, $_SERVER): - return $_SERVER[$name]; - default: - return getenv($name); + if (array_key_exists($name, static::$env)) { + return static::$env[$name]; } + // isset() is used for $_ENV and $_SERVER instead of array_key_exists() to fix a very strange issue that + // occured in CI running silverstripe/recipe-kitchen-sink where PHP would timeout due apparently due to an + // excessively high number of array method calls. isset() is not used for static::$env above because + // values there may be null, and isset() will return false for null values + // Symfony also uses isset() for reading $_ENV and $_SERVER values + // https://github.com/symfony/dependency-injection/blob/6.2/EnvVarProcessor.php#L148 + if (isset($_ENV[$name])) { + return $_ENV[$name]; + } + if (isset($_SERVER[$name])) { + return $_SERVER[$name]; + } + return getenv($name); } /** @@ -231,9 +237,10 @@ class Environment */ public static function hasEnv(string $name): bool { + // See getEnv() for an explanation of why isset() is used for $_ENV and $_SERVER return array_key_exists($name, static::$env) - || is_array($_ENV) && array_key_exists($name, $_ENV) - || is_array($_SERVER) && array_key_exists($name, $_SERVER) + || isset($_ENV[$name]) + || isset($_SERVER[$name]) || getenv($name) !== false; } diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php index 7e3a545dd..d74b7bf45 100644 --- a/src/Dev/Deprecation.php +++ b/src/Dev/Deprecation.php @@ -237,8 +237,8 @@ class Deprecation if (!Director::isDev()) { return false; } - $envVar = Environment::getEnv('SS_DEPRECATION_ENABLED'); if (Environment::hasEnv('SS_DEPRECATION_ENABLED')) { + $envVar = Environment::getEnv('SS_DEPRECATION_ENABLED'); return self::varAsBoolean($envVar); } return static::$currentlyEnabled; @@ -293,8 +293,8 @@ class Deprecation */ public static function shouldShowForHttp(): bool { - $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_HTTP'); if (Environment::hasEnv('SS_DEPRECATION_SHOW_HTTP')) { + $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_HTTP'); return self::varAsBoolean($envVar); } return self::$shouldShowForHttp; @@ -306,8 +306,8 @@ class Deprecation */ public static function shouldShowForCli(): bool { - $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_CLI'); if (Environment::hasEnv('SS_DEPRECATION_SHOW_CLI')) { + $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_CLI'); return self::varAsBoolean($envVar); } return self::$shouldShowForCli; diff --git a/src/Forms/GridField/GridFieldFilterHeader.php b/src/Forms/GridField/GridFieldFilterHeader.php index 58599b2fe..6292985be 100755 --- a/src/Forms/GridField/GridFieldFilterHeader.php +++ b/src/Forms/GridField/GridFieldFilterHeader.php @@ -254,21 +254,25 @@ class GridFieldFilterHeader extends AbstractGridFieldComponent implements GridFi public function canFilterAnyColumns($gridField) { $list = $gridField->getList(); - - if (!$this->checkDataType($list)) { + if (!($list instanceof Filterable) || !$this->checkDataType($list)) { return false; } - - $columns = $gridField->getColumns(); - foreach ($columns as $columnField) { - $metadata = $gridField->getColumnMetadata($columnField); - $title = $metadata['title']; - - if ($title && $list->canFilterBy($columnField)) { + $modelClass = $gridField->getModelClass(); + // note: searchableFields() will return summary_fields if there are no searchable_fields on the model + $searchableFields = array_keys($modelClass::singleton()->searchableFields()); + $summaryFields = array_keys($modelClass::singleton()->summaryFields()); + sort($searchableFields); + sort($summaryFields); + // searchable_fields has been explictily defined i.e. searchableFields() is not falling back to summary_fields + if ($searchableFields !== $summaryFields) { + return true; + } + // we have fallen back to summary_fields, check they are filterable + foreach ($searchableFields as $searchableField) { + if ($list->canFilterBy($searchableField)) { return true; } } - return false; } diff --git a/src/ORM/ArrayList.php b/src/ORM/ArrayList.php index 83e448b93..c5c478326 100644 --- a/src/ORM/ArrayList.php +++ b/src/ORM/ArrayList.php @@ -604,7 +604,7 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L $firstRecord = $this->first(); - return is_array($firstRecord) ? array_key_exists($by, $firstRecord) : property_exists($by, $firstRecord ?? ''); + return is_array($firstRecord) ? array_key_exists($by, $firstRecord) : property_exists($firstRecord, $by ?? ''); } /** diff --git a/tests/php/Core/EnvironmentTest.php b/tests/php/Core/EnvironmentTest.php index 221b2563a..7f864710b 100644 --- a/tests/php/Core/EnvironmentTest.php +++ b/tests/php/Core/EnvironmentTest.php @@ -79,7 +79,6 @@ class EnvironmentTest extends SapphireTest $valueOptions = [ true, false, - null, 0, 1, 1.75, @@ -97,6 +96,12 @@ class EnvironmentTest extends SapphireTest ]; } } + // `null` isn't a supported value outside of using the `.env` option. + $scenarios[] = [ + 'setAs' => '.env', + 'value' => null, + 'expected' => true, + ]; $scenarios[] = [ 'setAs' => null, 'value' => null, diff --git a/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php b/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php index db70ddddb..bf6c95b8c 100644 --- a/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php +++ b/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php @@ -182,4 +182,30 @@ class GridFieldFilterHeaderTest extends SapphireTest $this->assertEquals('ReallyCustomSearch', $this->component->getSearchField()); } + + public function testCanFilterAnyColumns() + { + $gridField = $this->gridField; + $filterHeader = $gridField->getConfig()->getComponentByType(GridFieldFilterHeader::class); + + // test that you can filter by something if searchable_fields is not defined + // silverstripe will scaffold db columns that are in the gridfield to be + // searchable by default + Config::modify()->remove(Team::class, 'searchable_fields'); + $this->assertTrue($filterHeader->canFilterAnyColumns($gridField)); + + // test that you can filterBy if searchable_fields is defined + Config::modify()->set(Team::class, 'searchable_fields', ['Name']); + $this->assertTrue($filterHeader->canFilterAnyColumns($gridField)); + + // test that you can filterBy if searchable_fields even if it is not a legit field + // this is because we're making a blind assumption it will be filterable later in a SearchContext + Config::modify()->set(Team::class, 'searchable_fields', ['WhatIsThis']); + $this->assertTrue($filterHeader->canFilterAnyColumns($gridField)); + + // test that you cannot filter by non-db field when it falls back to summary_fields + Config::modify()->remove(Team::class, 'searchable_fields'); + Config::modify()->set(Team::class, 'summary_fields', ['MySummaryField']); + $this->assertFalse($filterHeader->canFilterAnyColumns($gridField)); + } } diff --git a/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php b/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php index 98d587975..b39eefe56 100644 --- a/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php +++ b/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php @@ -24,4 +24,9 @@ class Team extends DataObject implements TestOnly 'Cheerleader' => Cheerleader::class, 'CheerleadersMom' => Mom::class ]; + + public function getMySummaryField() + { + return 'MY SUMMARY FIELD'; + } }