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

GridFieldEditableColumns does not support many_many through relationship's extra fields #371

Open
rasstislav opened this issue Aug 13, 2023 · 11 comments

Comments

@rasstislav
Copy link

rasstislav commented Aug 13, 2023

Hello,

I have this issue.

This module does not support many_many through relationship joined on a separate DataObject (https://docs.silverstripe.org/en/4/developer_guides/model/relations/#many-many-through-relationship-joined-on-a-separate-dataobject).

PRs

@GuySartorelli
Copy link
Collaborator

Hi rasstislav,

Can you please provide some details?
What happens when you try, are there errors? If so what are the errors including the stack trace?
What version of the module and of silverstripe/framework are you seeing this problem on?
What specific component(s) are you using? And what were you doing at the time of the failure? This module comes with a lot of components and I'd be shocked if they all don't work with a many_many through relationship.

@rasstislav
Copy link
Author

Hi @GuySartorelli,

Thank you for your response.

Module supports only standard ManyManyList, but not ManyManyThroughList.

You can see e.g. here https://github.com/symbiote/silverstripe-gridfieldextensions/blob/3.6.3/src/GridFieldEditableColumns.php#L253.

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Aug 14, 2023

Hi @rasstislav
Thank you for that. Please answer all of the questions I asked in my comment, including whether this is limited to just one or some components, or if it's actually all components in this module which don't support that relation type.

@rasstislav
Copy link
Author

I am only using the GridFieldEditableColumns component and for Ranking field is displayed ReadonlyField.

Versions:
silverstripe/framework: 4.13.13
symbiote/silverstripe-gridfieldextensions: 3.6.3

Code:

class Team extends DataObject
{
    private static $db = [
       'Name' => 'Varchar(35)',
    ];

    private static $many_many = [
        'Supporters' => [
            'through' => TeamSupporter::class,
            'from' => 'Team',
            'to' => 'Supporter',
        ]
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();

        $supportersGFConfig = ($supportersGF = $fields->dataFieldByName('Supporters'))
            ->getConfig();

        $columns = ($supportersGFDataColumns = $supportersGFConfig->getComponentByType(GridField\GridFieldDataColumns::class))
            ->getDisplayFields($supportersGF);

        unset($columns['Name']);

        $supportersGFDataColumns->setDisplayFields($columns);

        $supportersGFConfig->addComponent(
            GridFieldEditableColumns::create()
                ->setDisplayFields([
                    'Name' => 'Name',
                    'Ranking' => 'Ranking',
                ]),
            GridField\GridFieldButtonRow::class,
        );

        return $fields;
    }
}

@rasstislav rasstislav changed the title module does not support many_many through relationship module (component GridFieldEditableColumns) does not support many_many through relationship Aug 14, 2023
@GuySartorelli GuySartorelli changed the title module (component GridFieldEditableColumns) does not support many_many through relationship GridFieldEditableColumns does not support many_many through relationship's extra fields Aug 14, 2023
@GuySartorelli
Copy link
Collaborator

In the future please answer all questions asked in comments like that - it wasn't clear at all from your comments what about it wasn't working, so I had to boot up a fresh installation and run through a bunch of scenarios to find out for myself.

In any case: ManyManyThroughList works just fine for most scenarios, but doesn't support extra fields that are added as db fields on the through class. In the code example above, it is the "Ranking" field, which is taken from https://docs.silverstripe.org/en/5/developer_guides/model/relations/#many-many-through and is a db field on the TeamSupporter class.

It's an easy fix so I'll raise a PR momentarily.

@kinglozzer
Copy link
Collaborator

Fixed in #372

@rasstislav
Copy link
Author

@kinglozzer Thanks :).

@GuySartorelli It's better now, but it still doesn't work. Am I doing something wrong?

image

Team

ID ClassName LastEdited Created Name State
1 Team 2023-08-21 13:09:34 2023-08-21 13:00:05 First Team 1
2 Team 2023-08-21 13:10:00 2023-08-21 13:06:45 Second Team 0
3 Team 2023-08-21 13:06:50 2023-08-21 13:06:50 Third Team 0

Supporter

ID ClassName LastEdited Created Name State
1 Supporter 2023-08-21 13:06:22 2023-08-21 13:00:16 First Supporter 0
2 Supporter 2023-08-21 13:10:04 2023-08-21 13:04:34 Second Supporter 1
3 Supporter 2023-08-21 13:06:31 2023-08-21 13:04:38 Third Supporter 0

TeamSupporter

