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: Fixes #8459 Missing conditionals for optional constructor args #8684

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

phptek
Copy link

@phptek phptek commented Dec 18, 2018

Fixes #8459

  • Missing conditionals for optional constructor args
  • Missing calls to FieldList::setForm()
  • Missing guards around naive calls to Form::Fields()->foo()

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I think it would be better if the null checks were moved to the setFields() and setActions(). The constructor could just call the setters. That way we avoid duplicating logic between the setters and the constructor.

@kinglozzer
Copy link
Member

Would it be easier, rather than scatter around extra null conditional checks, to just add something to the constructor like:

$fields = $fields ? $fields : FieldList::create();
$actions = $actions ? $actions : FieldList::create();

@maxime-rainville
Copy link
Contributor

@kinglozzer's suggestion makes more sense.

@phptek
Copy link
Author

phptek commented Dec 18, 2018

Fixed. Force-pushed. Please excuse the unrelated changes, I need newlines to be there and typos not to be.

- Missing conditionals for optional constructor args
- Missing calls to FieldList::setForm()
- Missing guards around naive calls to Form::Fields()->foo()
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Looking good. Tested locally.

@maxime-rainville maxime-rainville merged commit b22af53 into silverstripe:4 Dec 19, 2018
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.

None yet

3 participants