Does it make sens to implement a new query to simplify listening all content? #5215

New Issue

OVERLORD · 2026-02-05T09:49:07+03:00

OVERLORD commented

2026-02-05 09:49:07 +03:00

Originally created by @513ry on GitHub (Mar 7, 2025).

API Endpoint or Feature

Hey, I made a hack for drawing a tree of content to easily traverse the wiki from the home page. My colleague noticed issue #616 and thought it would be good to work on a more generic and cleaner version of this code to merge with upstream.

I was thinking of expanding the view switch button with a "Tree View"/"Table View" option to integrate this feature and maybe adding a setting to display it in the sidebar as we currently have it. Anyway, I’ve been trying to follow your convention of having this view button for both Book and Bookshelf views.

To simplify the view code, I wrote an additional query that returns either an expanded version of a Book Collection with a contents member or a Collection of Bookshelves expanded by a books member. Here is my initial implementation with some limitations:

<?php

namespace BookStack\Entities\Queries;

use BookStack\Permissions\PermissionApplicator;
use BookStack\Entities\Models\Entity;
use BookStack\Entities\Models\Book;
use BookStack\Entities\Models\Bookshelf;
use Illuminate\Database\Eloquent\Collection;
use InvalidArgumentException;

class QueryUserContent
{
    private function __construct() {}

    /**
     * Return all books available for the current user with 'content' field.
     */
    public static function books(string $sortContentBy = 'id'): ?Collection
    {
        // Return null if no user is logged in
        if (user()->isGuest()) {
            return null;
        }

        $books = self::sortCollection(
            self::filterEntities(Book::class), $sortContentBy
        );
        // Expand books with their contents
        foreach ($books as $id => $book) {
            $directPages =
                ($directPages = $book->directPages) ? self::sortCollection($directPages, $sortContentBy) : [];
            $chapters =
                ($chapters = $book->chapters()->with('pages')->get()) ? self::sortCollection($chapters, $sortContent
By) : [];
            $contents = $chapters->union($directPages);
            $books[$id]['contents'] = $contents;
        }
        return $books;
    }

    /**
     * Return all books avaiable for the current user categorized in shelves
     * with 'books' field.
     *
     * @param string sortShelvesBy - Key by which to sort queried shelves
     * @param string sortContentBy - Key by which to sort queried content
     * @return Collection
     */
    public static function shelves(string $sortShelvesBy = 'id',
                                   string $sortContentBy = 'id'): ?Collection
    {
        $books = self::books($sortContentBy);
        if (!$books) return null;
        $shelves = self::sortCollection(
            self::filterEntities(Bookshelf::class), $sortShelvesBy
        );
        // Expand bookshelves with their books (ignores one to many relation of
        // books to shelves by using the first associated shelf)
        $shelves = $shelves->map(function ($shelf) use ($books) {
            $shelfBooks = $books->filter(fn($book) =>
                $book->shelves instanceof Collection
                && ($firstShelf = $book->shelves->first()) !== null
                && $firstShelf->id === $shelf->id
            );
            $shelf['books'] = $shelfBooks->values();
            return $shelf;
        });
        return $shelves;
    }

    /**
     * Ensure Entity type and return items that the user has view permission for.
     *
     * @param string entityClass - Name of the Entity subclass
     * @return Collection
     */
    private static function filterEntities(string $entityClass): Collection
    {
        if (!is_subclass_of($entityClass, Entity::class)) {
            throw new InvalidArgumentException("$entityClass must be a subclass of Entity");
        }

        $query = $entityClass::query();
        $permissions = new PermissionApplicator();
        $query = $permissions->restrictEntityQuery($query);
        return $query->get();
    }

    /**
     * Sort an Eloquent Collection by key.
     *
     * @return Collection
     */
    private static function sortCollection(Collection $collection, string $key): Collection
    {
        if ($collection->isEmpty()) {
            return $collection;
        }
        if (!array_key_exists($key, $collection->first()->getAttributes())) {
            throw new InvalidArgumentException(
                "Expected second argument to be one of Collection's keys but get \"$key\""
            );
        }

        return $collection->sortBy($key)->values();
    }
}

Any criticism is welcome since I just picked up PHP this week. I think the comments explain some of the limitations of this code so far.

However, I noticed that this will probably create a lot of overhead, given that you already list shelves and books accordingly when checking homepage options in the home controller:

if ($homepageOption === 'bookshelves') {
  $shelves = $this->queries->shelves->visibleForListWithCover()
    ->orderBy($commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder())
    ->paginate(18);
  $data = array_merge($commonData, ['shelves' => $shelves]);

  return view('home.shelves', $data);
}

