From 4c3a068859e94050797b01c16b57786dff4fa741 Mon Sep 17 00:00:00 2001 From: Simon Gow Date: Wed, 29 Aug 2018 14:34:21 +1200 Subject: [PATCH 1/8] Issue 3357 - Add GridField Readonly Transformation GridField doesn't have a valid readonly state if it's value is set to an Object without `forTemplate()`. The default behaviour is to render a ReadonlyField, but given GridField is a complex type this isn't suitable. This bugfix provides a transformation method to render only components that are whitelisted to provide a readonly state. @see #3357 - https://github.com/silverstripe/silverstripe-framework/issues/3357 --- src/Forms/GridField/GridField.php | 69 +++++++++++++++ .../Forms/GridField/GridFieldReadonlyTest.php | 87 +++++++++++++++++++ .../Forms/GridField/GridFieldReadonlyTest.yml | 27 ++++++ 3 files changed, 183 insertions(+) create mode 100644 tests/php/Forms/GridField/GridFieldReadonlyTest.php create mode 100644 tests/php/Forms/GridField/GridFieldReadonlyTest.yml diff --git a/src/Forms/GridField/GridField.php b/src/Forms/GridField/GridField.php index b66dc274e..5d959596d 100644 --- a/src/Forms/GridField/GridField.php +++ b/src/Forms/GridField/GridField.php @@ -105,6 +105,21 @@ class GridField extends FormField */ protected $name = ''; + /** + * A whitelist of readonly component classes allowed if performReadonlyTransform is called. + * + * @var array + */ + protected $readonlyComponents = array( + GridFieldDetailForm::class, + GridFieldDataColumns::class, + GridFieldConfig_RecordViewer::class, + GridFieldToolbarHeader::class, + GridFieldPageCount::class, + GridFieldPaginator::class, + GridState_Component::class + ); + /** * Pattern used for looking up */ @@ -193,6 +208,60 @@ class GridField extends FormField ); } + /** + * Overload the readonly components for this gridfield. + * + * @param array $components an array map of component class references to whitelist for a readonly version. + */ + public function setReadonlyComponents(array $components) + { + $this->readonlyComponents = $components; + } + + /** + * Return the readonly components + * + * @return array a map of component classes. + */ + public function getReadonlyComponents() + { + return $this->readonlyComponents; + } + + /** + * Custom Readonly transformation to remove actions which shouldn't be present for a readonly state. + * + * @return GridField + */ + public function performReadonlyTransformation() + { + $copy = clone $this; + $copy->setReadonly(true); + + // get the whitelist for allowable readonly components + $allowedComponents = $this->getReadonlyComponents(); + foreach ($this->getConfig()->getComponents() as $component) { + + // if a component doesn't exist, remove it from the readonly version. + if (!in_array(get_class($component), $allowedComponents)) { + $copy->getConfig()->removeComponent($component); + } + } + + return $copy; + } + + /** + * Disabling the gridfield should have the same affect as making it readonly (removing all action items). + * + * @return GridField + */ + public function performDisabledTransformation(){ + parent::performDisabledTransformation(); + + return $this->performReadonlyTransformation(); + } + /** * @return GridFieldConfig */ diff --git a/tests/php/Forms/GridField/GridFieldReadonlyTest.php b/tests/php/Forms/GridField/GridFieldReadonlyTest.php new file mode 100644 index 000000000..bb3d7136f --- /dev/null +++ b/tests/php/Forms/GridField/GridFieldReadonlyTest.php @@ -0,0 +1,87 @@ +getComponents('Cheerleaders'); + + $gridConfig = GridFieldConfig_RelationEditor::create(); + + // Build some commonly used components to make sure we're only allowing the correct components + $gridConfig->addComponent(new GridFieldButtonRow('before')); + $gridConfig->addComponent(new GridFieldAddNewButton('buttons-before-left')); + $gridConfig->addComponent(new GridFieldAddExistingAutocompleter('buttons-before-right')); + $gridConfig->addComponent(new GridFieldToolbarHeader()); + $gridConfig->addComponent($sort = new GridFieldSortableHeader()); + $gridConfig->addComponent($filter = new GridFieldFilterHeader()); + $gridConfig->addComponent(new GridFieldDataColumns()); + $gridConfig->addComponent(new GridFieldEditButton()); + $gridConfig->addComponent(new GridFieldDeleteAction(true)); + $gridConfig->addComponent(new GridField_ActionMenu()); + $gridConfig->addComponent(new GridFieldPageCount('toolbar-header-right')); + $gridConfig->addComponent($pagination = new GridFieldPaginator(2)); + $gridConfig->addComponent(new GridFieldDetailForm()); + $gridConfig->addComponent(new GridFieldDeleteAction()); + $gridConfig->addComponent(new VersionedGridFieldState()); + + $gridField = GridField::create( + 'Cheerleaders', + 'Cheerleaders', + $components, + $gridConfig + ); + + // Model Admin sets the value of the GridField directly to the relation, which doesn't have a forTemplate() + // function, if we rely on FormField to render into a ReadonlyField we'll get an error as HasManyRelation + // doesn't have a forTemplate() function. + $gridField->setValue($components); + $gridField->setModelClass(Cheerleader::class); + + // This function is called by $form->makeReadonly(). + $readonlyGridField = $gridField->performReadonlyTransformation(); + + // if we've made it this far, then the GridField is at least transforming correctly. + $readonlyComponents = $readonlyGridField->getReadonlyComponents(); + + // assert that all the components in the readonly version are present in the whitelist. + foreach($readonlyGridField->getConfig()->getComponents() as $component){ + $this->assertTrue(in_array(get_class($component), $readonlyComponents)); + } + } +} diff --git a/tests/php/Forms/GridField/GridFieldReadonlyTest.yml b/tests/php/Forms/GridField/GridFieldReadonlyTest.yml new file mode 100644 index 000000000..62855505e --- /dev/null +++ b/tests/php/Forms/GridField/GridFieldReadonlyTest.yml @@ -0,0 +1,27 @@ +SilverStripe\Forms\Tests\GridField\GridFieldTest\Team: + team1: + Name: Team 1 + City: Cologne + team2: + Name: Team 2 + City: Wellington + team3: + Name: Team 3 + City: Auckland + team4: + Name: Team 4 + City: Melbourne + +SilverStripe\Forms\Tests\GridField\GridFieldTest\Cheerleader: + cheerleader1: + Name: Heather + Team: =>SilverStripe\Forms\Tests\GridField\GridFieldTest\Team.team1 + cheerleader2: + Name: Bob + Team: =>SilverStripe\Forms\Tests\GridField\GridFieldTest\Team.team1 + cheerleader3: + Name: Jenny + Team: =>SilverStripe\Forms\Tests\GridField\GridFieldTest\Team.team1 + cheerleader4: + Name: Sam + Team: =>SilverStripe\Forms\Tests\GridField\GridFieldTest\Team.team1 From 710bc683fd3a72e3d9383e53c1725fc7a23e6f4b Mon Sep 17 00:00:00 2001 From: Stephan Date: Mon, 10 Sep 2018 16:10:35 +0200 Subject: [PATCH 2/8] Removed SS_CONFIGSTATICMANIFEST --- docs/en/00_Getting_Started/03_Environment_Management.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/en/00_Getting_Started/03_Environment_Management.md b/docs/en/00_Getting_Started/03_Environment_Management.md index 3c0299494..e1e98c5a6 100644 --- a/docs/en/00_Getting_Started/03_Environment_Management.md +++ b/docs/en/00_Getting_Started/03_Environment_Management.md @@ -94,7 +94,6 @@ SilverStripe core environment variables are listed here, though you're free to d | `SS_MANIFESTCACHE` | The manifest cache to use (defaults to file based caching). Must be a CacheInterface or CacheFactory class name | | `SS_IGNORE_DOT_ENV` | If set the .env file will be ignored. This is good for live to mitigate any performance implications of loading the .env file | | `SS_BASE_URL` | The url to use when it isn't determinable by other means (eg: for CLI commands) | -| `SS_CONFIGSTATICMANIFEST` | Set to `SS_ConfigStaticManifest_Reflection` to use the Silverstripe 4 Reflection config manifest (speed improvement during dev/build and ?flush) | | `SS_DATABASE_SSL_KEY` | Absolute path to SSL key file | | `SS_DATABASE_SSL_CERT` | Absolute path to SSL certificate file | | `SS_DATABASE_SSL_CA` | Absolute path to SSL Certificate Authority bundle file | From db63f55fbb8e635e4e7215b7b7eff4e1f1cb7b22 Mon Sep 17 00:00:00 2001 From: Luke Edwards Date: Tue, 18 Sep 2018 13:24:55 +1200 Subject: [PATCH 3/8] BUG Changes being detected on TreeMulti as values not sorted --- src/Forms/TreeMultiselectField.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Forms/TreeMultiselectField.php b/src/Forms/TreeMultiselectField.php index 72de4dc11..819c086b9 100644 --- a/src/Forms/TreeMultiselectField.php +++ b/src/Forms/TreeMultiselectField.php @@ -103,7 +103,12 @@ class TreeMultiselectField extends TreeDropdownField // cannot rely on $this->value as this could be a many-many relationship $value = array_column($values, 'id'); - $data['value'] = ($value) ? $value : 'unchanged'; + if ($value) { + sort($value); + $data['value'] = $value; + } else { + $data['value'] = 'unchanged'; + } return $data; } @@ -182,6 +187,7 @@ class TreeMultiselectField extends TreeDropdownField } $title = implode(", ", $titleArray); + sort($idArray); $value = implode(",", $idArray); } else { $title = $emptyTitle; From 3fc49dd4ce222b14881b76c57c4bde24a5d4a6cf Mon Sep 17 00:00:00 2001 From: Luke Edwards Date: Thu, 20 Sep 2018 13:32:52 +1200 Subject: [PATCH 4/8] Lint fixes and allow a few other components by default --- src/Forms/GridField/GridField.php | 13 ++++++++----- .../GridField/GridFieldConfig_RecordViewer.php | 1 + tests/php/Forms/GridField/GridFieldReadonlyTest.php | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/Forms/GridField/GridField.php b/src/Forms/GridField/GridField.php index 5d959596d..629b1e681 100644 --- a/src/Forms/GridField/GridField.php +++ b/src/Forms/GridField/GridField.php @@ -111,13 +111,16 @@ class GridField extends FormField * @var array */ protected $readonlyComponents = array( + GridField_ActionMenu::class, + GridState_Component::class, + GridFieldConfig_RecordViewer::class, GridFieldDetailForm::class, GridFieldDataColumns::class, - GridFieldConfig_RecordViewer::class, - GridFieldToolbarHeader::class, GridFieldPageCount::class, GridFieldPaginator::class, - GridState_Component::class + GridFieldSortableHeader::class, + GridFieldToolbarHeader::class, + GridFieldViewButton::class, ); /** @@ -241,7 +244,6 @@ class GridField extends FormField // get the whitelist for allowable readonly components $allowedComponents = $this->getReadonlyComponents(); foreach ($this->getConfig()->getComponents() as $component) { - // if a component doesn't exist, remove it from the readonly version. if (!in_array(get_class($component), $allowedComponents)) { $copy->getConfig()->removeComponent($component); @@ -256,7 +258,8 @@ class GridField extends FormField * * @return GridField */ - public function performDisabledTransformation(){ + public function performDisabledTransformation() + { parent::performDisabledTransformation(); return $this->performReadonlyTransformation(); diff --git a/src/Forms/GridField/GridFieldConfig_RecordViewer.php b/src/Forms/GridField/GridFieldConfig_RecordViewer.php index 07ea8977a..fdaa91139 100644 --- a/src/Forms/GridField/GridFieldConfig_RecordViewer.php +++ b/src/Forms/GridField/GridFieldConfig_RecordViewer.php @@ -14,6 +14,7 @@ class GridFieldConfig_RecordViewer extends GridFieldConfig_Base $this->addComponent(new GridFieldViewButton()); $this->addComponent(new GridFieldDetailForm()); + $this->removeComponentsByType(GridFieldFilterHeader::class); $this->extend('updateConfig'); } diff --git a/tests/php/Forms/GridField/GridFieldReadonlyTest.php b/tests/php/Forms/GridField/GridFieldReadonlyTest.php index bb3d7136f..c4a1e1deb 100644 --- a/tests/php/Forms/GridField/GridFieldReadonlyTest.php +++ b/tests/php/Forms/GridField/GridFieldReadonlyTest.php @@ -80,7 +80,7 @@ class GridFieldReadonlyTest extends SapphireTest $readonlyComponents = $readonlyGridField->getReadonlyComponents(); // assert that all the components in the readonly version are present in the whitelist. - foreach($readonlyGridField->getConfig()->getComponents() as $component){ + foreach ($readonlyGridField->getConfig()->getComponents() as $component) { $this->assertTrue(in_array(get_class($component), $readonlyComponents)); } } From c269a987d5305bd5efc20a15a15b2f7867840dd4 Mon Sep 17 00:00:00 2001 From: Simon Gow Date: Wed, 19 Sep 2018 14:53:12 +1200 Subject: [PATCH 5/8] Performance issues with BasicAuth and LoginAttempts Two functions interact with the LoginAttempt object which when used in conjunction with BasicAuth result in significant performance degradation over time, as the LoginAttempts Table fills. This fix adds an index to the lookup column EmailHashed and removes the Email filter part of getByEmail() so it can use the index resulting in a much faster query. For more information see https://github.com/silverstripe/silverstripe-framework/issues/8389 --- src/Security/LoginAttempt.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Security/LoginAttempt.php b/src/Security/LoginAttempt.php index 1a2b68939..832764a51 100644 --- a/src/Security/LoginAttempt.php +++ b/src/Security/LoginAttempt.php @@ -46,6 +46,10 @@ class LoginAttempt extends DataObject 'Member' => Member::class, // only linked if the member actually exists ); + private static $indexes = array( + "EmailHashed" => true + ); + private static $table_name = "LoginAttempt"; /** @@ -86,7 +90,6 @@ class LoginAttempt extends DataObject public static function getByEmail($email) { return static::get()->filterAny(array( - 'Email' => $email, 'EmailHashed' => sha1($email), )); } From 1d5ecd342e417b4707a3bbc34e97949bffd14afb Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 21 Sep 2018 14:54:26 +1200 Subject: [PATCH 6/8] BUG Prevent error on valid response status codes --- src/Control/HTTPResponse.php | 19 +++++++++++ tests/php/Control/HTTPResponseTest.php | 46 +++++++++++++++----------- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/Control/HTTPResponse.php b/src/Control/HTTPResponse.php index 3ec7006c4..1fa010866 100644 --- a/src/Control/HTTPResponse.php +++ b/src/Control/HTTPResponse.php @@ -22,6 +22,8 @@ class HTTPResponse protected static $status_codes = [ 100 => 'Continue', 101 => 'Switching Protocols', + 102 => 'Processing', + 103 => 'Early Hints', 200 => 'OK', 201 => 'Created', 202 => 'Accepted', @@ -29,6 +31,9 @@ class HTTPResponse 204 => 'No Content', 205 => 'Reset Content', 206 => 'Partial Content', + 207 => 'Multi-Status', + 208 => 'Already Reported', + 226 => 'IM Used', 301 => 'Moved Permanently', 302 => 'Found', 303 => 'See Other', @@ -38,6 +43,7 @@ class HTTPResponse 308 => 'Permanent Redirect', 400 => 'Bad Request', 401 => 'Unauthorized', + 402 => 'Payment Required', 403 => 'Forbidden', 404 => 'Not Found', 405 => 'Method Not Allowed', @@ -53,14 +59,27 @@ class HTTPResponse 415 => 'Unsupported Media Type', 416 => 'Request Range Not Satisfiable', 417 => 'Expectation Failed', + 418 => 'I\'m a Teapot', + 421 => 'Misdirected Request', 422 => 'Unprocessable Entity', + 423 => 'Locked', + 424 => 'Failed Dependency', + 426 => 'Upgrade Required', + 428 => 'Precondition Required', 429 => 'Too Many Requests', + 431 => 'Request Header Fields Too Large', + 451 => 'Unavailable For Legal Reasons', 500 => 'Internal Server Error', 501 => 'Not Implemented', 502 => 'Bad Gateway', 503 => 'Service Unavailable', 504 => 'Gateway Timeout', 505 => 'HTTP Version Not Supported', + 506 => 'Variant Also Negotiates', + 507 => 'Unsufficient Storage', + 508 => 'Loop Detected', + 510 => 'Not Extended', + 511 => 'Network Authentication Required', ]; /** diff --git a/tests/php/Control/HTTPResponseTest.php b/tests/php/Control/HTTPResponseTest.php index 3457b6985..18469ec62 100644 --- a/tests/php/Control/HTTPResponseTest.php +++ b/tests/php/Control/HTTPResponseTest.php @@ -8,7 +8,6 @@ use SilverStripe\Control\HTTPResponse_Exception; class HTTPResponseTest extends SapphireTest { - public function testStatusDescriptionStripsNewlines() { $r = new HTTPResponse('my body', 200, "my description \nwith newlines \rand carriage returns"); @@ -23,29 +22,16 @@ class HTTPResponseTest extends SapphireTest $response = new HTTPResponse("Test", 200, 'OK'); // Confirm that the exception's statusCode and statusDescription take precedence - try { - throw new HTTPResponse_Exception($response, 404, 'not even found'); - } catch (HTTPResponse_Exception $e) { - $this->assertEquals(404, $e->getResponse()->getStatusCode()); - $this->assertEquals('not even found', $e->getResponse()->getStatusDescription()); - return; - } - // Fail if we get to here - $this->assertFalse(true, 'Something went wrong with our test exception'); + $e = new HTTPResponse_Exception($response, 404, 'not even found'); + $this->assertEquals(404, $e->getResponse()->getStatusCode()); + $this->assertEquals('not even found', $e->getResponse()->getStatusDescription()); } public function testExceptionContentPlainByDefault() { - // Confirm that the exception's statusCode and statusDescription take precedence - try { - throw new HTTPResponse_Exception("Some content that may be from a hacker", 404, 'not even found'); - } catch (HTTPResponse_Exception $e) { - $this->assertEquals("text/plain", $e->getResponse()->getHeader("Content-Type")); - return; - } - // Fail if we get to here - $this->assertFalse(true, 'Something went wrong with our test exception'); + $e = new HTTPResponse_Exception("Some content that may be from a hacker", 404, 'not even found'); + $this->assertEquals("text/plain", $e->getResponse()->getHeader("Content-Type")); } public function testRemoveHeader() @@ -58,4 +44,26 @@ class HTTPResponseTest extends SapphireTest $response->removeHeader('X-Animal'); $this->assertEmpty($response->getHeader('X-Animal')); } + + public function providerTestValidStatusCodes() + { + return [ + [200, 'OK'], + [226, 'IM Used'], + [426, 'Upgrade Required'], + [451, 'Unavailable For Legal Reasons'], + ]; + } + + /** + * @dataProvider providerTestValidStatusCodes + * @param int $code + * @param string $status + */ + public function testValidStatusCodes($code, $status) + { + $response = new HTTPResponse(); + $response->setStatusCode($code); + $this->assertEquals($status, $response->getStatusDescription()); + } } From f5929d87e01ad2ed96d8bcb3a9d14ea5239cbc5a Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Tue, 25 Sep 2018 17:34:03 +0200 Subject: [PATCH 7/8] DOCS Update do blocks for DataQuery::having() to reflect correct input types --- src/ORM/DataQuery.php | 2 +- src/ORM/Queries/SQLSelect.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index e403f90b3..dd21d2bc2 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -599,7 +599,7 @@ class DataQuery /** * Append a HAVING clause to this query. * - * @param string $having Escaped SQL statement + * @param mixed $having Predicate(s) to set, as escaped SQL statements or parameterised queries * @return $this */ public function having($having) diff --git a/src/ORM/Queries/SQLSelect.php b/src/ORM/Queries/SQLSelect.php index 971a12422..229ab56ce 100644 --- a/src/ORM/Queries/SQLSelect.php +++ b/src/ORM/Queries/SQLSelect.php @@ -481,7 +481,7 @@ class SQLSelect extends SQLConditionalExpression * * @see SQLSelect::addWhere() for syntax examples * - * @param mixed $having Predicate(s) to set, as escaped SQL statements or paramaterised queries + * @param mixed $having Predicate(s) to set, as escaped SQL statements or parameterised queries * @param mixed $having,... Unlimited additional predicates * @return $this Self reference */ @@ -497,7 +497,7 @@ class SQLSelect extends SQLConditionalExpression * * @see SQLSelect::addWhere() for syntax examples * - * @param mixed $having Predicate(s) to set, as escaped SQL statements or paramaterised queries + * @param mixed $having Predicate(s) to set, as escaped SQL statements or parameterised queries * @param mixed $having,... Unlimited additional predicates * @return $this Self reference */ From 231d6d9a9f388e10cf77149aec22e947db648644 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 28 Sep 2018 16:25:10 +0200 Subject: [PATCH 8/8] FIX New members now receive the configured default locale, not the current locale --- src/Security/Member.php | 12 ++++++------ tests/php/Security/MemberTest.php | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/Security/Member.php b/src/Security/Member.php index 164f995ee..356c2c1d8 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -268,7 +268,7 @@ class Member extends DataObject public function populateDefaults() { parent::populateDefaults(); - $this->Locale = i18n::get_locale(); + $this->Locale = i18n::config()->get('default_locale'); } public function requireDefaultRecords() @@ -929,7 +929,7 @@ class Member extends DataObject // save locale if (!$this->Locale) { - $this->Locale = i18n::get_locale(); + $this->Locale = i18n::config()->get('default_locale'); } parent::onBeforeWrite(); @@ -1228,7 +1228,7 @@ class Member extends DataObject /** * Return the date format based on the user's chosen locale, - * falling back to the default format defined by the {@link i18n.get_locale()} setting. + * falling back to the default format defined by the i18n::config()->get('default_locale') config setting. * * @return string ISO date format */ @@ -1247,7 +1247,7 @@ class Member extends DataObject } /** - * Get user locale + * Get user locale, falling back to the configured default locale */ public function getLocale() { @@ -1256,12 +1256,12 @@ class Member extends DataObject return $locale; } - return i18n::get_locale(); + return i18n::config()->get('default_locale'); } /** * Return the time format based on the user's chosen locale, - * falling back to the default format defined by the {@link i18n.get_locale()} setting. + * falling back to the default format defined by the i18n::config()->get('default_locale') config setting. * * @return string ISO date format */ diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 942b732b0..bdf664c1f 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -56,6 +56,8 @@ class MemberTest extends FunctionalTest Member::config()->set('unique_identifier_field', 'Email'); Member::set_password_validator(null); + + i18n::set_locale('en_US'); } public function testPasswordEncryptionUpdatedOnChangedPassword() @@ -1533,4 +1535,20 @@ class MemberTest extends FunctionalTest $this->assertInstanceOf(ValidationResult::class, $result); $this->assertFalse($result->isValid()); } + + public function testNewMembersReceiveTheDefaultLocale() + { + // Set a different current locale to the default + i18n::set_locale('de_DE'); + + $newMember = Member::create(); + $newMember->update([ + 'FirstName' => 'Leslie', + 'Surname' => 'Longly', + 'Email' => 'longly.leslie@example.com', + ]); + $newMember->write(); + + $this->assertSame('en_US', $newMember->Locale, 'New members receive the default locale'); + } }