Cleaned up some user/image areas of the app

Further cleanup of docblocks and standardisation of repos.

app/Auth/Role.php

@@ -5,6 +5,7 @@ use BookStack\Auth\Permissions\RolePermission;
 use BookStack\Interfaces\Loggable;
 use BookStack\Model;
 use Illuminate\Database\Eloquent\Collection;
+use Illuminate\Database\Eloquent\Relations\BelongsToMany;
 use Illuminate\Database\Eloquent\Relations\HasMany;
 
 /**
@@ -23,7 +24,7 @@ class Role extends Model implements Loggable
     /**
      * The roles that belong to the role.
      */
-    public function users()
+    public function users(): BelongsToMany
     {
         return $this->belongsToMany(User::class)->orderBy('name', 'asc');
     }
@@ -39,7 +40,7 @@ class Role extends Model implements Loggable
     /**
      * The RolePermissions that belong to the role.
      */
-    public function permissions()
+    public function permissions(): BelongsToMany
     {
         return $this->belongsToMany(RolePermission::class, 'permission_role', 'role_id', 'permission_id');
     }

app/Auth/UserRepo.php

@@ -8,25 +8,24 @@ use BookStack\Entities\Models\Page;
 use BookStack\Exceptions\NotFoundException;
 use BookStack\Exceptions\UserUpdateException;
 use BookStack\Uploads\Image;
+use BookStack\Uploads\UserAvatars;
 use Exception;
 use Illuminate\Database\Eloquent\Builder;
