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

remove subclass parameter when generating sonata_admin_short_object_information #7845

Conversation

mpoiriert
Copy link
Contributor

@mpoiriert mpoiriert commented Jun 14, 2022

I am targeting this branch, because bug is present in this branch.

Closes #7843.

Changelog

### Fixed

Fix the subclass query parameter conflict in the sonata.admin.action.get_short_object_description controller.

To do

  • Update the tests;
  • Update the documentation;
  • Add an upgrade note.

@mpoiriert
Copy link
Contributor Author

@VincentLanglet This is something that would work and it's really specific to the bug I have found.
Admin may be conflicting somewhere else but I am not sure where and how to reproduce it. Maybe we could fix one at the time ? (Aka per PR).

I would rewrite the url generation of that specific url in a macro since it's getting bigger and noisy.

@VincentLanglet
Copy link
Member

This seems nice.
Can you

  • Add a comment to all the filter to explain the conflict you want to solve (for next developers)
  • Try to add a functional test to reproduce the previous error (and avoid a regression)

@mpoiriert
Copy link
Contributor Author

@VincentLanglet I have add another way to solve this. It may be a bit less clean from a coding point of view but it's safer (if someone generate the url from somewhere), easier to test (just need to make a unit test for the action) and maintain (the action is aware of it's behaviour and no one else need to).

I have add back the query parameter just so the request is still intact at the end but this could be optional and just removing the parameter would be enough.

@mpoiriert
Copy link
Contributor Author

@VincentLanglet I have add another way to solve this. It may be a bit less clean from a coding point of view but it's safer (if someone generate the url from somewhere), easier to test (just need to make a unit test for the action) and maintain (the action is aware of it's behaviour and no one else need to).

I have add back the query parameter just so the request is still intact at the end but this could be optional and just removing the parameter would be enough.

I prefer the second option, what do you think ?

@VincentLanglet
Copy link
Member

VincentLanglet commented Jun 15, 2022

So you just need to remove the subclass before

        $object = $admin->getObject($objectId);
        if (null === $object) {
            throw new NotFoundHttpException(sprintf('Could not find subject for id "%s"', $objectId));
        }

? (And the filter could be removed ?)

This is not easy to understand and would require some comments I think.
But this could be an easy BC fix.
We might add a NEXT_MAJOR comment to ask for changing some subclass parameter to avoid the conflict in Sonata 5

@mpoiriert
Copy link
Contributor Author

? (And the filter could be removed ?)

Yes the filter could be removed.

This is not easy to understand and would require some comments I think.

Or course I'll add comment.

We might add a NEXT_MAJOR comment to ask for changing some subclass parameter to avoid the conflict in Sonata 5

Not sure exactly where. I think the solution I have done with removing the subclass parameter in the action/controller that have conflict with would be the best way to do (and keep it that way).

This make sure that we don't need to bother how the url get generated. Other wise we would need to add the filter in the first solution every where we need it and we will never be sure someone does not generate the url somewhere else without the filter.

I don't know which controller would need this so I don't know where to remove the subclass and this is probably where the NEXT_MAJOR would be.

@VincentLanglet
Copy link
Member

Let's go with this solution then

@mpoiriert mpoiriert force-pushed the issue-7843-subclass-parameter-conflict branch from ca0fcab to c0e9eb3 Compare June 15, 2022 17:10
@mpoiriert
Copy link
Contributor Author

@VincentLanglet I added the automation test and some comment near the code for the developer.

Should be ready

Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

Good for now. For next major we should try to reduce parameters sent to methods to the minimum needed.

@VincentLanglet VincentLanglet merged commit 6d460d3 into sonata-project:4.x Jun 15, 2022
@VincentLanglet
Copy link
Member

I'll wait for #7819 to release

@mpoiriert mpoiriert deleted the issue-7843-subclass-parameter-conflict branch June 15, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subclass parameter conflict between admin
3 participants