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 selecting multiple entities #8413 #8414

Closed
wants to merge 2 commits into from

Conversation

Cartman34
Copy link

Fix #8413
Now allowing to select multiple entities and return all of them

Fix doctrine#8413
Now allowing to select multiple entities and return all of them
@Cartman34
Copy link
Author

This fix is not enough to really fix issue, it creates another issue.

Through multiple entities, result is not resetted, this fix it
@Cartman34
Copy link
Author

Tested selecting one or more entity in one row through several rows. It seems to work in all cases, it only affects iterations.

@SenseException
Copy link
Member

Thanks for your contribution. Your changes are currently breaking the existing tests. You also need to create a test for your use case.

@Cartman34
Copy link
Author

Cartman34 commented Jan 17, 2021

Thanks for your contribution. Your changes are currently breaking the existing tests. You also need to create a test for your use case.

There is no new code, no new function, why should I develop a unit test ?
It's an important fix and I don't know how to do, so anyone do it and push it 😅
The faulting code was developed recently and pushed without any test...

@SenseException
Copy link
Member

There is no new code, no new function, why should I develop a unit test ?

If changes don't fix or introduce anything, like in a refactoring, then there's no need for new tests. There's still the possibility that an assumed harmless change introduced a bug in a functionality, that wasn't covered by tests yet. New tests will close such gaps in the coverage too.

If your code introduces a change, a feature or a bugfix, then a new test has to guarantee that this newly code change is covered. This doesn't have anything to do with new classes or methods, but with code paths. The test should prevent that someone else is removing your fix accidentally (even after months or years). It's in your and the other affected developers' interest that this can be prevented.

@Cartman34
Copy link
Author

I have no doubt this is very useful and I should provide it but it's out of my knowledges. I developed and shared the fix in a short time but this is a complex test, with probably a solution to mock up the database. I don't have the skills to do that and no time.
So somebody else have to do that.

@beberlei beberlei added this to the 2.8.2 milestone Feb 5, 2021
@beberlei
Copy link
Member

beberlei commented Feb 8, 2021

Superseded by #8467

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.

Issue with toIterable() and selecting multiple entities
3 participants