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

Change GridfieldDetailForm to be opt-in #35

Conversation

tractorcow
Copy link
Contributor

Fixes #21

@tractorcow tractorcow force-pushed the pulls/1.0/opt-in-gridfield-versioning branch from 182a0bf to 4e04e4e Compare August 22, 2017 04:40
tractorcow pushed a commit to open-sausages/silverstripe-framework that referenced this pull request Aug 22, 2017
@tractorcow tractorcow force-pushed the pulls/1.0/opt-in-gridfield-versioning branch from 4e04e4e to ecdba87 Compare August 22, 2017 04:49
@tractorcow
Copy link
Contributor Author

See silverstripe/silverstripe-framework#7295 for docs.

Copy link

@unclecheese unclecheese left a comment

Choose a reason for hiding this comment

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

Code looks good, but the approach gives me pause. Something is really bothering me about the versioned_gridfield_extensions DataObject setting.

We all know DataObject is pretty monolithic and hosts a wide array of concerns, but it seems to me this is the first member of its API to speak directly to another class. Things like summary_fields and searchable_fields primarily used for GridField, but there's a very reasonable case for using them elsewhere. So I guess without getting into a huge philosophical discussion on one static, I'm wondering two things:

  1. Is there better semantics we can assign this, so that it could conceivably be consumed by other APIs?

  2. Is this the best place to store this setting? Just thinking out loud here, but what if we made it GridFieldItemRequest's concern, and registered versioned DataObjects with it, rather than the other way around?

@tractorcow
Copy link
Contributor Author

We all know DataObject is pretty monolithic and hosts a wide array of concerns, but it seems to me this is the first member of its API to speak directly to another class. Things like summary_fields and searchable_fields primarily used for GridField, but there's a very reasonable case for using them elsewhere. So I guess without getting into a huge philosophical discussion on one static, I'm wondering two things:

It's not an API of DataObject; I didn't add any private static to dataobject.

Is there better semantics we can assign this, so that it could conceivably be consumed by other APIs?

It doesn't need to be consumed by other APIs. This is a setting for a specific extension.

Is this the best place to store this setting? Just thinking out loud here, but what if we made it GridFieldItemRequest's concern, and registered versioned DataObjects with it, rather than the other way around?

It would be a whole lot more complicated to do it that way so I chose the simplest way.

tractorcow pushed a commit to open-sausages/silverstripe-framework that referenced this pull request Aug 23, 2017
@unclecheese unclecheese merged commit 8977674 into silverstripe:1 Aug 23, 2017
@unclecheese unclecheese deleted the pulls/1.0/opt-in-gridfield-versioning branch August 23, 2017 22:55
tractorcow pushed a commit to silverstripe/silverstripe-framework that referenced this pull request Aug 23, 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.

None yet

2 participants