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

BUGFIX: Fixed issue #213 causing crash when accessing a translation with a checkbox #215

Merged
merged 1 commit into from
Jan 13, 2016
Merged

Conversation

UndefinedOffset
Copy link
Contributor

Currently accessing a translation who's form contains a checkbox results in a crash (issue #213) caused by the first argument being passed is a CheckboxField_Readonly instance which is not a decedent of the CheckboxField class. This pull changes the first argument to expect an instance of CheckboxField_Readonly which allows the form to correctly display.

Generated error prior to this change

Argument 1 passed to Translatable_Transformation::transformCheckboxField() must be an instance of CheckboxField, instance of CheckboxField_Readonly given

@@ -1897,7 +1897,7 @@ public function transformFormField(FormField $field) {
* @param FormField $originalField The original editable field containing the translated value
* @return CheckboxField The field with a modified label
*/
protected function transformCheckboxField(CheckboxField $nonEditableField, CheckboxField $originalField) {
protected function transformCheckboxField(CheckboxField_Readonly $nonEditableField, CheckboxField $originalField) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we have the inverse errorwhen a non-readonly CheckboxField is passed in. If using a strict typehint it should match the phpdoc (or alter the phpdoc).

I suggest to just make it FormField.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this parameter even used? It doesn't seem to be even referenced inside the method body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya i don't think it is, probably only there for compatibility.

@chillu
Copy link
Member

chillu commented Jan 13, 2016

Both the PHPDoc and the argument name indicate a readonly field. Looking at transformFormField(), it calls FormField-> performReadOnlyTransformation() - which in the case of CheckboxField always returns a CheckboxField_Readonly. So I think it's a pretty safe change. This was introduced two years ago by 6460c08, wonder how that ever worked?

chillu added a commit that referenced this pull request Jan 13, 2016
BUGFIX: Fixed issue #213 causing crash when accessing a translation with a checkbox
@chillu chillu merged commit f702487 into silverstripe:2.1 Jan 13, 2016
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.

3 participants