NEW: Validate the Title on Group is not empty

[textagroup]

You can currently create a new Group in the Security section of the CMS and leave the Title blank or fill it with only whitespace.

This pull requests adds some more validation on the Group DataObject as well as updating the GroupTest unit test.

[michalkleiner]

Makes sense from my perspective, but I may not have all the history why it wasn't done in past.
@emteknetnz what's your view on this enhancement?

[emteknetnz]

I don't know the history either. Seems sensible to me though. There's a semi-decent chance it'll break somewhere else if there's a $g = new Group(); $g->Code='my-code'; $g->write(); type of thing in a different module. Though given we just released 4.9.0 we've got plenty of time to fix any instances and they should show up in recipe-kitchen-sink unit testing fairly quickly.

My bigger concern though would be with projects, rather than modules, this they'd be might break on composer update

Possible change this from validation to onBeforeWrite instead? e.g. if (!trim($this->Title)) .. $this->Title = ucwords(str_replace('-', ' ', $this->Code));

[michalkleiner]

Yeah, I think I view it the same, more concerned about existing project code that works with the Code field and doesn't bother about the Title.

I guess we could keep the validation and check if (!trim(Title) || !trim(Code)) .. raise the error, and also update the Title from the Code field as you suggested either in onBeforeWrite if it kept the validation on both fields functional, or in onAfterWrite using an extra write (which won't matter much since groups are not versioned).

[textagroup]

I can make the changes in validation or onBeforeWrite, did come across another bug where you can duplicate the group codes by creating a Group with a previous title.

Might be an idea to consider addressing this in this pull request as well.

MariaDB [ss4vanilla]> SELECT ID, Title, Code FROM Group;
+----+-----------------+-----------------+
| ID | Title | Code |
+----+-----------------+-----------------+
| 1 | Content Authors | content-authors |
| 2 | Administrators | administrators |
| 3 | A Group | a-group |
| 4 | Content Authors | content-authors |
+----+-----------------+-----------------+
4 rows in set (0.001 sec)

[emteknetnz]

Yes duplicate Title and/or Code are both bad, so catching those in validation would be helpful

[kinglozzer]

If we’re worried about this breaking things like $g = new Group(); $g->Code='my-code'; $g->write();, why don’t we add this as form validation?

public function getCMSCompositeValidator(): CompositeValidator
{
    $validator = parent::getCMSCompositeValidator();

    $validator->addValidator(RequiredFields::create([
        'Title'
    ]));

    return $validator;
}

Someone could then still create a group with no title programatically if they like. Admittedly that won’t stop someone creating a group with a whitespace-only title, but if someone really wants to create a group with a name “ ” then why stop them? 😛

[michalkleiner]

Nice one @kinglozzer!

If there's no field for code to manage via the CMS UI, should it act like URLSegment and automatically check if it's unique and append the -2, -3 to keep it unique?

[textagroup]

Changes made to validate Title (CMS) and handle duplicate group codes.
Had to amend SapphireTest->createMemberWithPermission by adding an extra check to see if a group exists before trying to create it.
This was not a issue before due to duplicate code/titles being allowed

[michalkleiner]

Looks ok to me, thanks @textagroup.

@kinglozzer or @emteknetnz — can you give it another tick, too? Thanks!

[kinglozzer]

Looks like this has caused some build failures 😞 not sure why the PR build passed https://github.com/silverstripe/silverstripe-framework/runs/4087515550

[kinglozzer]

I guess it’s because we now have to make the SapphireTest change in two places as a result of the PHPUnit 9 changes?

[kinglozzer]

Fix: #10136