+use Illuminate\Database\Eloquent\Collection;
 use Illuminate\Pagination\LengthAwarePaginator;
 use Images;
 use Log;
 
 class UserRepo
 {
-
+    protected $userAvatar;
-    protected $user;
-    protected $role;
 
     /**
      * UserRepo constructor.
      */
-    public function __construct(User $user, Role $role)
+    public function __construct(UserAvatars $userAvatar)
     {
-        $this->user = $user;
+        $this->userAvatar = $userAvatar;
-        $this->role = $role;
     }
 
     /**
@@ -34,25 +33,23 @@ class UserRepo
      */
     public function getByEmail(string $email): ?User
     {
-        return $this->user->where('email', '=', $email)->first();
+        return User::query()->where('email', '=', $email)->first();
     }
 
     /**
-     * @param int $id
+     * Get a user by their ID.
-     * @return User
      */
-    public function getById($id)
+    public function getById(int $id): User
     {
-        return $this->user->newQuery()->findOrFail($id);
+        return User::query()->findOrFail($id);
     }
 
     /**
      * Get all the users with their permissions.
-     * @return Builder|static
      */
-    public function getAllUsers()
+    public function getAllUsers(): Collection
     {
-        return $this->user->with('roles', 'avatar')->orderBy('name', 'asc')->get();
+        return User::query()->with('roles', 'avatar')->orderBy('name', 'asc')->get();
     }
 
     /**
@@ -68,7 +65,7 @@ class UserRepo
                 ->take(1);
         }
 
-        $query = $this->user->with(['roles', 'avatar', 'latestActivity'])
+        $query = User::query()->with(['roles', 'avatar', 'latestActivity'])
             ->orderBy($sort, $sortData['order']);
 
         if ($sortData['search']) {
@@ -96,14 +93,12 @@ class UserRepo
 
     /**
      * Assign a user to a system-level role.
-     * @param User $user
-     * @param $systemRoleName
      * @throws NotFoundException
      */
-    public function attachSystemRole(User $user, $systemRoleName)
+    public function attachSystemRole(User $user, string $systemRoleName)
     {
-        $role = $this->role->newQuery()->where('system_name', '=', $systemRoleName)->first();
+        $role = Role::getSystemRole($systemRoleName);
-        if ($role === null) {
+        if (is_null($role)) {
             throw new NotFoundException("Role '{$systemRoleName}' not found");
         }
         $user->attachRole($role);
@@ -111,26 +106,23 @@ class UserRepo
 
     /**
      * Checks if the give user is the only admin.
-     * @param User $user
-     * @return bool
      */
-    public function isOnlyAdmin(User $user)
+    public function isOnlyAdmin(User $user): bool
     {
         if (!$user->hasSystemRole('admin')) {
             return false;
         }
 
-        $adminRole = $this->role->getSystemRole('admin');
+        $adminRole = Role::getSystemRole('admin');
-        if ($adminRole->users->count() > 1) {
+        if ($adminRole->users()->count() > 1) {
             return false;
         }
+
         return true;
     }
 
     /**
      * Set the assigned user roles via an array of role IDs.
-     * @param User $user
-     * @param array $roles
      * @throws UserUpdateException
      */
     public function setUserRoles(User $user, array $roles)
@@ -145,14 +137,11 @@ class UserRepo
     /**
      * Check if the given user is the last admin and their new roles no longer
      * contains the admin role.
-     * @param User $user
-     * @param array $newRoles
-     * @return bool
      */
     protected function demotingLastAdmin(User $user, array $newRoles) : bool
     {
         if ($this->isOnlyAdmin($user)) {
-            $adminRole = $this->role->getSystemRole('admin');
+            $adminRole = Role::getSystemRole('admin');
             if (!in_array(strval($adminRole->id), $newRoles)) {
                 return true;
             }
@@ -166,18 +155,18 @@ class UserRepo
      */
     public function create(array $data, bool $emailConfirmed = false): User
     {
-        return $this->user->forceCreate([
+        $details = [
             'name'     => $data['name'],
             'email'    => $data['email'],
             'password' => bcrypt($data['password']),
             'email_confirmed' => $emailConfirmed,
             'external_auth_id' => $data['external_auth_id'] ?? '',
-        ]);
+        ];
+        return User::query()->forceCreate($details);
     }
 
     /**
      * Remove the given user from storage, Delete all related content.
-     * @param User $user
      * @throws Exception
      */
     public function destroy(User $user)
@@ -187,7 +176,10 @@ class UserRepo
         $user->delete();
         
         // Delete user profile images
-        $profileImages = Image::where('type', '=', 'user')->where('uploaded_to', '=', $user->id)->get();
+        $profileImages = Image::query()->where('type', '=', 'user')
+            ->where('uploaded_to', '=', $user->id)
+            ->get();
+
         foreach ($profileImages as $image) {
             Images::destroy($image);
         }
@@ -195,12 +187,8 @@ class UserRepo
 
     /**
      * Get the latest activity for a user.
-     * @param User $user
-     * @param int $count
-     * @param int $page
-     * @return array
      */
-    public function getActivity(User $user, $count = 20, $page = 0)
+    public function getActivity(User $user, int $count = 20, int $page = 0): array
     {
         return Activity::userActivity($user, $count, $page);
     }
@@ -241,33 +229,22 @@ class UserRepo
 
     /**
      * Get the roles in the system that are assignable to a user.
-     * @return mixed
      */
-    public function getAllRoles()
+    public function getAllRoles(): Collection
     {
-        return $this->role->newQuery()->orderBy('display_name', 'asc')->get();
+        return Role::query()->orderBy('display_name', 'asc')->get();
     }
 
     /**
      * Get an avatar image for a user and set it as their avatar.
      * Returns early if avatars disabled or not set in config.
-     * @param User $user
-     * @return bool
      */
-    public function downloadAndAssignUserAvatar(User $user)
+    public function downloadAndAssignUserAvatar(User $user): void
     {
-        if (!Images::avatarFetchEnabled()) {
-            return false;
-        }
-
         try {
-            $avatar = Images::saveUserAvatar($user);
+            $this->userAvatar->fetchAndAssignToUser($user);
-            $user->avatar()->associate($avatar);
-            $user->save();
-            return true;
         } catch (Exception $e) {
             Log::error('Failed to save user avatar image');
-            return false;
         }
     }
 }

app/Console/Commands/CreateAdmin.php

@@ -28,8 +28,6 @@ class CreateAdmin extends Command
 
     /**
      * Create a new command instance.
-     *
-     * @param UserRepo $userRepo
      */
     public function __construct(UserRepo $userRepo)
     {

app/Entities/Models/Entity.php

@@ -205,12 +205,12 @@ abstract class Entity extends Ownable
     }
 
     /**
-     * Get entity type.
+     * Get the entity type as a simple lowercase word.
-     * @return mixed
      */
-    public static function getType()
+    public static function getType(): string
     {
-        return strtolower(static::getClassName());
+        $className = array_slice(explode('\\', static::class), -1, 1)[0];
+        return strtolower($className);
     }
 
     /**

app/Http/Controllers/UserController.php

@@ -188,7 +188,7 @@ class UserController extends Controller
             $user->image_id = $image->id;
         }
 
-        // Delete the profile image if set to
+        // Delete the profile image if reset option is in request
         if ($request->has('profile_image_reset')) {
             $this->imageRepo->destroyImage($user->avatar);
         }

app/Ownable.php

@@ -26,12 +26,4 @@ abstract class Ownable extends Model
         return $this->belongsTo(User::class, 'updated_by');
     }
 
-    /**
-     * Gets the class name.
-     * @return string
-     */
-    public static function getClassName()
-    {
-        return strtolower(array_slice(explode('\\', static::class), -1, 1)[0]);
-    }
 }

app/Uploads/ImageService.php

@@ -1,7 +1,5 @@
 <?php namespace BookStack\Uploads;
 
-use BookStack\Auth\User;
-use BookStack\Exceptions\HttpFetchException;
 use BookStack\Exceptions\ImageUploadException;
 use DB;
 use ErrorException;
@@ -17,24 +15,21 @@ use Symfony\Component\HttpFoundation\File\UploadedFile;
 
 class ImageService
 {
-
     protected $imageTool;
     protected $cache;
     protected $storageUrl;
     protected $image;
-    protected $http;
     protected $fileSystem;
 
     /**
      * ImageService constructor.
      */
-    public function __construct(Image $image, ImageManager $imageTool, FileSystem $fileSystem, Cache $cache, HttpFetcher $http)
+    public function __construct(Image $image, ImageManager $imageTool, FileSystem $fileSystem, Cache $cache)
     {
         $this->image = $image;
         $this->imageTool = $imageTool;
         $this->fileSystem = $fileSystem;
         $this->cache = $cache;
-        $this->http = $http;
     }
 
     /**
@@ -77,14 +72,9 @@ class ImageService
 
     /**
      * Save a new image from a uri-encoded base64 string of data.
-     * @param string $base64Uri
-     * @param string $name
-     * @param string $type
-     * @param int $uploadedTo
-     * @return Image
      * @throws ImageUploadException
      */
-    public function saveNewFromBase64Uri(string $base64Uri, string $name, string $type, $uploadedTo = 0)
+    public function saveNewFromBase64Uri(string $base64Uri, string $name, string $type, int $uploadedTo = 0): Image
     {
         $splitData = explode(';base64,', $base64Uri);
         if (count($splitData) < 2) {
@@ -94,30 +84,11 @@ class ImageService
         return $this->saveNew($name, $data, $type, $uploadedTo);
     }
 
-    /**
-     * Gets an image from url and saves it to the database.
-     * @param             $url
-     * @param string $type
-     * @param bool|string $imageName
-     * @return mixed
-     * @throws Exception
-     */
-    private function saveNewFromUrl($url, $type, $imageName = false)
-    {
-        $imageName = $imageName ? $imageName : basename($url);
-        try {
-            $imageData = $this->http->fetch($url);
-        } catch (HttpFetchException $exception) {
-            throw new Exception(trans('errors.cannot_get_image_from_url', ['url' => $url]));
-        }
-        return $this->saveNew($imageName, $imageData, $type);
-    }
-
     /**
      * Save a new image into storage.
      * @throws ImageUploadException
      */
-    private function saveNew(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image
+    public function saveNew(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image
     {
         $storage = $this->getStorage($type);
         $secureUploads = setting('app-secure-images');
@@ -327,56 +298,6 @@ class ImageService
         return (count($files) === 0 && count($folders) === 0);
     }
 
-    /**
-     * Save an avatar image from an external service.
-     * @throws Exception
-     */
-    public function saveUserAvatar(User $user, int $size = 500): Image
-    {
-        $avatarUrl = $this->getAvatarUrl();
-        $email = strtolower(trim($user->email));
-
-        $replacements = [
-            '${hash}' => md5($email),
-            '${size}' => $size,
-            '${email}' => urlencode($email),
-        ];
-
-        $userAvatarUrl = strtr($avatarUrl, $replacements);
-        $imageName = str_replace(' ', '-', $user->name . '-avatar.png');
-        $image = $this->saveNewFromUrl($userAvatarUrl, 'user', $imageName);
-        $image->created_by = $user->id;
-        $image->updated_by = $user->id;
-        $image->uploaded_to = $user->id;
-        $image->save();
-
-        return $image;
-    }
-
-    /**
-     * Check if fetching external avatars is enabled.
-     */
-    public function avatarFetchEnabled(): bool
-    {
-        $fetchUrl = $this->getAvatarUrl();
-        return is_string($fetchUrl) && strpos($fetchUrl, 'http') === 0;
-    }
-
-    /**
-     * Get the URL to fetch avatars from.
-     * @return string|mixed
-     */
-    protected function getAvatarUrl()
-    {
-        $url = trim(config('services.avatar_url'));
-
-        if (empty($url) && !config('services.disable_services')) {
-            $url = 'https://www.gravatar.com/avatar/${hash}?s=${size}&d=identicon';
-        }
-
-        return $url;
-    }
-
     /**
      * Delete gallery and drawings that are not within HTML content of pages or page revisions.
      * Checks based off of only the image name.

app/Uploads/UserAvatars.php

@@ -0,0 +1,100 @@
+<?php namespace BookStack\Uploads;
+
+use BookStack\Auth\User;
+use BookStack\Exceptions\HttpFetchException;
+use Exception;
+
+class UserAvatars
+{
+    protected $imageService;
+    protected $http;
+
+    public function __construct(ImageService $imageService, HttpFetcher $http)
+    {
+        $this->imageService = $imageService;
+        $this->http = $http;
+    }
+
+    /**
+     * Fetch and assign an avatar image to the given user.
+     */
+    public function fetchAndAssignToUser(User $user): void
+    {
+        if (!$this->avatarFetchEnabled()) {
+            return;
+        }
+
+        try {
+            $avatar = $this->saveAvatarImage($user);
+            $user->avatar()->associate($avatar);
+            $user->save();
+        } catch (Exception $e) {
+            Log::error('Failed to save user avatar image');
+        }
+    }
+
+    /**
+     * Save an avatar image from an external service.
+     * @throws Exception
+     */
+    protected function saveAvatarImage(User $user, int $size = 500): Image
+    {
+        $avatarUrl = $this->getAvatarUrl();
+        $email = strtolower(trim($user->email));
+
+        $replacements = [
+            '${hash}' => md5($email),
+            '${size}' => $size,
+            '${email}' => urlencode($email),
+        ];
+
+        $userAvatarUrl = strtr($avatarUrl, $replacements);
+        $imageName = str_replace(' ', '-', $user->id . '-avatar.png');
+        $imageData = $this->getAvatarImageData($userAvatarUrl);
+
+        $image = $this->imageService->saveNew($imageName, $imageData, 'user', $user->id);
+        $image->created_by = $user->id;
+        $image->updated_by = $user->id;
+        $image->save();
+
+        return $image;
+    }
+
+    /**
+     * Gets an image from url and returns it as a string of image data.
+     * @throws Exception
+     */
+    protected function getAvatarImageData(string $url): string
+    {
+        try {
+            $imageData = $this->http->fetch($url);
+        } catch (HttpFetchException $exception) {
+            throw new Exception(trans('errors.cannot_get_image_from_url', ['url' => $url]));
+        }
+        return $imageData;
+    }
+
+    /**
+     * Check if fetching external avatars is enabled.
+     */
+    protected function avatarFetchEnabled(): bool
+    {
+        $fetchUrl = $this->getAvatarUrl();
+        return is_string($fetchUrl) && strpos($fetchUrl, 'http') === 0;
+    }
+
+    /**
+     * Get the URL to fetch avatars from.
+     */
+    protected function getAvatarUrl(): string
+    {
+        $url = trim(config('services.avatar_url'));
+
+        if (empty($url) && !config('services.disable_services')) {
+            $url = 'https://www.gravatar.com/avatar/${hash}?s=${size}&d=identicon';
+        }
+
+        return $url;
+    }
+
+}

resources/views/books/sort-box.blade.php

@@ -13,8 +13,8 @@
     <ul class="sortable-page-list sort-list">
 
         @foreach($bookChildren as $bookChild)
-            <li class="text-{{ $bookChild->getClassName() }}"
+            <li class="text-{{ $bookChild->getType() }}"
-                data-id="{{$bookChild->id}}" data-type="{{ $bookChild->getClassName() }}"
+                data-id="{{$bookChild->id}}" data-type="{{ $bookChild->getType() }}"
                 data-name="{{ $bookChild->name }}" data-created="{{ $bookChild->created_at->timestamp }}"
                 data-updated="{{ $bookChild->updated_at->timestamp }}">
                 <div class="entity-list-item">

resources/views/partials/book-tree.blade.php

@@ -12,7 +12,7 @@
         @endif
 
         @foreach($sidebarTree as $bookChild)
-            <li class="list-item-{{ $bookChild->getClassName() }} {{ $bookChild->getClassName() }} {{ $bookChild->isA('page') && $bookChild->draft ? 'draft' : '' }}">
+            <li class="list-item-{{ $bookChild->getType() }} {{ $bookChild->getType() }} {{ $bookChild->isA('page') && $bookChild->draft ? 'draft' : '' }}">
                 @include('partials.entity-list-item-basic', ['entity' => $bookChild, 'classes' => $current->matches($bookChild)? 'selected' : ''])
 
                 @if($bookChild->isA('chapter') && count($bookChild->pages) > 0)