if ($homepageOption === 'books') {
  $books = $this->queries->books->visibleForListWithCover()
    ->orderBy($commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder())
    ->paginate(18);
  $data = array_merge($commonData, ['books' => $books]);

  return view('home.books', $data);
}

I wonder now - should I do this work of associating contents with books and books with shelves inside my partial element code, or do you see a use for this query to reduce redundancy elsewhere? In the latter case, it would make sense to replace the home controller’s $books and $shelves with objects returned by this query.

Use-Case

Implementing solution to #616.

Additional context

No response

Originally created by @513ry on GitHub (Mar 7, 2025). ### API Endpoint or Feature Hey, I made a hack for drawing a tree of content to easily traverse the wiki from the home page. My colleague noticed issue #616 and thought it would be good to work on a more generic and cleaner version of this code to merge with upstream. I was thinking of expanding the view switch button with a "Tree View"/"Table View" option to integrate this feature and maybe adding a setting to display it in the sidebar as we currently have it. Anyway, I’ve been trying to follow your convention of having this view button for both `Book` and `Bookshelf` views. To simplify the view code, I wrote an additional query that returns either an expanded version of a `Book` `Collection` with a contents member or a `Collection` of `Bookshelve`s expanded by a books member. Here is my initial implementation with some limitations: ``` php <?php namespace BookStack\Entities\Queries; use BookStack\Permissions\PermissionApplicator; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; use Illuminate\Database\Eloquent\Collection; use InvalidArgumentException; class QueryUserContent { private function __construct() {} /** * Return all books available for the current user with 'content' field. */ public static function books(string $sortContentBy = 'id'): ?Collection { // Return null if no user is logged in if (user()->isGuest()) { return null; } $books = self::sortCollection( self::filterEntities(Book::class), $sortContentBy ); // Expand books with their contents foreach ($books as $id => $book) { $directPages = ($directPages = $book->directPages) ? self::sortCollection($directPages, $sortContentBy) : []; $chapters = ($chapters = $book->chapters()->with('pages')->get()) ? self::sortCollection($chapters, $sortContent By) : []; $contents = $chapters->union($directPages); $books[$id]['contents'] = $contents; } return $books; } /** * Return all books avaiable for the current user categorized in shelves * with 'books' field. * * @param string sortShelvesBy - Key by which to sort queried shelves * @param string sortContentBy - Key by which to sort queried content * @return Collection */ public static function shelves(string $sortShelvesBy = 'id', string $sortContentBy = 'id'): ?Collection { $books = self::books($sortContentBy); if (!$books) return null; $shelves = self::sortCollection( self::filterEntities(Bookshelf::class), $sortShelvesBy ); // Expand bookshelves with their books (ignores one to many relation of // books to shelves by using the first associated shelf) $shelves = $shelves->map(function ($shelf) use ($books) { $shelfBooks = $books->filter(fn($book) => $book->shelves instanceof Collection && ($firstShelf = $book->shelves->first()) !== null && $firstShelf->id === $shelf->id ); $shelf['books'] = $shelfBooks->values(); return $shelf; }); return $shelves; } /** * Ensure Entity type and return items that the user has view permission for. * * @param string entityClass - Name of the Entity subclass * @return Collection */ private static function filterEntities(string $entityClass): Collection { if (!is_subclass_of($entityClass, Entity::class)) { throw new InvalidArgumentException("$entityClass must be a subclass of Entity"); } $query = $entityClass::query(); $permissions = new PermissionApplicator(); $query = $permissions->restrictEntityQuery($query); return $query->get(); } /** * Sort an Eloquent Collection by key. * * @return Collection */ private static function sortCollection(Collection $collection, string $key): Collection { if ($collection->isEmpty()) { return $collection; } if (!array_key_exists($key, $collection->first()->getAttributes())) { throw new InvalidArgumentException( "Expected second argument to be one of Collection's keys but get \"$key\"" ); } return $collection->sortBy($key)->values(); } } ``` Any criticism is welcome since I just picked up PHP this week. I think the comments explain some of the limitations of this code so far. However, I noticed that this will probably create a lot of overhead, given that you already list shelves and books accordingly when checking homepage options in the home controller: ``` php if ($homepageOption === 'bookshelves') { $shelves = $this->queries->shelves->visibleForListWithCover() ->orderBy($commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()) ->paginate(18); $data = array_merge($commonData, ['shelves' => $shelves]); return view('home.shelves', $data); } if ($homepageOption === 'books') { $books = $this->queries->books->visibleForListWithCover() ->orderBy($commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()) ->paginate(18); $data = array_merge($commonData, ['books' => $books]); return view('home.books', $data); } ``` I wonder now - should I do this work of associating contents with books and books with shelves inside my partial element code, or do you see a use for this query to reduce redundancy elsewhere? In the latter case, it would make sense to replace the home controller’s `$books` and `$shelves` with objects returned by this query. ### Use-Case Implementing solution to #616. ### Additional context _No response_

