Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Handle getting HasOneRelationFieldInterface passed as an array #11213

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Apr 29, 2024

Issue silverstripe/silverstripe-elemental#1155

This fixes bugs with autoscaffolded SearchableDropdownField for has_one SiteTree relations

Replication steps

  • Create a non-inline editable elemental content block
  • Add a has_one relation to a 'MySiteTree' => SiteTree::class
  • Add include the validation php block below
    public function getCMSCompositeValidator(): CompositeValidator
    {
        return CompositeValidator::create([RequiredFields::create(['MySiteTreeID'])]);
    }

You can leave the field blank and submit and the required fields validation won't trigger

@GuySartorelli
Copy link
Member

GuySartorelli commented Apr 30, 2024

I think the validator is working correctly - the developer should use the correct field name when setting their validator, i.e. it should be

RequiredFields::create(['MySiteTreeID'])

and not

RequiredFields::create(['MySiteTree'])

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll treat this PR as an enhancement instead of a bugfix, since I think the current behaviour isn't buggy, even though it's potentially confusing since some fields use the ID suffix and some don't.

To that end, please update the commit prefix to ENH.

tests/php/Forms/RequiredFieldsTest.php Outdated Show resolved Hide resolved
src/Forms/RequiredFields.php Outdated Show resolved Hide resolved
src/Forms/RequiredFields.php Outdated Show resolved Hide resolved
src/Forms/RequiredFields.php Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/5/required-has-one branch 2 times, most recently from 3dbea62 to 9ade813 Compare May 2, 2024 00:02
@emteknetnz emteknetnz changed the title FIX Try getting field with ID suffix for RequiredFields validation FIX Handle getting HasOneRelationFieldInterface passed as an array May 2, 2024
@emteknetnz emteknetnz force-pushed the pulls/5/required-has-one branch from 9ade813 to b8f0b8c Compare May 2, 2024 00:06
@GuySartorelli
Copy link
Member

GuySartorelli commented May 3, 2024

Does the bug you're fixing affect CMS 5.2?
If so, please retarget to the 5.2 branch since it's fixing a bug

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to reproduce the bug mentioned in the description both from the 5 and 5.2 branch - I can't reproduce it from either. Validation works fine without this PR in both scenarios.

Please try spinning up a fresh installation and testing your own reproduction steps.

@emteknetnz
Copy link
Member Author

Yeah looks like we don't need this one, I think there may have been some initial confusion with the "ID" suffix making me think things were broken when they were not

@emteknetnz emteknetnz closed this May 5, 2024
@GuySartorelli GuySartorelli deleted the pulls/5/required-has-one branch May 5, 2024 23:00
@emteknetnz emteknetnz restored the pulls/5/required-has-one branch May 6, 2024 05:21
@emteknetnz emteknetnz reopened this May 6, 2024
@emteknetnz
Copy link
Member Author

Turns out we do need this one, it's required for doing a page save of an inline-block where the has_one SiteTree::class in in RequiredFields. It's NOT required for saving non-inline block's.

It showed up as a behat failure in page-save-validation.feature on https://github.com/silverstripe/silverstripe-elemental/actions/runs/8961905824/job/24613378755?pr=1178#step:12:1052

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out there's a difference with some fields between "I have not selected anything" and "I have specifically clicked on the dropdown and selected the empty option".

Since form submission already correctly handles both cases, and it's been like that for donkey's years, I think it makes sense to handle this here as you've done.

@GuySartorelli GuySartorelli merged commit 0c8fcfb into silverstripe:5 May 7, 2024
30 checks passed
@GuySartorelli GuySartorelli deleted the pulls/5/required-has-one branch May 7, 2024 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants