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 GridField components work with ViewableData where possible #11049

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Nov 12, 2023

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 GuySartorelli marked this pull request as draft November 12, 2023 23:14
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from b2a80b8 to 8c63c0c Compare November 12, 2023 23:15
@GuySartorelli GuySartorelli changed the title DEP Deprecate configurable silent failures in GridField components Make GridField components with with ViewableData where possible Nov 12, 2023
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from 8c63c0c to a29b079 Compare November 13, 2023 02:18
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from 0149f7b to 28ad2bb Compare November 20, 2023 03:49
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from 28ad2bb to ed347ad Compare November 20, 2023 03:51
@GuySartorelli GuySartorelli changed the title Make GridField components with with ViewableData where possible Make GridField components work with ViewableData where possible Nov 20, 2023
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from ed347ad to d7fee75 Compare November 21, 2023 22:52
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from d7fee75 to 58de539 Compare November 22, 2023 03:33
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch 4 times, most recently from a9afa5b to a3976f1 Compare November 26, 2023 23:48
@GuySartorelli
Copy link
Member Author

mariadb failure is unrelated.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a first pass on the code review.

Haven't had a go at running the thing locally yet.

src/Forms/Form.php Show resolved Hide resolved
src/Forms/Form.php Show resolved Hide resolved
src/Forms/GridField/GridField.php Show resolved Hide resolved
src/ORM/Search/SearchContext.php Outdated Show resolved Hide resolved
tests/php/Forms/GridField/GridFieldDeleteActionTest.php Outdated Show resolved Hide resolved
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Farmework test

I tested this with the frameworktest PR:

  • Searching for an explicit filter seemed to work fine. However, if I search a generic search term, I get a Uncaught InvalidArgumentException: singleton() Called without a class and a deprecation warning.
[Emergency] Uncaught LogicException: Cannot dynamically instantiate SearchContext. Pass the SearchContext to setSearchContext() or implement a getDefaultSearchContext() method on SilverStripe\View\ArrayData
GET /admin/pages/edit/show/2

Line 262 in /home/max/Projects/cms5x/vendor/silverstripe/framework/src/Forms/GridField/GridFieldFilterHeader.php
  • The Next/Back button doesn't seem to work on plain ArrayData.
  • If you give the GridField an empty list, everything breaks.

Maxime's plain ArrayData

I tried creating my own GridField with plain ArrayData and add it to a Page form:

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

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.

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?

Other things

Doesn't GridFieldGroupDeleteAction require any update?

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.

Maybe this should be a follow up card.

I've been using this bits of code for my tests: https://gist.github.com/maxime-rainville/eefa209bf727a0526304a2d5bacc72ca

These components simply cannot work with non-DataObjects. They have
explicit DataObject queries, and allowing arbitrary callbacks for these
components would be a case of diminishing returns.
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch 2 times, most recently from aa5a151 to 3db4e17 Compare December 10, 2023 23:57
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Dec 11, 2023

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 #11049 (comment) 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.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Framework test

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

One of the frameworktest gridfield would not seem to realise more data was available when viewing the edit form. But I can't replicate it after pulling the latest changes.

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

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
<?php

use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\GridField\GridField;
use SilverStripe\Forms\GridField\GridFieldAddNewButton;
use SilverStripe\Forms\GridField\GridFieldConfig_RecordViewer;
use SilverStripe\Forms\GridField\GridFieldDataColumns;
use SilverStripe\Forms\GridField\GridFieldDeleteAction;
use SilverStripe\Forms\TextField;
use SilverStripe\ORM\ArrayList;
use SilverStripe\View\ArrayData;
use SilverStripe\ORM\Search\BasicSearchContext;

class TestPage extends Page
{
    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->addFieldsToTab('Root.ViewableData', $this->GridField());
        return $fields;
    }

    public function GridField()
    {
        $dataset = ArrayList::create([
            // ArrayData::create([
            //     'ID' => 1,
            //     'Company' => 'Apple',
            //     'Industry' => 'Technology',
            //     'Valuation' => '$1.3T',
            //     'CEO' => 'Tim Cook',
            // ]),
        ]);
        $config = GridFieldConfig_RecordViewer::create();
        $config
            ->getComponentByType(GridFieldDataColumns::class)
            ->setDisplayFields([
                'Company' => 'Company',
                'Industry' => 'Industry',
                'Valuation' => 'Valuation',
                'CEO' => 'CEO',
            ]);

        $config->removeComponentsByType(GridFieldAddNewButton::class);
        $config->removeComponentsByType(GridFieldDeleteAction::class);
        $searchContext = BasicSearchContext::create(null);
        $searchContext->setFields(FieldList::create(
            TextField::create('ID'),
            TextField::create('Company'),
            TextField::create('Industry'),
            TextField::create('Valuation'),
            TextField::create('CEO'),
        ));


        return GridField::create('MyArrayList', 'My Array List', $dataset, $config);
    }
}

Maxime's plain ArrayData

I didn't notice the connection between my two requests.

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.

image

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.

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.

A 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?

src/ORM/Search/BasicSearchContext.php Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

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.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting to see a bottom to this barrel. My high level concerns have all been answered to my satisfaction.

Some of the minor outstanding points:

  • We could update some of the existing unit test to explicitly test that they work with ArrayLists.
    • Not really a hill I want to die on thought.
    • Some of the pre-existing unit coverage seem kind of light. Maybe we want to pick up a a follow up card to complete those unit test ... but only if you keen to do so. Happy to let it go otherwise.
  • I have some outstanding comments that on some of the DataProviders that I thought were a bit too convoluted. Again not overly fuss about those.

I do have a bit of a meta-concern: this seems like a big intricate code change. I would be keen to get other dev to try to use this before we tag a 5.2 beta. I'll have a look through the other PRs, especially the doc one. Maybe we bring a pre-beta validation card in early in the new year to de-risk this change.

src/Forms/GridField/GridField.php Show resolved Hide resolved
tests/php/Forms/GridField/GridFieldDataColumnsTest.php Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there be updates to tests/php/Forms/GridField/GridFieldDetailFormTest.php to account for the changes in this file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add one to validate that a missing ID throws an exception. If you had other tests you want me to add please specify what they should be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up adding full coverage for getRecordFromRequest since it had no direct test coverage

src/Forms/GridField/GridFieldEditButton.php Show resolved Hide resolved
src/Forms/GridField/GridFieldEditButton.php Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this one already had some ArrayList test. The can view logic also assumes that if there's no CanView method, the data can be outputted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if you're asking for some change here, or just making a note? Gonna assume the latter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be keen to have at least a minimalist test with an ArrayList for this one. Test coverage for this class seems quite light. I would be happy to split it off as a separate card as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer that to be a separate card. Behat testing covers the new use case so I think it's okay for unit testing to be a little light here.

src/Forms/GridField/GridFieldLevelup.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Dec 17, 2023

I've made all the requested changes that I agree with and commented on ones I don't. I've looked at #11049 (review) and it doesn't sound like there's any specific actions I need to take beyond what I've done.

@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from 3db4e17 to f80cdc2 Compare December 17, 2023 23:41
Note that the main tests are added as behat tests in the admin module
@GuySartorelli GuySartorelli force-pushed the pulls/5/gridfield-with-viewabledata branch from f80cdc2 to 7073246 Compare December 18, 2023 01:20
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me happy.

@maxime-rainville maxime-rainville merged commit 6c69d32 into silverstripe:5 Dec 19, 2023
15 checks passed
@maxime-rainville maxime-rainville deleted the pulls/5/gridfield-with-viewabledata branch December 19, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants