search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 820 forks source link

Make GridField components work with ViewableData where possible #11049

Closed GuySartorelli closed 6 months ago

GuySartorelli commented 8 months ago

Multiple commits

  1. Deprecates API that promotes intentional silent failures - in those cases the component should simply not be used with that GridField instance.
  2. Adds a new BasicSearchContext class which works on ArrayList, and makes some small changes necessary for GridFieldFilterHeader to work with arbitrary ViewableData.
  3. For the two components that cannot work with non-DataObjects, give an early and very clear exception if non-DataObject classes are being used.
  4. Make everything else work with arbitrary ViewableData, including adding more accurate typehints (to phpdocs) and clearer exceptions where appropriate.
  5. Add new behat function to allow deleting a gridfield row (I guess we just hadn't been testing that until now?)
  6. Unit tests

DO NOT SQUASH

Issue

GuySartorelli commented 7 months ago

mariadb failure is unrelated.

GuySartorelli commented 7 months ago

Framework test

if I search a generic search term, I get a Uncaught InvalidArgumentException: singleton() Called without a class and a deprecation warning.

Fixed

The Next/Back button doesn't seem to work on plain ArrayData.

I don't see this on my end - please provide more details.

If you give the GridField an empty list, everything breaks.

Please provide more details. What gridfield? Since this is related to the frameworktest I assume you mean a specific one? What error messaging do you see ("everything breaks" doesn't tell me much)? What steps can I take to replicate?

Maxime's plain ArrayData

GridFieldDetailForm doesn't work if my dataset doesn't have an ID. It doesn't throw an exception, it just doesn't populate any of the fields.

You mentioned this in https://github.com/silverstripe/silverstripe-framework/pull/11049#discussion_r1413421641 and I responded and then you never replied to that..... but I'm going to assume now that you want an explicit exception and just implement that and update the frameworktest example since that will result in the least amount of peer review ping pong.

Maxime's custom ArrayData subclass

I tried creating my own object that extends ArrayData so I could give it some permission. But I didn't give it any the things it might need to save, or update or delete the object.

  • The forms render fine initially. But I get TypeError because the thing is not a DataObjectInterface when I try to save or create new entries.
  • Deleting doesn't throw a type error, but it fails when it tries to call the non-existing delete method.
  • Seems like I should be blocked earlier and more explicitly.

I'll throw an explicit logic exception.

One thing I wasn't sure how to do is create something other than a Partial match filter for the search. Can you provide an example of how to do that?

For the general search, follow Specify a search filter for general search

For individual fields, you should be able to pass them into the setFilters() method on the BasicContext instance. SearchContext doesn't have a fallback for search filters, so that's the only way to do if for individual fields.

Other things

Doesn't GridFieldGroupDeleteAction require any update?

I didn't update this because it seems exceedingly clear it's only mean to work with Group records and isn't really for use outside the places that use it in core. We can't add a new exception in here because that will be a breaking change for anyone who's currently using it for weird purposes, or has added it to a gridfield they shouldn't have.

If you're okay with breaking those scenarios, and want me to add an exception anyway, say so and I'll add one.

More generally, I'm wondering how we can support devs who want to use this feature:

  • What kind of doc do we want to provide?
  • Is there a way we can provide a ArrayList specific GridFieldConfig or some other abstraction to make it easier to config columns and search field.

There is a documentation PR (oops, forgot to link that earlier) - any discussion about documentation should be in that PR.

I don't think an ArrayList specific config would make sense... but if you want to explore that further then I agree that's a separate card.

GuySartorelli commented 7 months ago

Framework test

If you pass an empty ArrayList to a gridfield, you get the following exception

Uncaught LogicException: GridField doesn't have a modelClassName, so it doesn't know the columns of this grid. Line 233 in /home/max/Projects/cms5x/vendor/silverstripe/framework/src/Forms/GridField/GridField.php

That's expected - if there's nothing in the arraylist, then neither the list nor the gridfield know what type of object they should be checking for fallbacks. You need to either call setDataClass() on the list or setModelClass() on the field.

Maxime's plain ArrayData

I looked at it with the explicit exception you've added. It's better. I get an exception thrown when I click the Edit button. I still get the full GridField rendered upfront. Maybe we want to fail hard right away even before clicking the Edit button. I can live with the way it is right now as well.

I don't think we can sensibly die early without making gridfield explicitly check for the presence of specific components, which doesn't seem like a great solution to me. I'd prefer to leave it at is for now.

It could be argued that it makes sense to display the data in the grid view, since there's no problems with doing so, and only throwing an exception when trying to do something there is a problem with.

Maxime's custom ArrayData subclass

I'm getting a "Uncaught LogicException: Company must implement SilverStripe\ORM\DataObjectInterface" exception now when I try to edit a specific row.

I can still get to the Creation form and I still get a Uncaught BadMethodCallException: Object->__call(): the method 'delete' does not exist when I click that delete button.

I bit like the previous point, it depends on much hand holding we want to do for the dev. Do we want to fail hard right away when rendering the initial gridfield? Or are we get waiting until the user interacts with the feature?

Same as above - I'd rather leave it as is for now.

GuySartorelli commented 6 months ago

I've made all the requested changes that I agree with and commented on ones I don't. I've looked at https://github.com/silverstripe/silverstripe-framework/pull/11049#pullrequestreview-1779363401 and it doesn't sound like there's any specific actions I need to take beyond what I've done.