From 48ecb8d8f4d3c127cdbf1fcbc9f9a8d35b660a6a Mon Sep 17 00:00:00 2001 From: Jake Ovenden Date: Wed, 24 Feb 2016 15:13:00 +1300 Subject: [PATCH 1/4] added if statement to catch NULL validators --- admin/code/LeftAndMain.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php index 46fb86f5e..97f675452 100644 --- a/admin/code/LeftAndMain.php +++ b/admin/code/LeftAndMain.php @@ -1301,7 +1301,9 @@ class LeftAndMain extends Controller implements PermissionProvider { // The clientside (mainly LeftAndMain*.js) rely on ajax responses // which can be evaluated as javascript, hence we need // to override any global changes to the validation handler. - $form->setValidator($validator); + if($validator != NULL){ + $form->setValidator($validator); + } } else { $form->unsetValidator(); } From f691a5da328d1343e4e4c1688d2c0ff230262795 Mon Sep 17 00:00:00 2001 From: Roman Schmid Date: Wed, 24 Feb 2016 13:23:38 +0100 Subject: [PATCH 2/4] Improve `Member_Validator` to: - properly check for existing members. - allow extensions. - remove old code and replace with new syntax and add config API. Fix issue in Group code where Member_Validator was instantiated via "new" which didn't allow injector overrides. Added unit-tests. Establish a link between the member and the validator for said member. --- security/Group.php | 2 +- security/Member.php | 132 +++++++++++++++++++--------- tests/security/MemberTest.php | 158 +++++++++++++++++++++++++++++++++- 3 files changed, 248 insertions(+), 44 deletions(-) diff --git a/security/Group.php b/security/Group.php index c45823d51..f9c661efd 100755 --- a/security/Group.php +++ b/security/Group.php @@ -115,7 +115,7 @@ class Group extends DataObject { $config->getComponentByType('GridFieldAddExistingAutocompleter') ->setResultsFormat('$Title ($Email)')->setSearchFields(array('FirstName', 'Surname', 'Email')); $config->getComponentByType('GridFieldDetailForm') - ->setValidator(new Member_Validator()) + ->setValidator(Member_Validator::create()) ->setItemEditFormCallback(function($form, $component) use($group) { $record = $form->getRecord(); $groupsField = $form->Fields()->dataFieldByName('DirectGroups'); diff --git a/security/Member.php b/security/Member.php index e0195153a..ab8f4b3e7 100644 --- a/security/Member.php +++ b/security/Member.php @@ -766,6 +766,7 @@ class Member extends DataObject implements TemplateGlobalProvider { */ public function getValidator() { $validator = Injector::inst()->create('Member_Validator'); + $validator->setForMember($this); $this->extend('updateValidator', $validator); return $validator; @@ -1801,21 +1802,39 @@ class Member_ForgotPasswordEmail extends Email { * Member Validator * * Custom validation for the Member object can be achieved either through an - * {@link DataExtension} on the Member object or, by specifying a subclass of + * {@link DataExtension} on the Member_Validator object or, by specifying a subclass of * {@link Member_Validator} through the {@link Injector} API. - * + * The Validator can also be modified by adding an Extension to Member and implement the + * updateValidator hook. * {@see Member::getValidator()} * + * Additional required fields can also be set via config API, eg. + * + * Member_Validator: + * customRequired: + * - Surname + * + * * @package framework * @subpackage security */ -class Member_Validator extends RequiredFields { - +class Member_Validator extends RequiredFields +{ + /** + * Fields that are required by this validator + * @config + * @var array + */ protected $customRequired = array( 'FirstName', 'Email' ); + /** + * Determine what member this validator is meant for + * @var Member + */ + protected $forMember = null; /** * Constructor @@ -1829,7 +1848,33 @@ class Member_Validator extends RequiredFields { $required = array_merge($required, $this->customRequired); - parent::__construct($required); + // check for config API values and merge them in + $config = $this->config()->customRequired; + if(is_array($config)){ + $required = array_merge($required, $config); + } + + parent::__construct(array_unique($required)); + } + + /** + * Get the member this validator applies to. + * @return Member + */ + public function getForMember() + { + return $this->forMember; + } + + /** + * Set the Member this validator applies to. + * @param Member $value + * @return $this + */ + public function setForMember(Member $value) + { + $this->forMember = $value; + return $this; } /** @@ -1842,47 +1887,54 @@ class Member_Validator extends RequiredFields { * @return bool Returns TRUE if the submitted data is valid, otherwise * FALSE. */ - public function php($data) { + public function php($data) + { $valid = parent::php($data); - $identifierField = Member::config()->unique_identifier_field; - $member = DataObject::get_one('Member', array( - "\"$identifierField\"" => $data[$identifierField] - )); + $identifierField = (string)Member::config()->unique_identifier_field; - // if we are in a complex table field popup, use ctf[childID], else use ID - if(isset($_REQUEST['ctf']['childID'])) { - $id = $_REQUEST['ctf']['childID']; - } elseif(isset($_REQUEST['ID'])) { - $id = $_REQUEST['ID']; - } else { - $id = null; - } - - if($id && is_object($member) && $member->ID != $id) { - $uniqueField = $this->form->Fields()->dataFieldByName($identifierField); - $this->validationError( - $uniqueField->id(), - _t( - 'Member.VALIDATIONMEMBEREXISTS', - 'A member already exists with the same %s', - array('identifier' => strtolower($identifierField)) - ), - 'required' - ); - $valid = false; - } - - // Execute the validators on the extensions - if($this->extension_instances) { - foreach($this->extension_instances as $extension) { - if(method_exists($extension, 'hasMethod') && $extension->hasMethod('updatePHP')) { - $valid &= $extension->updatePHP($data, $this->form); + // Only validate identifier field if it's actually set. This could be the case if + // somebody removes `Email` from the list of required fields. + if(isset($data[$identifierField])){ + $id = isset($data['ID']) ? (int)$data['ID'] : 0; + if(!$id && ($ctrl = $this->form->getController())){ + // get the record when within GridField (Member editing page in CMS) + if($ctrl instanceof GridFieldDetailForm_ItemRequest && $record = $ctrl->getRecord()){ + $id = $record->ID; } } + + // If there's no ID passed via controller or form-data, use the assigned member (if available) + if(!$id && ($member = $this->getForMember())){ + $id = $member->exists() ? $member->ID : 0; + } + + // set the found ID to the data array, so that extensions can also use it + $data['ID'] = $id; + + $members = Member::get()->filter($identifierField, $data[$identifierField]); + if($id) { + $members = $members->exclude('ID', $id); + } + + if($members->count() > 0) { + $this->validationError( + $identifierField, + _t( + 'Member.VALIDATIONMEMBEREXISTS', + 'A member already exists with the same {identifier}', + array('identifier' => Member::singleton()->fieldLabel($identifierField)) + ), + 'required' + ); + $valid = false; + } } - return $valid; - } + // Execute the validators on the extensions + $results = $this->extend('updatePHP', $data, $this->form); + $results[] = $valid; + return min($results); + } } diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index 4a8e66b4c..474dfd89a 100644 --- a/tests/security/MemberTest.php +++ b/tests/security/MemberTest.php @@ -678,7 +678,7 @@ class MemberTest extends FunctionalTest { $newAdminGroup = new Group(array('Title' => 'newadmin')); $newAdminGroup->write(); Permission::grant($newAdminGroup->ID, 'ADMIN'); - + // Test staff member can't be added to admin groups $this->assertFalse($staffMember->inGroup($newAdminGroup)); $staffMember->Groups()->setByIDList(array($newAdminGroup->ID)); @@ -842,7 +842,135 @@ class MemberTest extends FunctionalTest { } } - public function testCustomMemberValidator() { + public function testMemberValidator() + { + // clear custom requirements for this test + Config::inst()->update('Member_Validator', 'customRequired', null); + $memberA = $this->objFromFixture('Member', 'admin'); + $memberB = $this->objFromFixture('Member', 'test'); + + // create a blank form + $form = new MemberTest_ValidatorForm(); + + $validator = new Member_Validator(); + $validator->setForm($form); + + // Simulate creation of a new member via form, but use an existing member identifier + $fail = $validator->php(array( + 'FirstName' => 'Test', + 'Email' => $memberA->Email + )); + + $this->assertFalse( + $fail, + 'Member_Validator must fail when trying to create new Member with existing Email.' + ); + + // populate the form with values from another member + $form->loadDataFrom($memberB); + + // Assign the validator to an existing member + // (this is basically the same as passing the member ID with the form data) + $validator->setForMember($memberB); + + // Simulate update of a member via form and use an existing member Email + $fail = $validator->php(array( + 'FirstName' => 'Test', + 'Email' => $memberA->Email + )); + + // Simulate update to a new Email address + $pass1 = $validator->php(array( + 'FirstName' => 'Test', + 'Email' => 'membervalidatortest@testing.com' + )); + + // Pass in the same Email address that the member already has. Ensure that case is valid + $pass2 = $validator->php(array( + 'FirstName' => 'Test', + 'Surname' => 'User', + 'Email' => $memberB->Email + )); + + $this->assertFalse( + $fail, + 'Member_Validator must fail when trying to update existing member with existing Email.' + ); + + $this->assertTrue( + $pass1, + 'Member_Validator must pass when Email is updated to a value that\'s not in use.' + ); + + $this->assertTrue( + $pass2, + 'Member_Validator must pass when Member updates his own Email to the already existing value.' + ); + } + + public function testMemberValidatorWithExtensions() + { + // clear custom requirements for this test + Config::inst()->update('Member_Validator', 'customRequired', null); + + // create a blank form + $form = new MemberTest_ValidatorForm(); + + // Test extensions + Member_Validator::add_extension('MemberTest_MemberValidator_SurnameMustMatchFirstNameExtension'); + $validator = new Member_Validator(); + $validator->setForm($form); + + // This test should fail, since the extension enforces FirstName == Surname + $fail = $validator->php(array( + 'FirstName' => 'Test', + 'Surname' => 'User', + 'Email' => 'test-member-validator-extension@testing.com' + )); + + $pass = $validator->php(array( + 'FirstName' => 'Test', + 'Surname' => 'Test', + 'Email' => 'test-member-validator-extension@testing.com' + )); + + $this->assertFalse( + $fail, + 'Member_Validator must fail because of added extension.' + ); + + $this->assertTrue( + $pass, + 'Member_Validator must succeed, since it meets all requirements.' + ); + + // Add another extension that always fails. This ensures that all extensions are considered in the validation + Member_Validator::add_extension('MemberTest_MemberValidator_AlwaysFailsExtension'); + $validator = new Member_Validator(); + $validator->setForm($form); + + // Even though the data is valid, This test should still fail, since one extension always returns false + $fail = $validator->php(array( + 'FirstName' => 'Test', + 'Surname' => 'Test', + 'Email' => 'test-member-validator-extension@testing.com' + )); + + $this->assertFalse( + $fail, + 'Member_Validator must fail because of added extensions.' + ); + + // Remove added extensions + Member_Validator::remove_extension('MemberTest_MemberValidator_AlwaysFailsExtension'); + Member_Validator::remove_extension('MemberTest_MemberValidator_SurnameMustMatchFirstNameExtension'); + } + + public function testCustomMemberValidator() + { + // clear custom requirements for this test + Config::inst()->update('Member_Validator', 'customRequired', null); + $member = $this->objFromFixture('Member', 'admin'); $form = new MemberTest_ValidatorForm(); @@ -861,7 +989,7 @@ class MemberTest extends FunctionalTest { 'Surname' => '' )); - $this->assertTrue($pass, 'Validator requires on FirstName and Email'); + $this->assertTrue($pass, 'Validator requires a FirstName and Email'); $this->assertFalse($fail, 'Missing FirstName'); $ext = new MemberTest_ValidatorExtension(); @@ -919,6 +1047,30 @@ class MemberTest_ValidatorExtension extends DataExtension implements TestOnly { } } +/** + * Extension that adds additional validation criteria + * @package framework + * @subpackage tests + */ +class MemberTest_MemberValidator_SurnameMustMatchFirstNameExtension extends DataExtension implements TestOnly +{ + public function updatePHP($data, $form) { + return $data['FirstName'] == $data['Surname']; + } +} + +/** + * Extension that adds additional validation criteria + * @package framework + * @subpackage tests + */ +class MemberTest_MemberValidator_AlwaysFailsExtension extends DataExtension implements TestOnly +{ + public function updatePHP($data, $form) { + return false; + } +} + /** * @package framework * @subpackage tests From 5810ecf7b96f8c2402e54430c8e8917cf7f5d647 Mon Sep 17 00:00:00 2001 From: Matthew Hailwood Date: Sun, 28 Feb 2016 20:56:10 +1300 Subject: [PATCH 3/4] Populate foreign key before getting CMS fields In it's current state you need to revert to something like `Session::get('CMSMain.currentPage')` to get the foreign key of what the item you are creating relates to (e.g. a Book => has_many Author) - if you create a new Author you may need to reference the owning Book in the `getCMSFields` function. This is just a small quality of life buff that populates that foreign key before calling `getCMSFields()` rather than after. This should not break backwards compatibility in any way and isn't exactly a new feature so could be considered a bug fix. --- forms/gridfield/GridFieldDetailForm.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/forms/gridfield/GridFieldDetailForm.php b/forms/gridfield/GridFieldDetailForm.php index c6c320e9d..d70e16a8c 100644 --- a/forms/gridfield/GridFieldDetailForm.php +++ b/forms/gridfield/GridFieldDetailForm.php @@ -385,20 +385,22 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { $actions->push(new LiteralField('cancelbutton', $text)); } } + + // If we are creating a new record in a has-many list, then + // pre-populate the record's foreign key. + if($list instanceof HasManyList && !$this->record->isInDB()) { + $key = $list->getForeignKey(); + $id = $list->getForeignID(); + $this->record->$key = $id; + } $fields = $this->component->getFields(); if(!$fields) $fields = $this->record->getCMSFields(); // If we are creating a new record in a has-many list, then - // pre-populate the record's foreign key. Also disable the form field as - // it has no effect. + // Disable the form field as it has no effect. if($list instanceof HasManyList) { $key = $list->getForeignKey(); - $id = $list->getForeignID(); - - if(!$this->record->isInDB()) { - $this->record->$key = $id; - } if($field = $fields->dataFieldByName($key)) { $fields->makeFieldReadonly($field); From dcb83f0c6ed37fa5a9497339d34fabaf94f27b37 Mon Sep 17 00:00:00 2001 From: Petar Simic Date: Mon, 29 Feb 2016 18:28:07 +0100 Subject: [PATCH 4/4] Update it.yml - parse error on line 321 (323) Error while building Italian version of framework. There is a wrong formatting in line 321 (ex. 323). --- lang/it.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lang/it.yml b/lang/it.yml index e10283e12..d4660af67 100644 --- a/lang/it.yml +++ b/lang/it.yml @@ -318,9 +318,7 @@ it: PAGE: Pagina SUBJECT: 'Oggetto email' URL: URL - URLDESCRIPTION: 'Inserisci video e immagini dal Web nella tua pagina semplicemente inserendo l''URL del file. - - Si sicuro di avere i diritti o i permessi prima di condividere media direttamente dal Web.

NB : i file non sono aggiunti allo storage file del CMS, ma incorpora il file dalla sua location principale, se per un qualsiasi motivo il file non e'' più raggiungibile nella sua location principale, non sara'' più visibile su questa pagina.' + URLDESCRIPTION: 'Inserisci video e immagini dal Web nella tua pagina semplicemente inserendo l''URL del file.
Si sicuro di avere i diritti o i permessi prima di condividere media direttamente dal Web.

NB : i file non sono aggiunti allo storage file del CMS, ma incorpora il file dalla sua location principale, se per un qualsiasi motivo il file non e'' più raggiungibile nella sua location principale, non sara'' più visibile su questa pagina.' URLNOTANOEMBEDRESOURCE: 'L''URL ''{url}'' non può essere convertito in una risorsa media.' UpdateMEDIA: 'Aggiorna Media' Image: