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

[3.x] Models\Feature::scopeForBucket() return type of Builder doesn't match returned collection #2154

Closed
markcameron opened this issue Feb 15, 2023 · 3 comments · Fixed by #2161

Comments

@markcameron
Copy link

Description

I am upgrading a project from 2.x to 3.x

When using feature buckets to organize content, and then calling \A17\Twill\Models\Feature::forBucket('bucket-name') on the frontend, it throws the following error:

A17\Twill\Models\Feature::scopeForBucket(): Return value must be of type Illuminate\Database\Eloquent\Builder, Illuminate\Database\Eloquent\Collection returned

This appears to be because the scopeForBucket() method is now typed, and the code within probably does too much for a Laravel Scope method, since it does a ->get() and then does some post processing on the query result.

Steps to reproduce

Follow the steps from the documentation to create Featured Content and then in the front end of your application, render the contents of a bucket using $content = \A17\Twill\Models\Feature::forBucket('bucket-name')

Expected result

A Collection of Models that are stored in the Bucket

Actual result

Throws

A17\Twill\Models\Feature::scopeForBucket(): Return value must be of type Illuminate\Database\Eloquent\Builder, Illuminate\Database\Eloquent\Collection returned

Versions

Twill version: 3.0.0-rc4
Laravel version: 9.52.0
PHP version: 8.2.2
Database engine: mariadb-10

Note

I'm not sure what would be actual end result should be. It's programmed as a scope, so should return a builder and allow it to be chained with more query arguments. So maybe it should just be converted to a simple method on the Eloquent Model so that you can return the processed Collection

haringsrob added a commit that referenced this issue Feb 24, 2023
@haringsrob
Copy link
Contributor

Hey @markcameron,

I have proposed to use the scope as scope and add a helper in #2161.

@ifox ok like this?

@markcameron
Copy link
Author

Hey @haringsrob

Thanks for the PR, fixes the issue on my end. Guess we just need to wait to get it merged into the 3.x branch.

@haringsrob
Copy link
Contributor

Thanks for confirming. Will be in the next release.

haringsrob added a commit that referenced this issue Mar 10, 2023
zeezo887 pushed a commit to zeezo887/twill that referenced this issue Apr 10, 2023
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 a pull request may close this issue.

2 participants