ID ClassName LastEdited Created Ranking TeamID SupporterID
1 TeamSupporter 2023-08-21 13:13:33 2023-08-21 13:00:16 1 1 1
2 TeamSupporter 2023-08-21 13:13:33 2023-08-21 13:07:26 2 1 2
<?php

use SilverStripe\Forms\GridField;
use SilverStripe\ORM\DataObject;
use Symbiote\GridFieldExtensions\GridFieldEditableColumns;

class Team extends DataObject
{
    private static $db = [
       'Name' => 'Varchar(35)',
       'State' => 'Boolean',
    ];

    private static $many_many = [
        "Supporters" => [
            'through' => TeamSupporter::class,
            'from' => 'Team',
            'to' => 'Supporter',
        ]
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();

        if ($supportersGF = $fields->dataFieldByName('Supporters')) {
            $supportersGFConfig = $supportersGF->getConfig();

            $columns = ($supportersGFDataColumns = $supportersGFConfig->getComponentByType(GridField\GridFieldDataColumns::class))
                ->getDisplayFields($supportersGF);

            $columns['State'] = 'State';

            $supportersGFDataColumns->setDisplayFields($columns);

            $supportersGFConfig->addComponent(
                GridFieldEditableColumns::create()
                    ->setDisplayFields([
                        'Ranking' => 'Ranking',
                    ]),
                GridField\GridFieldDataColumns::class,
            );
        }

        return $fields;
    }
}

class Supporter extends DataObject
{
    private static $db = [
       'Name' => 'Varchar(35)',
       'State' => 'Boolean',
    ];

    private static $belongs_many_many = [
        'Supports' => Team::class,
    ];
}

class TeamSupporter extends DataObject
{
    private static $db = [
        'Ranking' => 'Int',
    ];

    private static $has_one = [
        'Team' => Team::class,
        'Supporter' => Supporter::class,
    ];

    private static $default_sort = '"TeamSupporter"."Ranking" ASC';
}

@kinglozzer
Copy link
Collaborator

My bad, I should’ve double-checked the fix was complete before merging.

The field now shows, but it won’t load the value because values on the “through” record for many-many-through aren’t lazily loaded like they are for a regular many-many, they have to be accessed via the DataObject::getJoin() method.

One approach for fixing this would be for us to say that when configuring editable columns, the array key has to be Join.FieldName instead of just FieldName (e.g. 'Join.Ranking' => 'Ranking' in the example above). That will load the correct value, but it breaks the auto-scaffolding of the field again (which the original PR fixed). It can then be fixed again with:

index e42be98..0dfbc58 100644
--- a/src/GridFieldEditableColumns.php
+++ b/src/GridFieldEditableColumns.php
@@ -252,10 +252,15 @@ class GridFieldEditableColumns extends GridFieldDataColumns implements
             }

             if (!$field && ($list instanceof ManyManyList || $list instanceof ManyManyThroughList)) {
-                $extra = $list->getExtraFields();
+                $colName = $col;
+                if ($list instanceof ManyManyThroughList) {
+                    // ManyManyThroughList join records need to be referenced with Join. prefix
+                    $colName = preg_replace('/^join\./i', '', $colName);
+                }

-                if ($extra && array_key_exists($col, $extra ?? [])) {
-                    $field = Injector::inst()->create($extra[$col], $col)->scaffoldFormField();
+                $extra = $list->getExtraFields();
+                if ($extra && array_key_exists($colName, $extra ?? [])) {
+                    $field = Injector::inst()->create($extra[$colName], $col)->scaffoldFormField();
                 }
             }

Do you think that’s a good approach @GuySartorelli?

@GuySartorelli
Copy link
Collaborator

Hmm. This was working for me locally but maybe I did something weird. I don't have time to look at this right now in any depth but we ideally shouldn't need to prefix the extrafield with the join name, as this would introduce a third syntax for handling many_many extrafields in gridfield (the main one being no prefix, and the second one being this weird nonsense).

I'll reopen for now in any case since you're reporting that it's not resolved.

@GuySartorelli GuySartorelli reopened this Aug 22, 2023
@rasstislav
Copy link
Author

Hi @GuySartorelli. Anything new in this issue?

Thanks

@GuySartorelli
Copy link
Collaborator

Nothing new, no. If there's something new there will be comments or a new PR :p

This is an open issue, but that doesn't mean it's something that is being actively looked into. If you'd like to submit a pull request resolving the problem I'll gladly review it.

In the meantime, this is in our backlog along with the other 42 issues on this repository, and all of the issues in the other core and supported modules. Hopefully that puts into perspective why this hasn't had any action since my last comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants