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

Update data object readone resolver for legacy scaffolding #73

Merged

Conversation

satrun77
Copy link
Contributor

@satrun77 satrun77 commented Sep 6, 2021

Module version 0.3
Silverstripe 4.8

The history tab for data objects displays the incorrect history. It displays the history of the first viewed object.

Steps to reproduce:

1- Data object with ModelAdmin.
2- Go to Model Admin
3- Click on any item.
4- View the history. The console output looks ok as follow
Screen Shot 2021-09-07 at 11 17 03 AM
5- View another item.
6- The history tab display incorrect history or the same history as the previously viewed item
Screen Shot 2021-09-07 at 11 20 39 AM

Fix

The fix is based on the code from https://github.com/silverstripe/silverstripe-cms/blob/4/_legacy/GraphQL/ReadOneResolver.php

@unclecheese
Copy link

Thanks for this. A bit of an oversight!

So I think the fix is correct, but we should move it to the admin module. The admin module takes on the responsibility of providing dual compatibility with graphql (it's what shims in the IDFilterType, for instance). We should extend that responsibility to update the READ_ONE resolver, as well, or at least provide a compatible option.

Suggested fix:

Take the resolver you've currently defined as an anonymous function, and move it to a static method in a class in the _legacy graphql directory in silverstripe-admin. Then, just reference the callable in here:

->setResolver([SomeClass::class, 'resolve'])

That way, other modules that may use readOne (thirdparty, e.g.) can have an easy fix when this happens and they won't have to duplicate any code.

Bonus points if we update the resolve function in the current readOne to warn, "Hey, it looks like you've got graphql 4 style filters and you're using the graphql 3 resolver."

Copy link

@unclecheese unclecheese left a comment

Choose a reason for hiding this comment

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

As described in comment

@satrun77 satrun77 force-pushed the pulls/add-dataobject-readone-resolver branch 2 times, most recently from 7453621 to 89e0f91 Compare October 21, 2021 02:42
@satrun77
Copy link
Contributor Author

I have updated this PR and submitted a PR with admin module silverstripe/silverstripe-admin#1260

@satrun77 satrun77 requested a review from unclecheese October 21, 2021 03:27
@satrun77 satrun77 force-pushed the pulls/add-dataobject-readone-resolver branch 2 times, most recently from aa482e5 to 19a747d Compare October 21, 2021 04:54
@satrun77 satrun77 force-pushed the pulls/add-dataobject-readone-resolver branch from 19a747d to 4eeb13e Compare October 21, 2021 05:11
Copy link

@unclecheese unclecheese left a comment

Choose a reason for hiding this comment

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

Awesome work. Thank you!

@unclecheese unclecheese merged commit 616d076 into silverstripe:0.3 Oct 21, 2021
@satrun77 satrun77 deleted the pulls/add-dataobject-readone-resolver branch October 21, 2021 20:08
@satrun77
Copy link
Contributor Author

@unclecheese I have submitted a PR for checking the resolver silverstripe/silverstripe-graphql#411

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.

3 participants