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 Reintroduce lost changes to CompositeField_holder.ss after namespacing #6511

Merged

Conversation

robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Jan 16, 2017

I'm not sure if this was deliberate or not, but a large amount of the CompositeField_holder.ss template was lost during namespacing (commit #8dd644d).

See 3.5 version compared with current master.

In its current state, frontend composite fields do not render correctly. No labels are displayed as the individual field's holder templates are not used.

If I've mistaken this, and this was a deliberate change, please feel free to close this pull request!

@helpfulrobot
Copy link

@robbieaverill, thanks for your PR! By analyzing the blame information on this pull request, I identified @wilr and @sminnee to be potential reviewers

@robbieaverill
Copy link
Contributor Author

cc @tractorcow

@dhensby
Copy link
Contributor

dhensby commented Jan 16, 2017

Test failures related...

@robbieaverill
Copy link
Contributor Author

Ok, those errors are happening because SelectionGroup extends CompositeField so will be inheriting the changed template.

@tractorcow are you happy that me "fixing" this regression is correct in this case? If so, I'll add the old version of this file as a template for SelectionGroup_holder to prevent this change from affecting it as well.

@tractorcow
Copy link
Contributor

tractorcow commented Jan 16, 2017

These changes weren't lost after namespacing, they were intentionally made during the bootrapping process.

Each field is built with a fixed <Class>_holder.ss, and a <Class>.ss. Typically the holder includes the inner field, but in some cases this was not done consistently. As of 4.0, it is.

If you take a look at ComposetField.ss, you'll see this contains the missing code you were looking for, and this is what would be included with $Field in CompositeField_holder.ss

If fields are not displayed correctly on the front end, then that is a real bug. Can you please show me the code you used to scaffold such a broken form @robbieaverill ?

@tractorcow
Copy link
Contributor

Ok, I've tested the code locally on both 3.5 and 4.0. There is a bug, but it can be fixed by changing $Field to $FieldHolder in CompositeField.ss, rather than reverting the entire CompositeField_holder.ss template.

The bootstrap version of this field / holder already does this properly. We just need to update the default template.

Copy link
Contributor

@tractorcow tractorcow left a comment

Choose a reason for hiding this comment

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

As above.

@robbieaverill robbieaverill force-pushed the bugfix/fix-compositefield-holder branch from 61daff8 to 7ad0278 Compare January 16, 2017 21:19
@tractorcow tractorcow merged commit 0013587 into silverstripe:master Jan 17, 2017
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.

4 participants