From 27bd5d12e3f4001f0c80a1a333a440cd8b0ae897 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 24 Sep 2020 17:09:37 -0700 Subject: [PATCH 1/2] ENH Replace E_USER_ERROR errors with exceptions --- .../00_Model/05_Extending_DataObjects.md | 14 +++++------ .../07_Debugging/01_Error_Handling.md | 3 ++- src/Control/Controller.php | 2 +- src/Control/HTTPRequest.php | 7 +++--- src/Control/RequestHandler.php | 2 +- src/Core/Extensible.php | 20 +++++++-------- src/Dev/CsvBulkLoader.php | 12 ++++++--- src/Dev/SapphireTest.php | 14 +++++------ src/Forms/CompositeField.php | 5 ++-- src/Forms/FieldList.php | 25 ++++++++----------- src/Forms/FormField.php | 7 +----- src/Forms/SelectField.php | 2 +- src/Forms/TreeMultiselectField.php | 6 ++--- src/ORM/Connect/Database.php | 3 +-- src/ORM/Connect/MySQLiConnector.php | 8 +++--- src/ORM/DataList.php | 4 +-- src/ORM/DataObject.php | 14 +++++++---- src/ORM/FieldType/DBBoolean.php | 10 ++++---- src/ORM/FieldType/DBDecimal.php | 4 ++- src/ORM/FieldType/DBEnum.php | 10 ++++---- src/ORM/FieldType/DBMultiEnum.php | 12 +++++---- src/ORM/HasManyList.php | 7 ++---- src/ORM/Hierarchy/Hierarchy.php | 5 ++-- src/ORM/Map.php | 10 +++----- src/ORM/PolymorphicHasManyList.php | 3 +-- src/ORM/Queries/SQLConditionalExpression.php | 13 +++++----- src/ORM/UnsavedRelationList.php | 6 ++--- src/Security/Member_GroupSet.php | 2 +- src/Security/Permission.php | 12 +++------ src/View/Parsers/ShortcodeParser.php | 13 +++++----- src/View/SSViewer_DataPresenter.php | 2 +- src/View/SSViewer_Scope.php | 2 +- 32 files changed, 120 insertions(+), 139 deletions(-) diff --git a/docs/en/02_Developer_Guides/00_Model/05_Extending_DataObjects.md b/docs/en/02_Developer_Guides/00_Model/05_Extending_DataObjects.md index 0a413e859..6d60febeb 100644 --- a/docs/en/02_Developer_Guides/00_Model/05_Extending_DataObjects.md +++ b/docs/en/02_Developer_Guides/00_Model/05_Extending_DataObjects.md @@ -27,25 +27,23 @@ use SilverStripe\ORM\DataObject; class Player extends DataObject { private static $has_many = [ - "Teams" => "Team", + 'Teams' => 'Team', ]; public function onBeforeWrite() { // check on first write action, aka "database row creation" (ID-property is not set) - if(!$this->isInDb()) { + if (!$this->isInDb()) { $currentPlayer = Security::getCurrentUser(); - if(!$currentPlayer->IsTeamManager()) { - user_error('Player-creation not allowed', E_USER_ERROR); - exit(); + if (!$currentPlayer->IsTeamManager()) { + throw new \Exception('Player-creation not allowed'); } } // check on every write action - if(!$this->record['TeamID']) { - user_error('Cannot save player without a valid team', E_USER_ERROR); - exit(); + if (!$this->record['TeamID']) { + throw new \Exception('Cannot save player without a valid team'); } // CAUTION: You are required to call the parent-function, otherwise diff --git a/docs/en/02_Developer_Guides/07_Debugging/01_Error_Handling.md b/docs/en/02_Developer_Guides/07_Debugging/01_Error_Handling.md index fd319a5a2..68d7045b2 100644 --- a/docs/en/02_Developer_Guides/07_Debugging/01_Error_Handling.md +++ b/docs/en/02_Developer_Guides/07_Debugging/01_Error_Handling.md @@ -115,7 +115,8 @@ developers know: * Things that will prevent an internal function from continuing. Throw a warning and return null. * **E_USER_ERROR:** Throwing one of these errors is going to take down the production site. So you should only throw -E_USER_ERROR if it's going to be **dangerous** or **impossible** to continue with the request. +E_USER_ERROR if it's going to be **dangerous** or **impossible** to continue with the request. Note that it is +preferable to now throw exceptions instead of `E_USER_ERROR`. ## Configuring error logging diff --git a/src/Control/Controller.php b/src/Control/Controller.php index ffbaadb91..3cb1b9833 100644 --- a/src/Control/Controller.php +++ b/src/Control/Controller.php @@ -199,7 +199,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider public function handleRequest(HTTPRequest $request) { if (!$request) { - user_error("Controller::handleRequest() not passed a request!", E_USER_ERROR); + throw new \RuntimeException('Controller::handleRequest() not passed a request!'); } //set up the controller for the incoming request diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index ad44687f2..6f9212cc2 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -858,7 +858,7 @@ class HTTPRequest implements ArrayAccess public function setHttpMethod($method) { if (!self::isValidHttpMethod($method)) { - user_error('HTTPRequest::setHttpMethod: Invalid HTTP method', E_USER_ERROR); + throw new \InvalidArgumentException('HTTPRequest::setHttpMethod: Invalid HTTP method'); } $this->httpMethod = strtoupper($method); @@ -912,12 +912,11 @@ class HTTPRequest implements ArrayAccess { if (isset($postVars['_method'])) { if (!self::isValidHttpMethod($postVars['_method'])) { - user_error('HTTPRequest::detect_method(): Invalid "_method" parameter', E_USER_ERROR); + throw new InvalidArgumentException('HTTPRequest::detect_method(): Invalid "_method" parameter'); } return strtoupper($postVars['_method']); - } else { - return $origMethod; } + return $origMethod; } /** diff --git a/src/Control/RequestHandler.php b/src/Control/RequestHandler.php index 9d6122779..a10d88047 100644 --- a/src/Control/RequestHandler.php +++ b/src/Control/RequestHandler.php @@ -187,7 +187,7 @@ class RequestHandler extends ViewableData } $action = "index"; } elseif (!is_string($action)) { - user_error("Non-string method name: " . var_export($action, true), E_USER_ERROR); + throw new InvalidArgumentException("Non-string method name: " . var_export($action, true)); } $classMessage = Director::isLive() ? 'on this handler' : 'on class ' . static::class; diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php index 1cc1c177a..48de82f65 100644 --- a/src/Core/Extensible.php +++ b/src/Core/Extensible.php @@ -183,17 +183,17 @@ trait Extensible } $extensionClass = $matches[1]; if (!class_exists($extensionClass)) { - user_error( - sprintf('Object::add_extension() - Can\'t find extension class for "%s"', $extensionClass), - E_USER_ERROR - ); + throw new InvalidArgumentException(sprintf( + 'Object::add_extension() - Can\'t find extension class for "%s"', + $extensionClass + )); } - if (!is_subclass_of($extensionClass, 'SilverStripe\\Core\\Extension')) { - user_error( - sprintf('Object::add_extension() - Extension "%s" is not a subclass of Extension', $extensionClass), - E_USER_ERROR - ); + if (!is_subclass_of($extensionClass, Extension::class)) { + throw new InvalidArgumentException(sprintf( + 'Object::add_extension() - Extension "%s" is not a subclass of Extension', + $extensionClass + )); } // unset some caches @@ -334,7 +334,7 @@ trait Extensible $sources = []; foreach ($extensions as $extension) { - list($extensionClass, $extensionArgs) = ClassInfo::parse_class_spec($extension); + [$extensionClass, $extensionArgs] = ClassInfo::parse_class_spec($extension); // Strip service name specifier $extensionClass = strtok($extensionClass, '.'); $sources[] = $extensionClass; diff --git a/src/Dev/CsvBulkLoader.php b/src/Dev/CsvBulkLoader.php index f90f76eca..efa3773a0 100644 --- a/src/Dev/CsvBulkLoader.php +++ b/src/Dev/CsvBulkLoader.php @@ -344,7 +344,7 @@ class CsvBulkLoader extends BulkLoader } } elseif (strpos($fieldName, '.') !== false) { // we have a relation column with dot notation - list($relationName, $columnName) = explode('.', $fieldName); + [$relationName, $columnName] = explode('.', $fieldName); // always gives us an component (either empty or existing) $relationObj = $obj->getComponent($relationName); if (!$preview) { @@ -445,15 +445,19 @@ class CsvBulkLoader extends BulkLoader } elseif ($SNG_objectClass->hasMethod($duplicateCheck['callback'])) { $existingRecord = $SNG_objectClass->{$duplicateCheck['callback']}($record[$fieldName], $record); } else { - user_error("CsvBulkLoader::processRecord():" - . " {$duplicateCheck['callback']} not found on importer or object class.", E_USER_ERROR); + throw new \RuntimeException( + "CsvBulkLoader::processRecord():" + . " {$duplicateCheck['callback']} not found on importer or object class." + ); } if ($existingRecord) { return $existingRecord; } } else { - user_error('CsvBulkLoader::processRecord(): Wrong format for $duplicateChecks', E_USER_ERROR); + throw new \InvalidArgumentException( + 'CsvBulkLoader::processRecord(): Wrong format for $duplicateChecks' + ); } } diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index 83a294859..b79e1e5c5 100644 --- a/src/Dev/SapphireTest.php +++ b/src/Dev/SapphireTest.php @@ -480,11 +480,11 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly $id = $state->getFixtureFactory(static::class)->getId($className, $identifier); if (!$id) { - user_error(sprintf( + throw new \InvalidArgumentException(sprintf( "Couldn't find object '%s' (class: %s)", $identifier, $className - ), E_USER_ERROR); + )); } return $id; @@ -519,11 +519,11 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly $obj = $state->getFixtureFactory(static::class)->get($className, $identifier); if (!$obj) { - user_error(sprintf( + throw new \InvalidArgumentException(sprintf( "Couldn't find object '%s' (class: %s)", $identifier, $className - ), E_USER_ERROR); + )); } return $obj; @@ -1021,16 +1021,16 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly $app = new HTTPApplication($kernel); $flush = array_key_exists('flush', $request->getVars()); - + // Custom application $app->execute($request, function (HTTPRequest $request) { // Start session and execute $request->getSession()->init($request); - + // Invalidate classname spec since the test manifest will now pull out new subclasses for each internal class // (e.g. Member will now have various subclasses of DataObjects that implement TestOnly) DataObject::reset(); - + // Set dummy controller; $controller = Controller::create(); $controller->setRequest($request); diff --git a/src/Forms/CompositeField.php b/src/Forms/CompositeField.php index 20517d0b5..8a4bb5b42 100644 --- a/src/Forms/CompositeField.php +++ b/src/Forms/CompositeField.php @@ -251,11 +251,10 @@ class CompositeField extends FormField if (isset($list[$name])) { $fieldClass = get_class($field); $otherFieldClass = get_class($list[$name]); - user_error( + throw new \RuntimeException( "collateDataFields() I noticed that a field called '$name' appears twice in" . " your form: '{$formName}'. One is a '{$fieldClass}' and the other is a" - . " '{$otherFieldClass}'", - E_USER_ERROR + . " '{$otherFieldClass}'" ); } $list[$name] = $field; diff --git a/src/Forms/FieldList.php b/src/Forms/FieldList.php index eac95b74d..c1d117c50 100644 --- a/src/Forms/FieldList.php +++ b/src/Forms/FieldList.php @@ -165,15 +165,12 @@ class FieldList extends ArrayList $errorSuffix = ''; } - user_error( - sprintf( - "%s() I noticed that a field called '%s' appears twice%s", - $functionName, - $field->getName(), - $errorSuffix - ), - E_USER_ERROR - ); + throw new \RuntimeException(sprintf( + "%s() I noticed that a field called '%s' appears twice%s", + $functionName, + $field->getName(), + $errorSuffix + )); } protected function flushFieldsCache() @@ -213,9 +210,8 @@ class FieldList extends ArrayList } else { $errSuffix = ''; } - user_error( - "collateDataFields() I noticed that a field called '$name' appears twice$errSuffix.", - E_USER_ERROR + throw new \RuntimeException( + "collateDataFields() I noticed that a field called '$name' appears twice$errSuffix." ); } $list[$name] = $field; @@ -493,10 +489,9 @@ class FieldList extends ArrayList ? " named '{$parentPointer->getName()}'" : null; $parentPointerClass = get_class($parentPointer); - user_error( + throw new \InvalidArgumentException( "FieldList::addFieldToTab() Tried to add a tab to object" - . " '{$parentPointerClass}'{$withName} - '{$part}' didn't exist.", - E_USER_ERROR + . " '{$parentPointerClass}'{$withName} - '{$part}' didn't exist." ); } } diff --git a/src/Forms/FormField.php b/src/Forms/FormField.php index fc6526589..414ba2808 100644 --- a/src/Forms/FormField.php +++ b/src/Forms/FormField.php @@ -1414,12 +1414,7 @@ class FormField extends RequestHandler } $class = static::class; - user_error( - "rootFieldList() called on {$class} object without a containerFieldList", - E_USER_ERROR - ); - - return null; + throw new \RuntimeException("rootFieldList() called on {$class} object without a containerFieldList"); } /** diff --git a/src/Forms/SelectField.php b/src/Forms/SelectField.php index 2a93589de..ec054cb2e 100644 --- a/src/Forms/SelectField.php +++ b/src/Forms/SelectField.php @@ -173,7 +173,7 @@ abstract class SelectField extends FormField $source = $source->toArray(); } if (!is_array($source) && !($source instanceof ArrayAccess)) { - user_error('$source passed in as invalid type', E_USER_ERROR); + throw new \InvalidArgumentException('$source passed in as invalid type'); } return $source; diff --git a/src/Forms/TreeMultiselectField.php b/src/Forms/TreeMultiselectField.php index c7ee21f30..2857a5c30 100644 --- a/src/Forms/TreeMultiselectField.php +++ b/src/Forms/TreeMultiselectField.php @@ -2,6 +2,7 @@ namespace SilverStripe\Forms; +use http\Exception\InvalidArgumentException; use SilverStripe\Control\Controller; use SilverStripe\Core\Convert; use SilverStripe\ORM\ArrayList; @@ -252,10 +253,9 @@ class TreeMultiselectField extends TreeDropdownField $saveDest = $record->$fieldName(); if (!$saveDest) { $recordClass = get_class($record); - user_error( + throw new \RuntimeException( "TreeMultiselectField::saveInto() Field '$fieldName' not found on" - . " {$recordClass}.{$record->ID}", - E_USER_ERROR + . " {$recordClass}.{$record->ID}" ); } diff --git a/src/ORM/Connect/Database.php b/src/ORM/Connect/Database.php index 75cfae10f..d8920a8d4 100644 --- a/src/ORM/Connect/Database.php +++ b/src/ORM/Connect/Database.php @@ -431,9 +431,8 @@ abstract class Database break; default: - user_error( + throw new \InvalidArgumentException( "SS_Database::manipulate() Can't recognise command '{$writeInfo['command']}'", - E_USER_ERROR ); } } diff --git a/src/ORM/Connect/MySQLiConnector.php b/src/ORM/Connect/MySQLiConnector.php index 2b2ae023a..354b05e68 100644 --- a/src/ORM/Connect/MySQLiConnector.php +++ b/src/ORM/Connect/MySQLiConnector.php @@ -2,9 +2,9 @@ namespace SilverStripe\ORM\Connect; -use SilverStripe\Core\Config\Config; use mysqli; use mysqli_stmt; +use SilverStripe\Core\Config\Config; /** * Connector for MySQL using the MySQLi method @@ -238,11 +238,9 @@ class MySQLiConnector extends DBConnector case 'array': case 'unknown type': default: - user_error( - "Cannot bind parameter \"$value\" as it is an unsupported type ($phpType)", - E_USER_ERROR + throw new \InvalidArgumentException( + "Cannot bind parameter \"$value\" as it is an unsupported type ($phpType)" ); - break; } $values[] = $value; } diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index ea5077a08..6d6275fc2 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -1274,7 +1274,7 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li */ public function offsetSet($key, $value) { - user_error("Can't alter items in a DataList using array-access", E_USER_ERROR); + throw new \BadMethodCallException("Can't alter items in a DataList using array-access"); } /** @@ -1284,6 +1284,6 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li */ public function offsetUnset($key) { - user_error("Can't alter items in a DataList using array-access", E_USER_ERROR); + throw new \BadMethodCallException("Can't alter items in a DataList using array-access"); } } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 511bfaef0..2e8888bde 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1391,8 +1391,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity $this->brokenOnWrite = true; $this->onBeforeWrite(); if ($this->brokenOnWrite) { - user_error(static::class . " has a broken onBeforeWrite() function." - . " Make sure that you call parent::onBeforeWrite().", E_USER_ERROR); + throw new LogicException( + static::class . " has a broken onBeforeWrite() function." + . " Make sure that you call parent::onBeforeWrite()." + ); } } @@ -1737,8 +1739,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity $this->brokenOnDelete = true; $this->onBeforeDelete(); if ($this->brokenOnDelete) { - user_error(static::class . " has a broken onBeforeDelete() function." - . " Make sure that you call parent::onBeforeDelete().", E_USER_ERROR); + throw new LogicException( + static::class . " has a broken onBeforeDelete() function." + . " Make sure that you call parent::onBeforeDelete()." + ); } // Deleting a record without an ID shouldn't do anything @@ -2874,7 +2878,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity public function setCastedField($fieldName, $value) { if (!$fieldName) { - user_error("DataObject::setCastedField: Called without a fieldName", E_USER_ERROR); + throw new InvalidArgumentException("DataObject::setCastedField: Called without a fieldName"); } $fieldObj = $this->dbObject($fieldName); if ($fieldObj) { diff --git a/src/ORM/FieldType/DBBoolean.php b/src/ORM/FieldType/DBBoolean.php index eae84b088..0a75eea95 100644 --- a/src/ORM/FieldType/DBBoolean.php +++ b/src/ORM/FieldType/DBBoolean.php @@ -35,7 +35,7 @@ class DBBoolean extends DBField public function Nice() { - return ($this->value) ? _t('SilverStripe\\ORM\\FieldType\\DBBoolean.YESANSWER', 'Yes') : _t('SilverStripe\\ORM\\FieldType\\DBBoolean.NOANSWER', 'No'); + return ($this->value) ? _t(__CLASS__ . '.YESANSWER', 'Yes') : _t(__CLASS__ . '.NOANSWER', 'No'); } public function NiceAsBoolean() @@ -50,7 +50,7 @@ class DBBoolean extends DBField $dataObject->$fieldName = ($this->value) ? 1 : 0; } else { $class = static::class; - user_error("DBField::saveInto() Called on a nameless '$class' object", E_USER_ERROR); + throw new \RuntimeException("DBField::saveInto() Called on a nameless '$class' object"); } } @@ -61,10 +61,10 @@ class DBBoolean extends DBField public function scaffoldSearchField($title = null) { - $anyText = _t('SilverStripe\\ORM\\FieldType\\DBBoolean.ANY', 'Any'); + $anyText = _t(__CLASS__ . '.ANY', 'Any'); $source = [ - 1 => _t('SilverStripe\\ORM\\FieldType\\DBBoolean.YESANSWER', 'Yes'), - 0 => _t('SilverStripe\\ORM\\FieldType\\DBBoolean.NOANSWER', 'No') + 1 => _t(__CLASS__ . '.YESANSWER', 'Yes'), + 0 => _t(__CLASS__ . '.NOANSWER', 'No') ]; $field = new DropdownField($this->name, $title, $source); diff --git a/src/ORM/FieldType/DBDecimal.php b/src/ORM/FieldType/DBDecimal.php index d0b39a616..6e11b7bdc 100644 --- a/src/ORM/FieldType/DBDecimal.php +++ b/src/ORM/FieldType/DBDecimal.php @@ -90,7 +90,9 @@ class DBDecimal extends DBField if ($fieldName) { $dataObject->$fieldName = (float)preg_replace('/[^0-9.\-\+]/', '', $this->value); } else { - user_error("DBField::saveInto() Called on a nameless '" . static::class . "' object", E_USER_ERROR); + throw new \UnexpectedValueException( + "DBField::saveInto() Called on a nameless '" . static::class . "' object" + ); } } diff --git a/src/ORM/FieldType/DBEnum.php b/src/ORM/FieldType/DBEnum.php index 28ea758a8..ab9da9781 100644 --- a/src/ORM/FieldType/DBEnum.php +++ b/src/ORM/FieldType/DBEnum.php @@ -5,6 +5,7 @@ namespace SilverStripe\ORM\FieldType; use SilverStripe\Core\Config\Config; use SilverStripe\Forms\DropdownField; use SilverStripe\ORM\ArrayLib; +use SilverStripe\ORM\Connect\MySQLDatabase; use SilverStripe\ORM\DB; /** @@ -77,9 +78,8 @@ class DBEnum extends DBString if (in_array($default, $enum)) { $this->setDefault($default); } else { - user_error( - "Enum::__construct() The default value '$default' does not match any item in the enumeration", - E_USER_ERROR + throw new \InvalidArgumentException( + "Enum::__construct() The default value '$default' does not match any item in the enumeration" ); } } elseif (is_int($default) && $default < count($enum)) { @@ -99,8 +99,8 @@ class DBEnum extends DBString */ public function requireField() { - $charset = Config::inst()->get('SilverStripe\ORM\Connect\MySQLDatabase', 'charset'); - $collation = Config::inst()->get('SilverStripe\ORM\Connect\MySQLDatabase', 'collation'); + $charset = Config::inst()->get(MySQLDatabase::class, 'charset'); + $collation = Config::inst()->get(MySQLDatabase::class, 'collation'); $parts = [ 'datatype' => 'enum', diff --git a/src/ORM/FieldType/DBMultiEnum.php b/src/ORM/FieldType/DBMultiEnum.php index a39c5aa1e..b4afcad4c 100644 --- a/src/ORM/FieldType/DBMultiEnum.php +++ b/src/ORM/FieldType/DBMultiEnum.php @@ -3,6 +3,7 @@ namespace SilverStripe\ORM\FieldType; use SilverStripe\Core\Config\Config; +use SilverStripe\ORM\Connect\MySQLDatabase; use SilverStripe\ORM\DB; use SilverStripe\Forms\CheckboxSetField; @@ -22,9 +23,10 @@ class DBMultiEnum extends DBEnum $defaults = preg_split('/ *, */', trim($default)); foreach ($defaults as $thisDefault) { if (!in_array($thisDefault, $this->enum)) { - user_error("Enum::__construct() The default value '$thisDefault' does not match " - . "any item in the enumeration", E_USER_ERROR); - return; + throw new \InvalidArgumentException( + "Enum::__construct() The default value '$thisDefault' does not match " + . "any item in the enumeration" + ); } } $this->default = implode(',', $defaults); @@ -34,8 +36,8 @@ class DBMultiEnum extends DBEnum public function requireField() { // @todo: Remove mysql-centric logic from this - $charset = Config::inst()->get('SilverStripe\ORM\Connect\MySQLDatabase', 'charset'); - $collation = Config::inst()->get('SilverStripe\ORM\Connect\MySQLDatabase', 'collation'); + $charset = Config::inst()->get(MySQLDatabase::class, 'charset'); + $collation = Config::inst()->get(MySQLDatabase::class, 'collation'); $values=[ 'type'=>'set', 'parts'=>[ diff --git a/src/ORM/HasManyList.php b/src/ORM/HasManyList.php index 13fcbacaa..c93c9cb6e 100644 --- a/src/ORM/HasManyList.php +++ b/src/ORM/HasManyList.php @@ -74,7 +74,7 @@ class HasManyList extends RelationList if (is_numeric($item)) { $item = DataObject::get_by_id($this->dataClass, $item); } elseif (!($item instanceof $this->dataClass)) { - user_error("HasManyList::add() expecting a $this->dataClass object, or ID value", E_USER_ERROR); + throw new InvalidArgumentException("HasManyList::add() expecting a $this->dataClass object, or ID value"); } $foreignID = $this->getForeignID(); @@ -123,10 +123,7 @@ class HasManyList extends RelationList public function remove($item) { if (!($item instanceof $this->dataClass)) { - throw new InvalidArgumentException( - "HasManyList::remove() expecting a $this->dataClass object, or ID", - E_USER_ERROR - ); + throw new InvalidArgumentException("HasManyList::remove() expecting a $this->dataClass object, or ID"); } // Don't remove item which doesn't belong to this list diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index f5eb22375..0b344e449 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -368,9 +368,8 @@ class Hierarchy extends DataExtension // Validate the ID list foreach ($idList as $id) { if (!is_numeric($id)) { - user_error( - "Bad ID passed to Versioned::prepopulate_numchildren_cache() in \$idList: " . $id, - E_USER_ERROR + throw new \InvalidArgumentException( + "Bad ID passed to Versioned::prepopulate_numchildren_cache() in \$idList: " . $id ); } } diff --git a/src/ORM/Map.php b/src/ORM/Map.php index 127d55fb6..c3dac10d4 100644 --- a/src/ORM/Map.php +++ b/src/ORM/Map.php @@ -212,10 +212,7 @@ class Map implements ArrayAccess, Countable, IteratorAggregate $this->lastItems[$key] = $value; } - user_error( - 'Map is read-only. Please use $map->push($key, $value) to append values', - E_USER_ERROR - ); + throw new \BadMethodCallException('Map is read-only. Please use $map->push($key, $value) to append values'); } /** @@ -243,9 +240,8 @@ class Map implements ArrayAccess, Countable, IteratorAggregate return; } - user_error( - "Map is read-only. Unset cannot be called on keys derived from the DataQuery", - E_USER_ERROR + throw new \BadMethodCallException( + 'Map is read-only. Unset cannot be called on keys derived from the DataQuery' ); } diff --git a/src/ORM/PolymorphicHasManyList.php b/src/ORM/PolymorphicHasManyList.php index cb9ddbbea..308686987 100644 --- a/src/ORM/PolymorphicHasManyList.php +++ b/src/ORM/PolymorphicHasManyList.php @@ -109,8 +109,7 @@ class PolymorphicHasManyList extends HasManyList { if (!($item instanceof $this->dataClass)) { throw new InvalidArgumentException( - "HasManyList::remove() expecting a $this->dataClass object, or ID", - E_USER_ERROR + "HasManyList::remove() expecting a $this->dataClass object, or ID" ); } diff --git a/src/ORM/Queries/SQLConditionalExpression.php b/src/ORM/Queries/SQLConditionalExpression.php index 9ba2d97dc..be5e37bf9 100644 --- a/src/ORM/Queries/SQLConditionalExpression.php +++ b/src/ORM/Queries/SQLConditionalExpression.php @@ -612,9 +612,8 @@ abstract class SQLConditionalExpression extends SQLExpression if ($parameterCount === 1) { $key .= " = ?"; } elseif ($parameterCount > 1) { - user_error( - "Incorrect number of '?' in predicate $key. Expected $parameterCount but none given.", - E_USER_ERROR + throw new \InvalidArgumentException( + "Incorrect number of '?' in predicate $key. Expected $parameterCount but none given." ); } } @@ -622,9 +621,11 @@ abstract class SQLConditionalExpression extends SQLExpression } elseif (is_array($value)) { // If predicates are nested one per array (as per the internal format) // then run a quick check over the contents and recursively parse - if (count($value) != 1) { - user_error('Nested predicates should be given as a single item array in ' - . 'array($predicate => array($prameters)) format)', E_USER_ERROR); + if (count($value) !== 1) { + throw new \InvalidArgumentException( + 'Nested predicates should be given as a single item array in ' + . 'array($predicate => array($prameters)) format)' + ); } foreach ($value as $key => $pairValue) { return $this->parsePredicate($key, $pairValue); diff --git a/src/ORM/UnsavedRelationList.php b/src/ORM/UnsavedRelationList.php index ba65e2ba9..1e940383b 100644 --- a/src/ORM/UnsavedRelationList.php +++ b/src/ORM/UnsavedRelationList.php @@ -95,10 +95,10 @@ class UnsavedRelationList extends ArrayList implements Relation public function push($item, $extraFields = null) { if ((is_object($item) && !$item instanceof $this->dataClass) - || (!is_object($item) && !is_numeric($item))) { + || (!is_object($item) && !is_numeric($item)) + ) { throw new InvalidArgumentException( - "UnsavedRelationList::add() expecting a $this->dataClass object, or ID value", - E_USER_ERROR + "UnsavedRelationList::add() expecting a $this->dataClass object, or ID value" ); } if (is_object($item) && $item->ID) { diff --git a/src/Security/Member_GroupSet.php b/src/Security/Member_GroupSet.php index f2f948c6b..bc51f6d8d 100644 --- a/src/Security/Member_GroupSet.php +++ b/src/Security/Member_GroupSet.php @@ -19,7 +19,7 @@ class Member_GroupSet extends ManyManyList { // Do not join the table directly if ($this->extraFields) { - user_error('Member_GroupSet does not support many_many_extraFields', E_USER_ERROR); + throw new \BadMethodCallException('Member_GroupSet does not support many_many_extraFields'); } } diff --git a/src/Security/Permission.php b/src/Security/Permission.php index 4db499ed7..6866d3d19 100644 --- a/src/Security/Permission.php +++ b/src/Security/Permission.php @@ -246,7 +246,7 @@ class Permission extends DataObject implements TemplateGlobalProvider, Resettabl $argClause = "AND \"Arg\" IN (?, ?) "; $argParams = [-1, $arg]; } else { - user_error("Permission::checkMember: bad arg '$arg'", E_USER_ERROR); + throw new \InvalidArgumentException("Permission::checkMember: bad arg '$arg'"); } } @@ -408,10 +408,7 @@ class Permission extends DataObject implements TemplateGlobalProvider, Resettabl if (is_numeric($arg)) { $perm->Arg = $arg; } else { - user_error( - "Permission::checkMember: bad arg '$arg'", - E_USER_ERROR - ); + throw new \InvalidArgumentException("Permission::checkMember: bad arg '$arg'"); } } @@ -446,10 +443,7 @@ class Permission extends DataObject implements TemplateGlobalProvider, Resettabl if (is_numeric($arg)) { $perm->Arg = $arg; } else { - user_error( - "Permission::checkMember: bad arg '$arg'", - E_USER_ERROR - ); + throw new \InvalidArgumentException("Permission::checkMember: bad arg '$arg'"); } } diff --git a/src/View/Parsers/ShortcodeParser.php b/src/View/Parsers/ShortcodeParser.php index ffa26cd33..1a4f468cc 100644 --- a/src/View/Parsers/ShortcodeParser.php +++ b/src/View/Parsers/ShortcodeParser.php @@ -191,7 +191,7 @@ class ShortcodeParser // Missing tag if ($content === false) { if (ShortcodeParser::$error_behavior == ShortcodeParser::ERROR) { - user_error('Unknown shortcode tag ' . $tag['open'], E_USER_ERROR); + throw new \InvalidArgumentException('Unknown shortcode tag ' . $tag['open']); } elseif (self::$error_behavior == self::WARN && $isHTMLAllowed) { $content = '' . $tag['text'] . ''; } elseif (ShortcodeParser::$error_behavior == ShortcodeParser::STRIP) { @@ -381,7 +381,7 @@ class ShortcodeParser if ($err) { if (self::$error_behavior == self::ERROR) { - user_error($err, E_USER_ERROR); + throw new \Exception($err); } } else { if ($tags[$i]['escaped']) { @@ -603,7 +603,7 @@ class ShortcodeParser } // NOP } else { - user_error('Unknown value for $location argument ' . $location, E_USER_ERROR); + throw new \UnexpectedValueException('Unknown value for $location argument ' . $location); } } @@ -664,7 +664,7 @@ class ShortcodeParser // Now parse the result into a DOM if (!$htmlvalue->isValid()) { if (self::$error_behavior == self::ERROR) { - user_error('Couldn\'t decode HTML when processing short codes', E_USER_ERROR); + throw new \Exception('Couldn\'t decode HTML when processing short codes'); } else { $continue = false; } @@ -706,9 +706,8 @@ class ShortcodeParser if (!$parent) { if ($location !== self::INLINE) { - user_error( - "Parent block for shortcode couldn't be found, but location wasn't INLINE", - E_USER_ERROR + throw new \RuntimeException( + "Parent block for shortcode couldn't be found, but location wasn't INLINE" ); } } else { diff --git a/src/View/SSViewer_DataPresenter.php b/src/View/SSViewer_DataPresenter.php index dafc9e3e3..1d37875ee 100644 --- a/src/View/SSViewer_DataPresenter.php +++ b/src/View/SSViewer_DataPresenter.php @@ -249,7 +249,7 @@ class SSViewer_DataPresenter extends SSViewer_Scope case 'Up': $upIndex = $this->getUpIndex(); if ($upIndex === null) { - user_error('Up called when we\'re already at the top of the scope', E_USER_ERROR); + throw new \LogicException('Up called when we\'re already at the top of the scope'); } $overlayIndex = $upIndex; // Parent scope diff --git a/src/View/SSViewer_Scope.php b/src/View/SSViewer_Scope.php index fdbce4c73..250a965dd 100644 --- a/src/View/SSViewer_Scope.php +++ b/src/View/SSViewer_Scope.php @@ -191,7 +191,7 @@ class SSViewer_Scope switch ($name) { case 'Up': if ($this->upIndex === null) { - user_error('Up called when we\'re already at the top of the scope', E_USER_ERROR); + throw new \LogicException('Up called when we\'re already at the top of the scope'); } list( From ae1e17edeca1334bf0d6ad459a2111fa1b96a09e Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 25 Sep 2020 09:30:52 -0700 Subject: [PATCH 2/2] Update exception assertions in tests and remove deprecated annotations --- src/ORM/Connect/Database.php | 2 +- tests/php/Control/HTTPRequestTest.php | 8 ++------ tests/php/Forms/CompositeFieldTest.php | 10 +++++----- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/ORM/Connect/Database.php b/src/ORM/Connect/Database.php index d8920a8d4..da66d9314 100644 --- a/src/ORM/Connect/Database.php +++ b/src/ORM/Connect/Database.php @@ -432,7 +432,7 @@ abstract class Database default: throw new \InvalidArgumentException( - "SS_Database::manipulate() Can't recognise command '{$writeInfo['command']}'", + "SS_Database::manipulate() Can't recognise command '{$writeInfo['command']}'" ); } } diff --git a/tests/php/Control/HTTPRequestTest.php b/tests/php/Control/HTTPRequestTest.php index 51ea76fa3..52ce61b49 100644 --- a/tests/php/Control/HTTPRequestTest.php +++ b/tests/php/Control/HTTPRequestTest.php @@ -160,11 +160,9 @@ class HTTPRequestTest extends SapphireTest $this->assertEquals($expected, $actual); } - /** - * @expectedException PHPUnit_Framework_Error - */ public function testBadDetectMethod() { + $this->expectException(\InvalidArgumentException::class); HTTPRequest::detect_method('POST', ['_method' => 'Boom']); } @@ -191,11 +189,9 @@ class HTTPRequestTest extends SapphireTest $this->assertEquals($request, $returnedRequest); } - /** - * @expectedException PHPUnit_Framework_Error - */ public function testBadSetHttpMethod() { + $this->expectException(\InvalidArgumentException::class); $request = new HTTPRequest('GET', '/hello'); $request->setHttpMethod('boom'); } diff --git a/tests/php/Forms/CompositeFieldTest.php b/tests/php/Forms/CompositeFieldTest.php index 382c339dc..6bc72cda4 100644 --- a/tests/php/Forms/CompositeFieldTest.php +++ b/tests/php/Forms/CompositeFieldTest.php @@ -2,7 +2,6 @@ namespace SilverStripe\Forms\Tests; -use PHPUnit_Framework_Error; use SilverStripe\Dev\CSSContentParser; use SilverStripe\Dev\SapphireTest; use SilverStripe\Forms\CompositeField; @@ -161,12 +160,13 @@ class CompositeFieldTest extends SapphireTest $this->assertNull($result['title']); } - /** - * @expectedException PHPUnit_Framework_Error - * @expectedExceptionMessageRegExp /a field called 'Test' appears twice in your form.*TextField.*TextField/ - */ public function testCollateDataFieldsThrowsErrorOnDuplicateChildren() { + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageRegExp( + "/a field called 'Test' appears twice in your form.*TextField.*TextField/" + ); + $field = CompositeField::create( TextField::create('Test'), TextField::create('Test')