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

Make GridFieldComponents work with arbitrary ViewableData where practical #10771

Closed
3 tasks done
GuySartorelli opened this issue May 8, 2023 · 4 comments
Closed
3 tasks done

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented May 8, 2023

Currently most GridFieldComponent classes implicitly require some methods which are implemented on DataObject but not on ViewableData.

It is not uncommon to want to use a GridField with other viewable data, such as ArrayData (e.g. pulled from some API).

It should be easy to use most GridField components with arbitrary ViewableData. Any components for which this is not practical should be clearly documented as requiring specific methods (and in CMS 6 probably will be typehinted to DataObject).

Acceptance Criteria

  • Where practical, all GridFieldComponent classes work with arbitrary ViewableData
  • Tests are put in place to avoid regressions
  • For any component where it is not practical to use it with ViewableData (if any), this is clearly documented in a way that developers can identify from within their IDE.

Notes

PRs

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jun 11, 2023

My proposed rules of thumb for what "where practical" means:

  • If making a component "work" with ViewableData means just hiding the component or making the component do nothing, then that is not practical. Hiding a component or disabling a component is not making the component work and those should throw clear exceptions instead. e.g. GridFieldDetailForm_ItemRequest
  • Some components already allow using ViewableData by providing setters for information that would otherwise be fetched from DataObjects - e.g. GridFieldDetailForm allows setting a validator, fields, and even a custom item request class in lieu of GridFieldDetailForm_ItemRequest.
    • In these cases, if the setters have not been called and this would result in an exception being thrown, the exception should say something like "Implement $missingMethod on your data class or call $setterMethod on this component".
    • If a component already provides some of these setters but still has a barrier that could be bypassed with another setter, it is practical to add that setter.
    • If a component does not already provide these setters and adding them would be 'hacky' or complex, it may be more appropriate to just throw exceptions - this should be assessed on a case-by-case basis and the pull request should indicate what the reason was for not adding a setter (so the reviewer doesn't have to ask).
  • Some components just rely on so much logic from DataObject that it's not practical to find workarounds for it all. One example is GridFieldDetailForm_ItemRequest. In these cases, the PHPDoc for the class should clearly note that it is intended for use with DataObject.
    • Without a new major release, we can't explicitly type-check for DataObject, but we can check up-front for the methods or fields we're expecting and throw an exception if any are missing. That may seem cumbersome but it's probably easier than adding a method_exists() or ->hasMethod() or ClassInfo::hasMethod() check for every method we want to call on the record and provides a much clearer DX as to why this component isn't working for them.
  • Ultimately this issue is aimed at removing barriers that shouldn't be there, and adding clear DX about the barriers that should be there. At the end of the day "practical" is a judgment call as to whether the barrier seems reasonable or not to the person doing the work.

@maxime-rainville
Copy link
Contributor

The following modules have GridField components:

  • bringyourownideas/silverstripe-maintenance
  • colymba/gridfield-bulk-editing
  • silverstripe/blog
  • silverstripe/ckan-registry
  • silverstripe/gridfieldqueuedexport
  • silverstripe/lumberjack
  • silverstripe/securityreport
  • fluent
  • subsite
  • userforms
  • versioned-admin
  • versioned
  • symbiote/silverstripe-advanced-workflow
  • symbiote/silverstripe-gridfieldextensions

Maybe updating those is beyond the scope of this card.

@GuySartorelli
Copy link
Member Author

@maxime-rainville I already made a card for symbiote/silverstripe-gridfieldextensions since that has generic components (see link in issue description)

Advanced workflow is explicitly out of scope - as are a few others there which are only intended to be used in the way the module uses it, rather than in developer-defined gridfields. ckan-registry isn't even supported anymore....

Maybe open a card to cover any there which:

  • are actually still supported
  • are intended to be used in developer-defined gridfields beyond those already declared in the module

@maxime-rainville
Copy link
Contributor

It is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants