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

Model::findMultipleByIds() always returns a collection #266

Closed
leofeyer opened this Issue Jan 7, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@leofeyer
Copy link
Member

leofeyer commented Jan 7, 2019

Affected version(s)

Contao 4.4+

Description

As noticed by @aschempp, the Model::findMultipleByIds() method always returns a collection, even if there are no models and the method should return null. All derived methods such as FilesModel::findMultipleByIds() behave correctly and return null if there are no models.

@contao/developers Any objections against fixing this in Contao 4.4?

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 7, 2019

@contao/developers Any objections against fixing this in Contao 4.4?

Yes, it's a BC break.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 7, 2019

But it is also a bug.

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 7, 2019

No it's not a bug. It's just not consistent but we're not going to break BC here just for the sake of consistency.

@fritzmg

This comment has been minimized.

Copy link
Contributor

fritzmg commented Jan 7, 2019

@Toflar the doc says

@return Model\Collection|null The model collection or null if there are no records

If it doesn't do what the doc always said, it's a bug, isn't it?

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Jan 7, 2019

It is also very unlikely that someone queries for multiple IDs and none of them are found in the DB, so I'm all for fixing this as well.

@leofeyer leofeyer added this to the 4.4.33 milestone Jan 8, 2019

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 8, 2019

Then the docs are wrong and should be fixed. What the method does is more important than the docs.
So I'm against it.

@leofeyer leofeyer self-assigned this Jan 14, 2019

leofeyer added a commit that referenced this issue Jan 14, 2019

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2019

Fixed in fcd21f8.

@leofeyer leofeyer closed this Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment