search

silverstripe / cwp

Common Web Platform (CWP) features module. We strongly recommend using it for all new CWP projects. Future features will be delivered here.
https://www.cwp.govt.nz
BSD 3-Clause "New" or "Revised" License
10 stars 27 forks source link

FIX Set RelatedPages gridfield to be structural #339

Closed emteknetnz closed 1 month ago

emteknetnz commented 1 month ago

Issue https://github.com/silverstripe/.github/issues/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

michalkleiner commented 1 month ago

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?

emteknetnz commented 1 month ago

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

michalkleiner commented 1 month ago

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).

emteknetnz commented 1 month ago

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

michalkleiner commented 1 month ago

Fair enough on all the points, and thanks for explaining the context.