GuySartorelli
commented
1 year ago My proposed rules of thumb for what "where practical" means:
- If making a component "work" with
ViewableDatameans 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
ViewableDataby providing setters for information that would otherwise be fetched fromDataObjects- e.g.GridFieldDetailFormallows setting a validator, fields, and even a custom item request class in lieu ofGridFieldDetailForm_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).
- 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
- Some components just rely on so much logic from
DataObjectthat it's not practical to find workarounds for it all. One example isGridFieldDetailForm_ItemRequest. In these cases, the PHPDoc for the class should clearly note that it is intended for use withDataObject.- 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 amethod_exists()or->hasMethod()orClassInfo::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.
- Without a new major release, we can't explicitly type-check for
- 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
Currently most
GridFieldComponentclasses implicitly require some methods which are implemented onDataObjectbut not onViewableData.It is not uncommon to want to use a
GridFieldwith other viewable data, such asArrayData(e.g. pulled from some API).It should be easy to use most
GridFieldcomponents with arbitraryViewableData. Any components for which this is not practical should be clearly documented as requiring specific methods (and in CMS 6 probably will be typehinted toDataObject).Acceptance Criteria
GridFieldComponentclasses work with arbitraryViewableDataViewableData(if any), this is clearly documented in a way that developers can identify from within their IDE.Notes
PRs