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 "Filter" from baked collections. #317

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Remove "Filter" from baked collections. #317

merged 1 commit into from
Feb 16, 2022

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Feb 16, 2022

Closes #315

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #317 (22f8411) into master (1ef5fe9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #317   +/-   ##
=========================================
  Coverage     93.66%   93.66%           
  Complexity      224      224           
=========================================
  Files            19       19           
  Lines           616      616           
=========================================
  Hits            577      577           
  Misses           39       39           
Impacted Files Coverage Δ
src/Command/Bake/FilterCollectionCommand.php 89.87% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ef5fe9...22f8411. Read the comment docs.

dereuromark
dereuromark previously approved these changes Feb 16, 2022
Copy link
Member

@dereuromark dereuromark left a comment

Choose a reason for hiding this comment

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

OK
I would still like it if we used Filter suffix everywhere to make it more clear and searchable what those classes are for.

But since that would be a BC break now, I guess we have to go with this fix at this point.

@dereuromark dereuromark dismissed their stale review February 16, 2022 15:02

Actually, my code works correctly with the prefix, maybe docs should be adjusted instead?

@dereuromark
Copy link
Member

Ah, because of this one it works:

$this->Videos->addBehavior('Search.Search', [
	'collectionClass' => VideosFilterCollection::class,
]);

@ADmad
Copy link
Member Author

ADmad commented Feb 16, 2022

Ah, you are setting the collection class as default one. Without that it would also require one to specify the collection as foo_filter instead of just foo in the find call.

So it's best to remove "Filter" avoid confusion for users.

@dereuromark
Copy link
Member

Sure, I would just like to revisit this for a new major then.

@dereuromark dereuromark merged commit 760d996 into master Feb 16, 2022
@dereuromark dereuromark deleted the issue-315 branch February 16, 2022 15:58
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 this pull request may close these issues.

Bug/inconvenience when baking FilterCollections
2 participants