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

FIX Work with ManyManyThroughList #258

Closed
wants to merge 1 commit into from

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented May 11, 2018

This PR depends on silverstripe/silverstripe-framework#8065

This PR adds support for sorting via a ManyManyThroughList

fixes #254

@dhensby
Copy link
Contributor Author

dhensby commented May 11, 2018

There are other instances in the class where we check if $list instanceof ManyManyList but none of those seemed to need changing to make sorting in the CMS work.

@mfendeksilverstripe
Copy link
Contributor

@dhensby I tested your new changes on a setup which uses versioned Many Many Through Relation and it doesn't work properly. I noticed two problems here:

  • GridFieldOrderableRows::executeReorder() has some custom logic for ManyManyList and is likely in need of custom logic for ManyManyThroughList as well (note that ManyManyThroughList does not inherit from ManyManyList), the issue I found is that the $current contains empty values
  • GridFieldOrderableRows::reorderItems() handles versioned ManyManyThroughList as non-versioned which is definitely not good, have a look at how the $isVersioned is determined, you may need to add a new case for ManyManyThroughList

@NightJar
Copy link
Contributor

NightJar commented May 20, 2018

@dhensby @mfendeksilverstripe - I have an in progress work of this as a solution to #25 - however there needs to be a bit more done in order to have it work well, thus there's been no PR just yet. If one or both of you could check it out I'd be grateful.

(see last comment on that issue for links to fork).


WIP PR opened at #260

@tractorcow
Copy link

Merged silverstripe/silverstripe-framework#8065, so this PR can be re-reviewed.

@NightJar
Copy link
Contributor

NightJar commented May 31, 2018

Still too shallow an approach, as @mfendeksilverstripe mentioned.
Notably the first line below each of the changed if conditions; $list->getExtraFields() which is not a thing on a ManyManyThroughList.

As such I saw fit to continue the work on #260 - which is now functional. @mfendeksilverstripe would you care to confirm please?

@dhensby
Copy link
Contributor Author

dhensby commented Jul 4, 2018

replaced by #260

@dhensby dhensby closed this Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GridFieldOrderableRows - No support for ManyManyThroughList
5 participants