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

NEW Allow DataObject classes to define scaffolded relation formfields #11269

Conversation

GuySartorelli
Copy link
Member

Implements new API for scaffolding has_one, has_many, and many_many relations.

Needs silverstripe/silverstripe-assets#613 to ensure file and image has_one relations are scaffolded with the same logic they were before, hence the bump in dependency.

Issue

src/ORM/DataObject.php Show resolved Hide resolved
src/ORM/DataObject.php Show resolved Hide resolved
src/ORM/DataObject.php Show resolved Hide resolved
@GuySartorelli GuySartorelli force-pushed the pulls/5/scaffold-relation-fields branch from 085d0b6 to a358fb7 Compare June 6, 2024 00:17
Comment on lines +2495 to +2496
* @param string $fieldName The name we usually expect the field to have. This is often the has_one relation
* name with "ID" suffixed to it.
Copy link
Member Author

Choose a reason for hiding this comment

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

Note I've said it's "often" that because you could create a DBForeignKey object with whatever field name you want. It would be unusual but it's valid.

@GuySartorelli GuySartorelli force-pushed the pulls/5/scaffold-relation-fields branch from a358fb7 to cd72258 Compare June 6, 2024 00:54
@GuySartorelli
Copy link
Member Author

Noticed a small issue - if $includeInOwnTab was false, it would still create the tab. Now it doesn't create the tab unless that is set to true. Also added test coverage to check that.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Tested locally, a couple of minor things

  • Seems that $includeInOwnTab doesn't work for many_many, though does work for has_many. I did update the parent gridfield like in the doc instructions, so may be related to that. I do have the latest sha cd72258 for framework
  • These new API are not called when the DataObject is a SiteTree, because that of course does its own thing. Worth mentioning that in the docs.
  • There's no consideration given for extension hooks. We should probably add these for each of the new methods. For instance the first thing I tried was to swap out LinkField for something else on Link, though I'm unable to do that.

Testing setup:

app/src/MyDataObject.php

<?php

use SilverStripe\ORM\DataObject;
use SilverStripe\Forms\FormField;
use SilverStripe\Forms\SearchableDropdownField;
use SilverStripe\Forms\SearchableMultiDropdownField;
use SilverStripe\Forms\GridField\GridFieldAddExistingAutocompleter;

class MyDataObject extends DataObject
{
    private static $table_name = 'MyDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $has_one = [
        'AnotherDataObject' => AnotherDataObject::class,
        'Page' => Page::class,
    ];

    public function scaffoldFormFieldForHasOne(
        string $fieldName,
        ?string $fieldTitle,
        string $relationName,
        DataObject $ownerRecord
    ): FormField {
        // Return a form field that should be used for selecting this model type for has_one relations.
        $field = SearchableDropdownField::create($fieldName, $fieldTitle, MyDataObject::get());
        $field->setIsSearchable(false);
        return $field;
    }

    public function scaffoldFormFieldForHasMany(
        string $relationName,
        ?string $fieldTitle,
        DataObject $ownerRecord,
        bool &$includeInOwnTab
    ): FormField {
        // If this should be in its own tab, set $includeInOwnTab to true, otherwise set it to false.
        $includeInOwnTab = false;
        // Return a form field that should be used for selecting this model type for has_many relations.
        $field = SearchableMultiDropdownField::create($relationName, $fieldTitle, MyDataObject::get());
        $field->setIsSearchable(false);
        return $field;
    }

    public function scaffoldFormFieldForManyMany(
        string $relationName,
        ?string $fieldTitle,
        DataObject $ownerRecord,
        bool &$includeInOwnTab
    ): FormField {
        $includeInOwnTab = false;
        // The default implementation for this method returns a GridField, which we can modify.
        $gridField = parent::scaffoldFormFieldForManyMany($relationName, $fieldTitle, $ownerRecord, $includeInOwnTab);
        $gridField->getConfig()->removeComponentsByType(GridFieldAddExistingAutocompleter::class);
        return $gridField;
    }
}

app/src/MyModelAdmin.php

<?php

use SilverStripe\Admin\ModelAdmin;

class MyModelAdmin extends ModelAdmin
{
    private static $url_segment = 'MyModelAdmin';

    private static $menu_title = 'My model admin';

    private static $managed_models = [
        MyDataObject::class,
    ];
}

app/src/AnotherDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class AnotherDataObject extends DataObject
{
    private static $has_one = [
        'HasOneMyDataObject' => MyDataObject::class,
    ];

    private static $has_many = [
        'HasManyMyDataObjects' => MyDataObject::class,
    ];

    private static $many_many = [
        'ManyManyMyDataObjects' => MyDataObject::class,
    ];
}

app/src/AnotherModelAdmin.php

<?php

use SilverStripe\Admin\ModelAdmin;

class AnotherModelAdmin extends ModelAdmin
{
    private static $url_segment = 'AnotherModelAdmin';

    private static $menu_title = 'Another admin';

    private static $managed_models = [
        AnotherDataObject::class,
    ];
}

app/src/Page.php

<?php

use SilverStripe\CMS\Model\SiteTree;

class Page extends SiteTree
{
    private static $has_one = [
        'HasOneMyDataObjects' => MyDataObject::class,
    ];

    private static $has_many = [
        'HasManyMyDataObjects' => MyDataObject::class,
    ];

    private static $many_many = [
        'ManyManyMyDataObjects' => MyDataObject::class,
    ];
}

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jun 6, 2024

These new API are not called when the DataObject is a SiteTree, because that of course does its own thing. Worth mentioning that in the docs.

I think it's known that SiteTree doesn't use scaffolded form fields. I'm not sure where you'd want that documented. Feel free to point out somewhere specific to add it if you want it added (i.e. just the changelog? Or in the scaffolding page generally somewhere?) - and please make that comment in the docs PR instead of here.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jun 6, 2024

Seems that $includeInOwnTab doesn't work for many_many, though does work for has_many. I did update the parent gridfield like in the doc instructions, so may be related to that. I do have the latest sha cd72258 for framework

You're setting it, but then you're calling the parent method which sets it again. You need to set it after calling the parent method.

If you like I can remove the bit that's setting it in the parent method, since I think it's setting it to the same as the default value anyway?

@GuySartorelli
Copy link
Member Author

There's no consideration given for extension hooks. We should probably add these for each of the new methods. For instance the first thing I tried was to swap out LinkField for something else on Link, though I'm unable to do that.

I had intentionally excluded them because this is going to be called a lot and that means the overhead of calling extensions isn't going to be small.

I can add them if you still want them added after knowing that context.

@emteknetnz
Copy link
Member

emteknetnz commented Jun 6, 2024

I've made an recommended up to the docs PR regarding SiteTree forms

Tested moving the $includeInOwnTab locally, you are correct it worth when setting afterwards

May as well keep the hooks off, yes there would be a lot of extra hook calls, and it's a pretty niche scenario. People still have the option of swapping out something like core classes with their own subclass using Injector

@emteknetnz emteknetnz merged commit be0eab2 into silverstripe:5 Jun 7, 2024
15 checks passed
@emteknetnz emteknetnz deleted the pulls/5/scaffold-relation-fields branch June 7, 2024 05:06
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