From c9347aafc1eee27b3b7f1a4bbcaeefb2f60df40d Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 6 Apr 2026 14:41:41 +0100 Subject: [PATCH 1/3] Maintenance: Started work for PHPStan Level 4 --- .gitignore | 1 + app/Access/EmailConfirmationService.php | 2 ++ app/Access/LoginService.php | 2 +- app/Access/Mfa/MfaValue.php | 5 ++- app/Access/Oidc/OidcJwtSigningKey.php | 17 ++++------ app/Access/Oidc/OidcJwtWithClaims.php | 4 +-- app/Access/Oidc/OidcUserDetails.php | 2 +- app/Access/Saml2Service.php | 4 +-- app/Access/SocialAuthService.php | 4 +-- .../Notifications/NotificationManager.php | 14 ++++---- app/Api/ApiDocsGenerator.php | 10 ++++-- app/Api/ApiEntityListFormatter.php | 2 +- app/Api/ApiTokenGuard.php | 33 +++++-------------- app/Entities/Models/Page.php | 2 +- phpstan.neon.dist | 4 +-- 15 files changed, 47 insertions(+), 59 deletions(-) diff --git a/.gitignore b/.gitignore index b545d161f13..06a8723c5c1 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ /node_modules /.vscode /composer +/composer.phar /coverage Homestead.yaml .env diff --git a/app/Access/EmailConfirmationService.php b/app/Access/EmailConfirmationService.php index 1a5156d3e7a..e950c5504b7 100644 --- a/app/Access/EmailConfirmationService.php +++ b/app/Access/EmailConfirmationService.php @@ -5,6 +5,7 @@ use BookStack\Access\Notifications\ConfirmEmailNotification; use BookStack\Exceptions\ConfirmationEmailException; use BookStack\Users\Models\User; +use Exception; class EmailConfirmationService extends UserTokenService { @@ -16,6 +17,7 @@ class EmailConfirmationService extends UserTokenService * Also removes any existing old ones. * * @throws ConfirmationEmailException + * @throws Exception */ public function sendConfirmation(User $user): void { diff --git a/app/Access/LoginService.php b/app/Access/LoginService.php index c81e955722c..0c32b538f28 100644 --- a/app/Access/LoginService.php +++ b/app/Access/LoginService.php @@ -71,7 +71,7 @@ public function reattemptLoginFor(User $user): void } $lastLoginDetails = $this->getLastLoginAttemptDetails(); - $this->login($user, $lastLoginDetails['method'], $lastLoginDetails['remember'] ?? false); + $this->login($user, $lastLoginDetails['method'], $lastLoginDetails['remember']); } /** diff --git a/app/Access/Mfa/MfaValue.php b/app/Access/Mfa/MfaValue.php index dd3e04618f7..b0f08e82684 100644 --- a/app/Access/Mfa/MfaValue.php +++ b/app/Access/Mfa/MfaValue.php @@ -48,17 +48,16 @@ public static function upsertWithValue(User $user, string $method, string $value } /** - * Easily get the decrypted MFA value for the given user and method. + * Get the decrypted MFA value for the given user and method. */ public static function getValueForUser(User $user, string $method): ?string { - /** @var MfaValue $mfaVal */ $mfaVal = static::query() ->where('user_id', '=', $user->id) ->where('method', '=', $method) ->first(); - return $mfaVal ? $mfaVal->getValue() : null; + return $mfaVal?->getValue(); } /** diff --git a/app/Access/Oidc/OidcJwtSigningKey.php b/app/Access/Oidc/OidcJwtSigningKey.php index 3dab3e44275..3edba12b36f 100644 --- a/app/Access/Oidc/OidcJwtSigningKey.php +++ b/app/Access/Oidc/OidcJwtSigningKey.php @@ -9,10 +9,7 @@ class OidcJwtSigningKey { - /** - * @var PublicKey - */ - protected $key; + protected PublicKey $key; /** * Can be created either from a JWK parameter array or local file path to load a certificate from. @@ -20,15 +17,13 @@ class OidcJwtSigningKey * 'file:///var/www/cert.pem' * ['kty' => 'RSA', 'alg' => 'RS256', 'n' => 'abc123...']. * - * @param array|string $jwkOrKeyPath - * * @throws OidcInvalidKeyException */ - public function __construct($jwkOrKeyPath) + public function __construct(array|string $jwkOrKeyPath) { if (is_array($jwkOrKeyPath)) { $this->loadFromJwkArray($jwkOrKeyPath); - } elseif (is_string($jwkOrKeyPath) && strpos($jwkOrKeyPath, 'file://') === 0) { + } elseif (str_starts_with($jwkOrKeyPath, 'file://')) { $this->loadFromPath($jwkOrKeyPath); } else { throw new OidcInvalidKeyException('Unexpected type of key value provided'); @@ -38,7 +33,7 @@ public function __construct($jwkOrKeyPath) /** * @throws OidcInvalidKeyException */ - protected function loadFromPath(string $path) + protected function loadFromPath(string $path): void { try { $key = PublicKeyLoader::load( @@ -58,7 +53,7 @@ protected function loadFromPath(string $path) /** * @throws OidcInvalidKeyException */ - protected function loadFromJwkArray(array $jwk) + protected function loadFromJwkArray(array $jwk): void { // 'alg' is optional for a JWK, but we will still attempt to validate if // it exists otherwise presume it will be compatible. @@ -82,7 +77,7 @@ protected function loadFromJwkArray(array $jwk) throw new OidcInvalidKeyException('A "n" parameter on the provided key is expected'); } - $n = strtr($jwk['n'] ?? '', '-_', '+/'); + $n = strtr($jwk['n'], '-_', '+/'); try { $key = PublicKeyLoader::load([ diff --git a/app/Access/Oidc/OidcJwtWithClaims.php b/app/Access/Oidc/OidcJwtWithClaims.php index 06c04d81eb7..9d7eeead1a9 100644 --- a/app/Access/Oidc/OidcJwtWithClaims.php +++ b/app/Access/Oidc/OidcJwtWithClaims.php @@ -102,12 +102,12 @@ public function replaceClaims(array $claims): void protected function validateTokenStructure(): void { foreach (['header', 'payload'] as $prop) { - if (empty($this->$prop) || !is_array($this->$prop)) { + if (empty($this->$prop)) { throw new OidcInvalidTokenException("Could not parse out a valid {$prop} within the provided token"); } } - if (empty($this->signature) || !is_string($this->signature)) { + if (empty($this->signature)) { throw new OidcInvalidTokenException('Could not parse out a valid signature within the provided token'); } } diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php index 7a422a58de2..b1736f97d31 100644 --- a/app/Access/Oidc/OidcUserDetails.php +++ b/app/Access/Oidc/OidcUserDetails.php @@ -39,7 +39,7 @@ public function populate( ): void { $this->externalId = $claims->getClaim($idClaim) ?? $this->externalId; $this->email = $claims->getClaim('email') ?? $this->email; - $this->name = static::getUserDisplayName($displayNameClaims, $claims) ?? $this->name; + $this->name = static::getUserDisplayName($displayNameClaims, $claims) ?: $this->name; $this->groups = static::getUserGroups($groupsClaim, $claims) ?? $this->groups; $this->picture = static::getPicture($claims) ?: $this->picture; } diff --git a/app/Access/Saml2Service.php b/app/Access/Saml2Service.php index 106a7a22906..5572d210401 100644 --- a/app/Access/Saml2Service.php +++ b/app/Access/Saml2Service.php @@ -266,7 +266,7 @@ protected function getExternalId(array $samlAttributes, string $defaultValue) /** * Extract the details of a user from a SAML response. * - * @return array{external_id: string, name: string, email: string, saml_id: string} + * @return array{external_id: string, name: string, email: string|null, saml_id: string} */ protected function getUserDetails(string $samlID, $samlAttributes): array { @@ -357,7 +357,7 @@ public function processLoginCallback(string $samlID, array $samlAttributes): Use ]); } - if ($userDetails['email'] === null) { + if (empty($userDetails['email'])) { throw new SamlException(trans('errors.saml_no_email_address')); } diff --git a/app/Access/SocialAuthService.php b/app/Access/SocialAuthService.php index c3c20587db3..bdcfb45c865 100644 --- a/app/Access/SocialAuthService.php +++ b/app/Access/SocialAuthService.php @@ -117,14 +117,14 @@ public function handleLoginCallback(string $socialDriver, SocialUser $socialUser } // When a user is logged in and the social account exists and is already linked to the current user. - if ($isLoggedIn && $socialAccount !== null && $socialAccount->user->id === $currentUser->id) { + if ($isLoggedIn && $socialAccount->user->id === $currentUser->id) { session()->flash('error', trans('errors.social_account_existing', ['socialAccount' => $titleCaseDriver])); return redirect('/my-account/auth#social_accounts'); } // When a user is logged in, A social account exists but the users do not match. - if ($isLoggedIn && $socialAccount !== null && $socialAccount->user->id != $currentUser->id) { + if ($isLoggedIn && $socialAccount->user->id != $currentUser->id) { session()->flash('error', trans('errors.social_account_already_used_existing', ['socialAccount' => $titleCaseDriver])); return redirect('/my-account/auth#social_accounts'); diff --git a/app/Activity/Notifications/NotificationManager.php b/app/Activity/Notifications/NotificationManager.php index 8a6c26ffbed..38da2c552a5 100644 --- a/app/Activity/Notifications/NotificationManager.php +++ b/app/Activity/Notifications/NotificationManager.php @@ -15,14 +15,14 @@ class NotificationManager { /** - * @var class-string[] + * @var array[]> */ - protected array $handlers = []; + protected array $handlersByActivity = []; public function handle(Activity $activity, string|Loggable $detail, User $user): void { $activityType = $activity->type; - $handlersToRun = $this->handlers[$activityType] ?? []; + $handlersToRun = $this->handlersByActivity[$activityType] ?? []; foreach ($handlersToRun as $handlerClass) { /** @var NotificationHandler $handler */ $handler = new $handlerClass(); @@ -35,12 +35,12 @@ public function handle(Activity $activity, string|Loggable $detail, User $user): */ public function registerHandler(string $activityType, string $handlerClass): void { - if (!isset($this->handlers[$activityType])) { - $this->handlers[$activityType] = []; + if (!isset($this->handlersByActivity[$activityType])) { + $this->handlersByActivity[$activityType] = []; } - if (!in_array($handlerClass, $this->handlers[$activityType])) { - $this->handlers[$activityType][] = $handlerClass; + if (!in_array($handlerClass, $this->handlersByActivity[$activityType])) { + $this->handlersByActivity[$activityType][] = $handlerClass; } } diff --git a/app/Api/ApiDocsGenerator.php b/app/Api/ApiDocsGenerator.php index eb8f5508c70..a59cb8198e2 100644 --- a/app/Api/ApiDocsGenerator.php +++ b/app/Api/ApiDocsGenerator.php @@ -17,7 +17,14 @@ class ApiDocsGenerator { + /** + * @var array + */ protected array $reflectionClasses = []; + + /** + * @var array + */ protected array $controllerClasses = []; /** @@ -107,7 +114,6 @@ protected function loadDetailsFromControllers(Collection $routes): Collection */ protected function getBodyParamsFromClass(string $className, string $methodName): ?array { - /** @var ApiController $class */ $class = $this->controllerClasses[$className] ?? null; if ($class === null) { $class = app()->make($className); @@ -153,7 +159,7 @@ protected function parseDescriptionFromDocBlockComment(string $comment): string $matches = []; preg_match_all('/^\s*?\*\s?($|((?![\/@\s]).*?))$/m', $comment, $matches); - $text = implode(' ', $matches[1] ?? []); + $text = implode(' ', $matches[1]); return str_replace(' ', "\n", $text); } diff --git a/app/Api/ApiEntityListFormatter.php b/app/Api/ApiEntityListFormatter.php index 3c94d96ee60..c5a378269a8 100644 --- a/app/Api/ApiEntityListFormatter.php +++ b/app/Api/ApiEntityListFormatter.php @@ -78,7 +78,7 @@ public function withTags(): self public function withParents(): self { $this->withField('book', function (Entity $entity) { - if ($entity instanceof BookChild && $entity->book) { + if ($entity instanceof BookChild) { return $entity->book->only(['id', 'name', 'slug']); } return null; diff --git a/app/Api/ApiTokenGuard.php b/app/Api/ApiTokenGuard.php index 9f4537b296b..f1a3f0dc883 100644 --- a/app/Api/ApiTokenGuard.php +++ b/app/Api/ApiTokenGuard.php @@ -16,30 +16,15 @@ class ApiTokenGuard implements Guard { use GuardHelpers; - /** - * The request instance. - */ - protected $request; - - /** - * @var LoginService - */ - protected $loginService; - /** * The last auth exception thrown in this request. - * - * @var ApiAuthException */ - protected $lastAuthException; + protected ApiAuthException|null $lastAuthException = null; - /** - * ApiTokenGuard constructor. - */ - public function __construct(Request $request, LoginService $loginService) - { - $this->request = $request; - $this->loginService = $loginService; + public function __construct( + protected Request $request, + protected LoginService $loginService + ) { } /** @@ -67,7 +52,7 @@ public function user() } /** - * Determine if current user is authenticated. If not, throw an exception. + * Determine if the current user is authenticated. If not, throw an exception. * * @throws ApiAuthException * @@ -121,7 +106,7 @@ protected function validateTokenHeaderValue(string $authToken): void throw new ApiAuthException(trans('errors.api_no_authorization_found')); } - if (strpos($authToken, ':') === false || strpos($authToken, 'Token ') !== 0) { + if (!str_contains($authToken, ':') || !str_starts_with($authToken, 'Token ')) { throw new ApiAuthException(trans('errors.api_bad_authorization_format')); } } @@ -155,7 +140,7 @@ protected function validateToken(?ApiToken $token, string $secret): void /** * {@inheritdoc} */ - public function validate(array $credentials = []) + public function validate(array $credentials = []): bool { if (empty($credentials['id']) || empty($credentials['secret'])) { return false; @@ -175,7 +160,7 @@ public function validate(array $credentials = []) /** * "Log out" the currently authenticated user. */ - public function logout() + public function logout(): void { $this->user = null; } diff --git a/app/Entities/Models/Page.php b/app/Entities/Models/Page.php index a1d3fc7b40d..d3a392da6fa 100644 --- a/app/Entities/Models/Page.php +++ b/app/Entities/Models/Page.php @@ -23,7 +23,7 @@ * @property bool $draft * @property int $revision_count * @property string $editor - * @property Chapter $chapter + * @property Chapter|null $chapter * @property Collection $attachments * @property Collection $revisions * @property PageRevision $currentRevision diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 72189222fcf..bab28ea0eb3 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -7,11 +7,11 @@ parameters: - app # The level 8 is the highest level - level: 3 + level: 4 phpVersion: min: 80200 - max: 80400 + max: 80500 bootstrapFiles: - bootstrap/phpstan.php From 3b0abcfc39f218b438150afae7b9c6fc3cb9db12 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 6 Apr 2026 21:38:18 +0100 Subject: [PATCH 2/3] Maintenance: Continued work for phpstan level 4 --- app/Console/Commands/AssignSortRuleCommand.php | 2 +- .../Commands/CopyShelfPermissionsCommand.php | 17 ++++++++++------- app/Entities/Models/Entity.php | 1 + app/Entities/Repos/PageRepo.php | 2 +- app/Http/Controller.php | 2 +- app/Search/SearchOptions.php | 12 +++++------- app/Sorting/BookSorter.php | 3 +-- app/Uploads/ImageRepo.php | 2 +- app/Uploads/UserAvatars.php | 2 +- .../CopyShelfPermissionsCommandTest.php | 18 ++++++++++++++++++ 10 files changed, 40 insertions(+), 21 deletions(-) diff --git a/app/Console/Commands/AssignSortRuleCommand.php b/app/Console/Commands/AssignSortRuleCommand.php index c438d078326..f00df83831c 100644 --- a/app/Console/Commands/AssignSortRuleCommand.php +++ b/app/Console/Commands/AssignSortRuleCommand.php @@ -32,7 +32,7 @@ class AssignSortRuleCommand extends Command */ public function handle(BookSorter $sorter): int { - $sortRuleId = intval($this->argument('sort-rule')) ?? 0; + $sortRuleId = intval($this->argument('sort-rule')); if ($sortRuleId === 0) { return $this->listSortRules(); } diff --git a/app/Console/Commands/CopyShelfPermissionsCommand.php b/app/Console/Commands/CopyShelfPermissionsCommand.php index c5e2d504e75..1207621debc 100644 --- a/app/Console/Commands/CopyShelfPermissionsCommand.php +++ b/app/Console/Commands/CopyShelfPermissionsCommand.php @@ -32,6 +32,7 @@ public function handle(PermissionsUpdater $permissionsUpdater, BookshelfQueries { $shelfSlug = $this->option('slug'); $cascadeAll = $this->option('all'); + $noInteraction = boolval($this->option('no-interaction')); $shelves = null; if (!$cascadeAll && !$shelfSlug) { @@ -41,14 +42,16 @@ public function handle(PermissionsUpdater $permissionsUpdater, BookshelfQueries } if ($cascadeAll) { - $continue = $this->confirm( - 'Permission settings for all shelves will be cascaded. ' . - 'Books assigned to multiple shelves will receive only the permissions of it\'s last processed shelf. ' . - 'Are you sure you want to proceed?' - ); + if (!$noInteraction) { + $continue = $this->confirm( + 'Permission settings for all shelves will be cascaded. ' . + 'Books assigned to multiple shelves will receive only the permissions of it\'s last processed shelf. ' . + 'Are you sure you want to proceed?', + ); - if (!$continue && !$this->hasOption('no-interaction')) { - return 0; + if (!$continue) { + return 0; + } } $shelves = $queries->start()->get(['id']); diff --git a/app/Entities/Models/Entity.php b/app/Entities/Models/Entity.php index 47e13462691..27cfccaa836 100644 --- a/app/Entities/Models/Entity.php +++ b/app/Entities/Models/Entity.php @@ -479,6 +479,7 @@ public static function instanceFromType(string $type): self 'chapter' => new Chapter(), 'book' => new Book(), 'bookshelf' => new Bookshelf(), + default => throw new \InvalidArgumentException("Invalid entity type: {$type}"), }; } } diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php index bc590785d93..375bf1d2bc1 100644 --- a/app/Entities/Repos/PageRepo.php +++ b/app/Entities/Repos/PageRepo.php @@ -60,7 +60,7 @@ public function getNewDraftPage(Entity $parent): Page $page->book_id = $parent->id; } - $defaultTemplate = $page->chapter?->defaultTemplate()->get() ?? $page->book?->defaultTemplate()->get(); + $defaultTemplate = $page->chapter?->defaultTemplate()->get() ?? $page->book->defaultTemplate()->get(); if ($defaultTemplate) { $page->forceFill([ 'html' => $defaultTemplate->html, diff --git a/app/Http/Controller.php b/app/Http/Controller.php index 1a0f5932e6f..796505795e5 100644 --- a/app/Http/Controller.php +++ b/app/Http/Controller.php @@ -62,7 +62,7 @@ protected function showPermissionError(string $redirectLocation = '/'): never */ protected function checkPermission(string|Permission $permission): void { - if (!user() || !user()->can($permission)) { + if (!user()->can($permission)) { $this->showPermissionError(); } } diff --git a/app/Search/SearchOptions.php b/app/Search/SearchOptions.php index 83af2d043d8..cfd068386ef 100644 --- a/app/Search/SearchOptions.php +++ b/app/Search/SearchOptions.php @@ -121,13 +121,11 @@ protected function addOptionsFromString(string $searchString): void foreach ($patterns as $termType => $pattern) { $matches = []; preg_match_all($pattern, $searchString, $matches); - if (count($matches) > 0) { - foreach ($matches[1] as $index => $value) { - $negated = str_starts_with($matches[0][$index], '-'); - $terms[$termType][] = $constructors[$termType]($value, $negated); - } - $searchString = preg_replace($pattern, '', $searchString); + foreach ($matches[1] as $index => $value) { + $negated = str_starts_with($matches[0][$index], '-'); + $terms[$termType][] = $constructors[$termType]($value, $negated); } + $searchString = preg_replace($pattern, '', $searchString); } // Unescape exacts and backslash escapes @@ -261,7 +259,7 @@ public function getAdditionalOptionsString(): string $userFilters = ['updated_by', 'created_by', 'owned_by']; $unsupportedFilters = ['is_template', 'sort_by']; foreach ($this->filters->all() as $filter) { - if (in_array($filter->getKey(), $userFilters, true) && $filter->value !== null && $filter->value !== 'me') { + if (in_array($filter->getKey(), $userFilters, true) && $filter->value && $filter->value !== 'me') { $options[] = $filter; } else if (in_array($filter->getKey(), $unsupportedFilters, true)) { $options[] = $filter; diff --git a/app/Sorting/BookSorter.php b/app/Sorting/BookSorter.php index b4f93d47b11..0862aaa8877 100644 --- a/app/Sorting/BookSorter.php +++ b/app/Sorting/BookSorter.php @@ -125,9 +125,8 @@ public function sortUsingMap(BookSortMap $sortMap): array */ protected function applySortUpdates(BookSortMapItem $sortMapItem, array $modelMap): void { - /** @var BookChild $model */ $model = $modelMap[$sortMapItem->type . ':' . $sortMapItem->id] ?? null; - if (!$model) { + if (!($model instanceof BookChild)) { return; } diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index a16b87bd75b..e87e22b3a3c 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -91,7 +91,7 @@ public function getEntityFiltered( $parentFilter = function (Builder $query) use ($filterType, $contextPage) { if ($filterType === 'page') { $query->where('uploaded_to', '=', $contextPage->id); - } else if ($filterType === 'book') { + } else { $validPageIds = $contextPage->book->pages() ->scopes('visible') ->pluck('id') diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index 0cc640f225c..8fcf6358008 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -148,7 +148,7 @@ protected function getAvatarImageData(string $url): string $responseCount++; $isRedirect = ($response->getStatusCode() === 301 || $response->getStatusCode() === 302); $url = $response->getHeader('Location')[0] ?? ''; - } while ($responseCount < 3 && $isRedirect && is_string($url) && str_starts_with($url, 'http')); + } while ($responseCount < 3 && $isRedirect && str_starts_with($url, 'http')); if ($responseCount === 3) { throw new HttpFetchException("Failed to fetch image, max redirect limit of 3 tries reached. Last fetched URL: {$url}"); diff --git a/tests/Commands/CopyShelfPermissionsCommandTest.php b/tests/Commands/CopyShelfPermissionsCommandTest.php index 5c21a2e341c..d5f9677a229 100644 --- a/tests/Commands/CopyShelfPermissionsCommandTest.php +++ b/tests/Commands/CopyShelfPermissionsCommandTest.php @@ -2,6 +2,7 @@ namespace Tests\Commands; +use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; use Tests\TestCase; @@ -61,4 +62,21 @@ public function test_copy_shelf_permissions_command_using_all() 'view' => true, 'update' => true, 'create' => false, 'delete' => false, ]); } + + public function test_copy_shelf_permissions_command_using_slug_without_interaction() + { + $shelf = $this->entities->shelfHasBooks(); + $editorRole = $this->users->editor()->roles()->first(); + /** @var Book $child */ + $child = $shelf->books()->first(); + $child->shelves()->where('id', '!=', $shelf->id)->delete(); + + $this->assertFalse($child->hasPermissions()); + + $this->permissions->setEntityPermissions($shelf, ['view', 'update'], [$editorRole]); + $this->artisan('bookstack:copy-shelf-permissions --all --no-interaction'); + + $child->refresh(); + $this->assertTrue($child->hasPermissions(), 'Child book should now be restricted'); + } } From 58e2e0ea241e4059e202084fec40d7668613e774 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 8 Apr 2026 16:39:14 +0100 Subject: [PATCH 3/3] Maintenance: Finished getting to PHPStan level 4 --- app/Api/ApiEntityListFormatter.php | 7 +++++-- app/Entities/Models/Book.php | 2 +- app/Entities/Tools/PageContent.php | 2 +- app/Entities/Tools/PermissionsUpdater.php | 2 +- app/Exports/ExportFormatter.php | 4 ++-- app/Exports/ZipExports/ZipImportRunner.php | 14 ++++++-------- app/Exports/ZipExports/ZipReferenceParser.php | 4 ---- app/Permissions/JointPermissionBuilder.php | 3 +-- app/Users/Models/User.php | 3 +-- tests/Api/SearchApiTest.php | 1 + 10 files changed, 19 insertions(+), 23 deletions(-) diff --git a/app/Api/ApiEntityListFormatter.php b/app/Api/ApiEntityListFormatter.php index c5a378269a8..23073bfc2fd 100644 --- a/app/Api/ApiEntityListFormatter.php +++ b/app/Api/ApiEntityListFormatter.php @@ -74,18 +74,21 @@ public function withTags(): self /** * Include parent book/chapter info in the formatted data. + * These functions are careful to not load the relation themselves, since they should + * have already been loaded in a more efficient manner, with permissions applied, by the time + * the parent fields are handled here. */ public function withParents(): self { $this->withField('book', function (Entity $entity) { - if ($entity instanceof BookChild) { + if ($entity instanceof BookChild && $entity->relationLoaded('book') && $entity->getRelationValue('book')) { return $entity->book->only(['id', 'name', 'slug']); } return null; }); $this->withField('chapter', function (Entity $entity) { - if ($entity instanceof Page && $entity->chapter) { + if ($entity instanceof Page && $entity->relationLoaded('chapter') && $entity->getRelationValue('chapter')) { return $entity->chapter->only(['id', 'name', 'slug']); } return null; diff --git a/app/Entities/Models/Book.php b/app/Entities/Models/Book.php index 1909dbd5631..10f04695a5e 100644 --- a/app/Entities/Models/Book.php +++ b/app/Entities/Models/Book.php @@ -17,7 +17,7 @@ * * @property string $description * @property string $description_html - * @property int $image_id + * @property ?int $image_id * @property ?int $default_template_id * @property ?int $sort_rule_id * @property \Illuminate\Database\Eloquent\Collection $chapters diff --git a/app/Entities/Tools/PageContent.php b/app/Entities/Tools/PageContent.php index 8d89a86cff4..6a764990079 100644 --- a/app/Entities/Tools/PageContent.php +++ b/app/Entities/Tools/PageContent.php @@ -359,7 +359,7 @@ protected function getContentCacheKey(string $html): string { $contentHash = md5($html); $contentId = $this->page->id; - $contentTime = $this->page->updated_at?->timestamp ?? time(); + $contentTime = $this->page->updated_at->timestamp ?? time(); $appVersion = AppVersion::get(); $filterConfig = config('app.content_filtering') ?? ''; return "page-content-cache::{$filterConfig}::{$appVersion}::{$contentId}::{$contentTime}::{$contentHash}"; diff --git a/app/Entities/Tools/PermissionsUpdater.php b/app/Entities/Tools/PermissionsUpdater.php index fa9ae753c51..f3165b603e5 100644 --- a/app/Entities/Tools/PermissionsUpdater.php +++ b/app/Entities/Tools/PermissionsUpdater.php @@ -47,7 +47,7 @@ public function updateFromApiRequestData(Entity $entity, array $data): void { if (isset($data['role_permissions'])) { $entity->permissions()->where('role_id', '!=', 0)->delete(); - $rolePermissionData = $this->formatPermissionsFromApiRequestToEntityPermissions($data['role_permissions'] ?? [], false); + $rolePermissionData = $this->formatPermissionsFromApiRequestToEntityPermissions($data['role_permissions'], false); $entity->permissions()->createMany($rolePermissionData); } diff --git a/app/Exports/ExportFormatter.php b/app/Exports/ExportFormatter.php index c5973eace29..6bf0a05add9 100644 --- a/app/Exports/ExportFormatter.php +++ b/app/Exports/ExportFormatter.php @@ -208,7 +208,7 @@ protected function containHtml(string $htmlContent): string preg_match_all("/\/i", $htmlContent, $imageTagsOutput); // Replace image src with base64 encoded image strings - if (isset($imageTagsOutput[0]) && count($imageTagsOutput[0]) > 0) { + if (count($imageTagsOutput[0]) > 0) { foreach ($imageTagsOutput[0] as $index => $imgMatch) { $oldImgTagString = $imgMatch; $srcString = $imageTagsOutput[2][$index]; @@ -225,7 +225,7 @@ protected function containHtml(string $htmlContent): string preg_match_all("/\/i", $htmlContent, $linksOutput); // Update relative links to be absolute, with instance url - if (isset($linksOutput[0]) && count($linksOutput[0]) > 0) { + if (count($linksOutput[0]) > 0) { foreach ($linksOutput[0] as $index => $linkMatch) { $oldLinkString = $linkMatch; $srcString = $linksOutput[2][$index]; diff --git a/app/Exports/ZipExports/ZipImportRunner.php b/app/Exports/ZipExports/ZipImportRunner.php index 382e4073eec..9fa7dec3afe 100644 --- a/app/Exports/ZipExports/ZipImportRunner.php +++ b/app/Exports/ZipExports/ZipImportRunner.php @@ -82,10 +82,8 @@ public function run(Import $import, ?Entity $parent = null): Entity $entity = $this->importBook($exportModel, $reader); } else if ($exportModel instanceof ZipExportChapter) { $entity = $this->importChapter($exportModel, $parent, $reader); - } else if ($exportModel instanceof ZipExportPage) { - $entity = $this->importPage($exportModel, $parent, $reader); } else { - throw new ZipImportException(['No importable data found in import data.']); + $entity = $this->importPage($exportModel, $parent, $reader); } $this->references->replaceReferences(); @@ -132,7 +130,7 @@ protected function importBook(ZipExportBook $exportBook, ZipExportReader $reader 'name' => $exportBook->name, 'description_html' => $exportBook->description_html ?? '', 'image' => $exportBook->cover ? $this->zipFileToUploadedFile($exportBook->cover, $reader) : null, - 'tags' => $this->exportTagsToInputArray($exportBook->tags ?? []), + 'tags' => $this->exportTagsToInputArray($exportBook->tags), ]); if ($book->coverInfo()->getImage()) { @@ -151,7 +149,7 @@ protected function importBook(ZipExportBook $exportBook, ZipExportReader $reader foreach ($children as $child) { if ($child instanceof ZipExportChapter) { $this->importChapter($child, $book, $reader); - } else if ($child instanceof ZipExportPage) { + } else { $this->importPage($child, $book, $reader); } } @@ -166,7 +164,7 @@ protected function importChapter(ZipExportChapter $exportChapter, Book $parent, $chapter = $this->chapterRepo->create([ 'name' => $exportChapter->name, 'description_html' => $exportChapter->description_html ?? '', - 'tags' => $this->exportTagsToInputArray($exportChapter->tags ?? []), + 'tags' => $this->exportTagsToInputArray($exportChapter->tags), ], $parent); $exportPages = $exportChapter->pages; @@ -199,7 +197,7 @@ protected function importPage(ZipExportPage $exportPage, Book|Chapter $parent, Z 'name' => $exportPage->name, 'markdown' => $exportPage->markdown ?? '', 'html' => $exportPage->html ?? '', - 'tags' => $this->exportTagsToInputArray($exportPage->tags ?? []), + 'tags' => $this->exportTagsToInputArray($exportPage->tags), ]); $this->references->addPage($page, $exportPage); @@ -302,7 +300,7 @@ protected function ensurePermissionsPermitImport(ZipExportPage|ZipExportChapter| array_push($chapters, ...$exportModel->chapters); } else if ($exportModel instanceof ZipExportChapter) { $chapters[] = $exportModel; - } else if ($exportModel instanceof ZipExportPage) { + } else { $pages[] = $exportModel; } diff --git a/app/Exports/ZipExports/ZipReferenceParser.php b/app/Exports/ZipExports/ZipReferenceParser.php index a6560e3f289..9bb069ab7f1 100644 --- a/app/Exports/ZipExports/ZipReferenceParser.php +++ b/app/Exports/ZipExports/ZipReferenceParser.php @@ -68,10 +68,6 @@ public function parseReferences(string $content, callable $handler): string $matches = []; preg_match_all($referenceRegex, $content, $matches); - if (count($matches) < 3) { - return $content; - } - for ($i = 0; $i < count($matches[0]); $i++) { $referenceText = $matches[0][$i]; $type = strtolower($matches[1][$i]); diff --git a/app/Permissions/JointPermissionBuilder.php b/app/Permissions/JointPermissionBuilder.php index 56b22ad1604..94f18916d4a 100644 --- a/app/Permissions/JointPermissionBuilder.php +++ b/app/Permissions/JointPermissionBuilder.php @@ -61,8 +61,7 @@ public function rebuildForEntity(Entity $entity): void return; } - /** @var BookChild $entity */ - if ($entity->book) { + if ($entity instanceof BookChild) { $entities[] = $entity->book; } diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 50efdcdad60..b9289a5a7b5 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -222,8 +222,7 @@ public function hasSocialAccount(string $socialDriver = ''): bool public function getAvatar(int $size = 50): string { $default = url('/user_avatar.png'); - $imageId = $this->image_id; - if ($imageId === 0 || $imageId === '0' || $imageId === null) { + if ($this->image_id === 0) { return $default; } diff --git a/tests/Api/SearchApiTest.php b/tests/Api/SearchApiTest.php index 517c5d8e4ef..5d0ce53ec1c 100644 --- a/tests/Api/SearchApiTest.php +++ b/tests/Api/SearchApiTest.php @@ -106,6 +106,7 @@ public function test_all_endpoint_includes_parent_details_where_visible() $this->permissions->setEntityPermissions($page, ['view'], [$editor->roles()->first()]); $resp = $this->getJson($this->baseEndpoint . '?query=superextrauniquevalue'); + $resp->assertOk(); $resp->assertJsonPath('data.0.id', $page->id); $resp->assertJsonPath('data.0.book.name', $book->name); $resp->assertJsonMissingPath('data.0.chapter');