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/API Always set polymorphic class name in gridfield on new records. #10297

Conversation

GuySartorelli
Copy link
Member

When creating a new record in a gridfield item edit form and polymorphic has_one relations have the ID set, but not the class. In polymorphic relations the ID is useless without the class.

This has caused at least one known issue (see parent issue below) where a relation is relied on for canX permissions but ultimately the relation only exists after the record has been created.

Parent issue

@GuySartorelli GuySartorelli force-pushed the pulls/4/gridfield-set-polymorphic-class branch from 95ede91 to faacbd9 Compare May 2, 2022 07:04
@GuySartorelli GuySartorelli changed the title Pulls/4/gridfield set polymorphic class FIX/API Always set polymorphic class name in gridfield on new records. May 2, 2022
@GuySartorelli GuySartorelli force-pushed the pulls/4/gridfield-set-polymorphic-class branch 3 times, most recently from 43a4446 to db4153e Compare May 3, 2022 01:35
@michalkleiner
Copy link
Contributor

I thought I submitted a PR for this in past, but could've been related to polymorphic has_one, not has_many.

Unrelated — I don't think we support two change types in the commit message such as FIX/API or ENH/DOC for automatic changelog creation, so you need to choose one or split the commit into two.

@GuySartorelli
Copy link
Member Author

I don't think we support two change types in the commit message

There are two commits, one for each the API and the FIX. I don't think the PR name itself will affect the changelog.
If I'm wrong about that then let me know and I'll change the changelog tag to API as the higher priority of the two.

@GuySartorelli GuySartorelli force-pushed the pulls/4/gridfield-set-polymorphic-class branch from db4153e to cd3573f Compare May 3, 2022 03:41
@michalkleiner
Copy link
Contributor

True, the issue name has no role in that, forgot to check the list of actual commits. All good.

/**
* Gets the field name which holds the related object class.
*/
public function getForeignClassKey(): string
Copy link
Contributor

@maxime-rainville maxime-rainville May 9, 2022

Choose a reason for hiding this comment

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

Can we add an explicit test for this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing

Copy link
Member Author

Choose a reason for hiding this comment

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

Test added.

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.

Looks good ... I would like to see an explicit test to cover that new method.

@sabina-talipova Can you confirm that you've manually tested that this PR fixes the user form issue?

@sabina-talipova
Copy link
Contributor

sabina-talipova commented May 10, 2022

I've done test in my local environment. Manual tests - PASSED (NO ERROR or misbehaving), phpUnit tests in local - PASSED. Should I run other tests as well?

@GuySartorelli GuySartorelli force-pushed the pulls/4/gridfield-set-polymorphic-class branch from cd3573f to 5436df5 Compare May 11, 2022 02:05
@GuySartorelli
Copy link
Member Author

@maxime-rainville I've added the test you requested.

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.

4 participants