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

Avoid to mutate original request on CRUDController::batchAction() #6321

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

phansys
Copy link
Member

@phansys phansys commented Aug 25, 2020

Subject

Avoid to mutate original request on CRUDController::batchAction().

I am targeting this branch, because this change respects BC.

Follow up of #6271.

Changelog

### Fixed
- Fixed mutating the original request at `CRUDController::batchAction()`.

@phansys phansys added the patch label Aug 25, 2020
@phansys phansys requested a review from a team August 25, 2020 16:56
@jordisala1991
Copy link
Member

There is a psalm failure non related to this Pr that should be fixed

@phansys phansys marked this pull request as ready for review August 25, 2020 17:01
@phansys
Copy link
Member Author

phansys commented Aug 25, 2020

There is a psalm failure non related to this Pr that should be fixed

I'm not familiarized with psalm, so I don't know if these issues are fixable or if we should add them as allowed failures at psalm-baseline.xml.

@phansys
Copy link
Member Author

phansys commented Aug 25, 2020

@jordisala1991, do you have any clue? Otherwise, I'd like to avoid tying this PR to the psalm failure.

@VincentLanglet
Copy link
Member

@jordisala1991, do you have any clue? Otherwise, I'd like to avoid tying this PR to the psalm failure.

Maybe @vladyslavstartsev can help us.

I made #6322

I may understand badly the issue, but I would consider this as a psalm bug.
When using new ArrayCollection(), psalm considers this as ArrayCollection<array-key, empty> but I would expect this to be ArrayCollection<array-key, mixed> instead.

Adding /** @var ArrayCollection<object> $collection */ fix the issues.

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.

Have you check if everything works as expected?

@phansys
Copy link
Member Author

phansys commented Aug 26, 2020

Have you check if everything works as expected?

Yes, at least with batchActionDelete() everything is working as expected.

@jordisala1991 jordisala1991 merged commit 51deb2d into sonata-project:3.x Aug 26, 2020
@jordisala1991
Copy link
Member

Thank you @phansys

@phansys phansys deleted the pr_6271 branch August 26, 2020 11:19
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.

4 participants