OVERLORD added the 🔩 API Request label 2026-02-05 09:49:07 +03:00

OVERLORD closed this issue

2026-02-05 09:49:08 +03:00

OVERLORD commented

2026-02-05 09:49:10 +03:00

@ssddanbrown commented on GitHub (Mar 8, 2025):

Hi @513ry,
Thanks for offering, but I don't really see that existing code as redundant as it's a bit different to what you're getting via that class, and since that existing query logic is within its own class of queries that are used elsewhere.

BTW, If it's important to your use-case, I'm not sure your class is considering permissions/visibility for pages/chapters.
There is an example of book tree building here.

@ssddanbrown commented on GitHub (Mar 8, 2025): Hi @513ry, Thanks for offering, but I don't really see that existing code as redundant as it's a bit different to what you're getting via that class, and since that existing query logic is within its own [class of queries](https://github.com/BookStackApp/BookStack/blob/ed9c013f6e7ee7a7bfe1c67b4cb1a84ca198b7a6/app/Entities/Queries/BookshelfQueries.php#L57-L60) that are used elsewhere. BTW, If it's important to your use-case, I'm not sure your class is considering permissions/visibility for pages/chapters. There is an example of [book tree building here](https://github.com/BookStackApp/BookStack/blob/ac0cd9995d8b420e33e392ba82d40bde8df94205/app/Entities/Tools/BookContents.php#L44).

OVERLORD commented

2026-02-05 09:49:11 +03:00

@513ry commented on GitHub (Mar 8, 2025):

Since there is already this nice tool which solves the problem of sorting book contents for a tree, maybe the better solution would be to use this tool to implement similar thing for bookshelves. What do you think about this?

@513ry commented on GitHub (Mar 8, 2025): Since there is already [this nice tool](https://github.com/BookStackApp/BookStack/blob/ac0cd9995d8b420e33e392ba82d40bde8df94205/app/Entities/Tools/BookContents.php#L44) which solves the problem of sorting book contents for a tree, maybe the better solution would be to use this tool to implement similar thing for bookshelves. What do you think about this?

OVERLORD commented

2026-02-05 09:49:13 +03:00

@ssddanbrown commented on GitHub (Mar 8, 2025):

We don't have a need in the core codebase for that right now so it's not something I'd look to add.
Use would direct implementation, and we don't have a use for that at the moment.

@ssddanbrown commented on GitHub (Mar 8, 2025): We don't have a need in the core codebase for that right now so it's not something I'd look to add. Use would direct implementation, and we don't have a use for that at the moment.

OVERLORD commented

2026-02-05 09:49:15 +03:00

@513ry commented on GitHub (Mar 8, 2025):

Should I then restrict the implementation to the controller for the tree view partial?

Do you intend thought to merge #616 with the core codebase? A lot of people seem to want this.

@513ry commented on GitHub (Mar 8, 2025): Should I then restrict the implementation to the controller for the tree view partial? Do you intend thought to merge #616 with the core codebase? A lot of people seem to want this.

OVERLORD commented

2026-02-05 09:49:17 +03:00

@ssddanbrown commented on GitHub (Mar 8, 2025):

Should I then restrict the implementation to the controller for the tree view partial?

Depends on your customization I guess. If this was officially done, I'd still have a builder class which is then used in that controller method, although some refactoring of the home route handling might be warranted upon further growth.

Do you intend thought to merge https://github.com/BookStackApp/BookStack/issues/616 with the core codebase?

No current intent, but I'm not specifically against the idea, just needs some consideration/evaluation to confirm the idea before going to any kind of implementation stage.
I'll add a comment there to start this process for discussion/feedback.

@ssddanbrown commented on GitHub (Mar 8, 2025): > Should I then restrict the implementation to the controller for the tree view partial? Depends on your customization I guess. If this was officially done, I'd still have a builder class which is then used in that controller method, although some refactoring of the home route handling might be warranted upon further growth. > Do you intend thought to merge https://github.com/BookStackApp/BookStack/issues/616 with the core codebase? No current intent, but I'm not specifically against the idea, just needs some consideration/evaluation to confirm the idea before going to any kind of implementation stage. I'll add a comment there to start this process for discussion/feedback.

OVERLORD commented

2026-02-05 09:49:18 +03:00

@ssddanbrown commented on GitHub (May 8, 2025):

I'm going to go ahead and close this off since there's been no further follow-up.

@ssddanbrown commented on GitHub (May 8, 2025): I'm going to go ahead and close this off since there's been no further follow-up.

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/BookStack#5215