FIX Set RelatedPages gridfield to be structural #339
Conversation
emteknetnz
force-pushed
the
pulls/3.0/fix-behat
branch
from
661bfe9 to
1289817
Compare
|
This feels like just hiding a problem somewhere else, haven't seen this construction done anywhere else yet. What is special about the gridfield compared to other relation gridfields that needs to be addressed this way? |
|
Gridfield's don't work in a react context because there is no corresponding GridField react component. Setting the $schemaDataType to structural simply means it won't attempt to find a javascript react component, thus meaning the rest of the frontend will render. silverstripe/versioned-admin uses react. This means that you cannot actually view the page history details of a subclass of BasePage. I'm not quite sure how this has gone unnoticed for so long, perhaps very few people use BasePage? From memory GridField doesn't have a structural schemaDataType out of the box because we devlopers to get a hard failure as feedback rather than a graceful silent failure and then developer wonder what happened to their gridfield The real solution here is to actually create a react component for GridField, though that's way out of scope for now |
|
Could an alternative be to have the structural type added to gridfield by default and controlled e.g. by environment type, so that in dev it always fails and in test/live it's silent? Do the behat tests run in live mode? It still feels like a workaround though it might be ok as the cwp recipe may not be used as much or the basepage is not used or overridden significantly (the last being what we do). |
|
Behat tests can run in either dev or test. However having things behave differently in different environment types is generally bad, it makes testing unreliable so it's not something I'd want to do It's worth nothing that this fix only has an effect in a react based context, in a non-react context, such as the regular page edit form, there is no difference Versioned-admin is in the only react context that I can think of where you would be editing a page object (in this instance, it's in readonly mode), so I think this is a pretty safe change to make. Even then, all the change is doing is changing a hard exception to a graceful soft exception. As mentioned above the only reason we use the hard exception is to provide developers with better feedback. Given where CWP is at this point (unsupported except for high level security issues) I don't think we need to be too concerned with the developer-experience |
|
Fair enough on all the points, and thanks for explaining the context. |
Issue silverstripe/.github#272
Fixes versioned-admin behat when running in kitchen-sink
Replicatable locally by try to view an individual entry in page history of a page that extends the cwp BasePage