Skip to content

Add simple predicate functions #84

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

Closed
wants to merge 2 commits into from
Closed

Add simple predicate functions #84

wants to merge 2 commits into from

Conversation

mabdullahsari
Copy link

Disclaimer: I'm not really sure about the placing of the new trait and its naming. I'm prepared to carry out any changes as requested. Thanks!

The proposed predicate functions in this PR already exist as query scopes within HasRecursiveRelationshipScopes. However, it would be nice to also have them as instance methods on the models itself. I haven't gone overboard and re-implemented every single one as predicates, but these simple ones are IMHO the most useful ones.


Assume the following scenario:

class PageController
{
    public function __invoke(Page $page)
    {
        return View::make('editorialPage', [
            'crumbs' => $this->getBreadcrumbs($page),
            'page' => $page,
        ]);
    }

    private function getBreadcrumbs(Page $page): Collection
    {
        if ($page->isRoot()) {
            return Collection::make([]);
        }

        $page->load('ancestors:id,parent_id,title,slug,path');

        return $page->ancestors->reverse()->values()->push($page);
    }
}
  • The page model, which represents a dynamic page on a website, is always loaded regardless of its relationship status.
  • A breadcrumb trail is generated if, and only if, the page is not a root and thus has ancestors which is then eager loaded.

@staudenmeir
Copy link
Owner

Thanks, I'll take a look.

@mabdullahsari
Copy link
Author

mabdullahsari commented Feb 12, 2022

Thanks, I'll take a look.

Seems like some tests are failing, and rightfully so.
I totally forgot about Eloquent delegating to the QueryBuilder if you do something like Category::isRoot()->get().

I'm not really sure if this fixable as it will probably require a new major release in this case. 😕

@mabdullahsari
Copy link
Author

I think the only possible "fix" in this case is moving the Predicates trait out of the base trait and making it opt-in in userland code. There is no real solution to this problem without introducing a breaking change, I'm afraid.

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.

2 participants