From 88e148ba0046f96a16b57d978b1f2cc89e6a4f20 Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Thu, 10 Aug 2023 15:44:27 +0200 Subject: [PATCH 01/97] Initial Draft --- app/App/HomeController.php | 150 ++++++++++++++++++------- resources/views/layouts/base.blade.php | 4 + routes/web.php | 1 + 3 files changed, 112 insertions(+), 43 deletions(-) diff --git a/app/App/HomeController.php b/app/App/HomeController.php index 667af80d3..2c57da3c1 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -18,41 +18,41 @@ use Illuminate\Http\Request; class HomeController extends Controller { /** - * Display the homepage. - */ + * Display the homepage. + */ public function index(Request $request, ActivityQueries $activities) { $activity = $activities->latest(10); $draftPages = []; - + if ($this->isSignedIn()) { $draftPages = Page::visible() - ->where('draft', '=', true) - ->where('created_by', '=', user()->id) - ->orderBy('updated_at', 'desc') - ->with('book') - ->take(6) - ->get(); + ->where('draft', '=', true) + ->where('created_by', '=', user()->id) + ->orderBy('updated_at', 'desc') + ->with('book') + ->take(6) + ->get(); } - + $recentFactor = count($draftPages) > 0 ? 0.5 : 1; $recents = $this->isSignedIn() ? - (new RecentlyViewed())->run(12 * $recentFactor, 1) - : Book::visible()->orderBy('created_at', 'desc')->take(12 * $recentFactor)->get(); + (new RecentlyViewed())->run(12 * $recentFactor, 1) + : Book::visible()->orderBy('created_at', 'desc')->take(12 * $recentFactor)->get(); $favourites = (new TopFavourites())->run(6); $recentlyUpdatedPages = Page::visible()->with('book') - ->where('draft', false) - ->orderBy('updated_at', 'desc') - ->take($favourites->count() > 0 ? 5 : 10) - ->select(Page::$listAttributes) - ->get(); - + ->where('draft', false) + ->orderBy('updated_at', 'desc') + ->take($favourites->count() > 0 ? 5 : 10) + ->select(Page::$listAttributes) + ->get(); + $homepageOptions = ['default', 'books', 'bookshelves', 'page']; $homepageOption = setting('app-homepage-type', 'default'); if (!in_array($homepageOption, $homepageOptions)) { $homepageOption = 'default'; } - + $commonData = [ 'activity' => $activity, 'recents' => $recents, @@ -60,7 +60,7 @@ class HomeController extends Controller 'draftPages' => $draftPages, 'favourites' => $favourites, ]; - + // Add required list ordering & sorting for books & shelves views. if ($homepageOption === 'bookshelves' || $homepageOption === 'books') { $key = $homepageOption; @@ -70,27 +70,27 @@ class HomeController extends Controller 'created_at' => trans('common.sort_created_at'), 'updated_at' => trans('common.sort_updated_at'), ]); - + $commonData = array_merge($commonData, [ 'view' => $view, 'listOptions' => $listOptions, ]); } - + if ($homepageOption === 'bookshelves') { $shelves = app(BookshelfRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); $data = array_merge($commonData, ['shelves' => $shelves]); - + return view('home.shelves', $data); } - + if ($homepageOption === 'books') { $books = app(BookRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); $data = array_merge($commonData, ['books' => $books]); - + return view('home.books', $data); } - + if ($homepageOption === 'page') { $homepageSetting = setting('app-homepage', '0:'); $id = intval(explode(':', $homepageSetting)[0]); @@ -98,46 +98,110 @@ class HomeController extends Controller $customHomepage = Page::query()->where('draft', '=', false)->findOrFail($id); $pageContent = new PageContent($customHomepage); $customHomepage->html = $pageContent->render(false); - + return view('home.specific-page', array_merge($commonData, ['customHomepage' => $customHomepage])); } - + return view('home.default', $commonData); } - + /** - * Show the view for /robots.txt. - */ + * Show the view for /robots.txt. + */ public function robots() { $sitePublic = setting('app-public', false); $allowRobots = config('app.allow_robots'); - + if ($allowRobots === null) { $allowRobots = $sitePublic; } - + return response() - ->view('misc.robots', ['allowRobots' => $allowRobots]) - ->header('Content-Type', 'text/plain'); + ->view('misc.robots', ['allowRobots' => $allowRobots]) + ->header('Content-Type', 'text/plain'); } - + /** - * Show the route for 404 responses. - */ + * Show the route for 404 responses. + */ public function notFound() { return response()->view('errors.404', [], 404); } - + /** - * Serve the application favicon. - * Ensures a 'favicon.ico' file exists at the web root location (if writable) to be served - * directly by the webserver in the future. - */ + * Serve the application favicon. + * Ensures a 'favicon.ico' file exists at the web root location (if writable) to be served + * directly by the webserver in the future. + */ public function favicon(FaviconHandler $favicons) { $exists = $favicons->restoreOriginalIfNotExists(); return response()->file($exists ? $favicons->getPath() : $favicons->getOriginalPath()); } + + /** + * Serve the application manifest. + * Ensures a 'manifest.json' + */ + public function manifest() + { + $manifest = [ + "name" => config('app.name' | 'BookStack'), + "short_name" => "bookstack", + "start_url" => "/", + "scope" => "/", + "display" => "standalone", + "background_color" => "#fff", + "description" => config('app.name' | 'BookStack'), + "categories" => [ + "productivity", + "lifestyle" + ], + "launch_handler" => [ + "client_mode" => "focus-existing" + ], + "orientation" => "portrait", + "icons" => [ + [ + "src" => "/icon-64.png", + "sizes" => "64x64", + "type" => "image/png" + ], + [ + "src" => "/icon-32.png", + "sizes" => "32x32", + "type" => "image/png" + ], + [ + "src" => "/icon-128.png", + "sizes" => "128x128", + "type" => "image/png" + ], + [ + "src" => "icon-180.png", + "sizes" => "180x180", + "type" => "image/png" + ], + [ + "src" => "icon.png", + "sizes" => "256x256", + "type" => "image/png" + ], + [ + "src" => "icon.ico", + "sizes" => "48x48", + "type" => "image/vnd.microsoft.icon" + ], + [ + "src" => "favicon.ico", + "sizes" => "48x48", + "type" => "image/vnd.microsoft.icon" + ], + ], + ]; + + return response()->json($manifest); + } } diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index e0a6f46d0..4a0422dcd 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -29,6 +29,10 @@ + + + + @yield('head') diff --git a/routes/web.php b/routes/web.php index 74ee74a2c..6e80635e0 100644 --- a/routes/web.php +++ b/routes/web.php @@ -20,6 +20,7 @@ use Illuminate\View\Middleware\ShareErrorsFromSession; Route::get('/status', [SettingControllers\StatusController::class, 'show']); Route::get('/robots.txt', [HomeController::class, 'robots']); Route::get('/favicon.ico', [HomeController::class, 'favicon']); +Route::get('/manifest.json', [HomeController::class, 'manifest']); // Authenticated routes... Route::middleware('auth')->group(function () { From 601491b275a3d7f81007223f52c79af528e337ba Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Thu, 10 Aug 2023 15:51:09 +0200 Subject: [PATCH 02/97] Add Color --- app/App/HomeController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/App/HomeController.php b/app/App/HomeController.php index 2c57da3c1..64d2865ec 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -153,7 +153,7 @@ class HomeController extends Controller "start_url" => "/", "scope" => "/", "display" => "standalone", - "background_color" => "#fff", + "background_color" => setting('app-color'), "description" => config('app.name' | 'BookStack'), "categories" => [ "productivity", From 08ea97fd8346edad23440ffe5c65e1f98467e76a Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Thu, 10 Aug 2023 16:43:14 +0200 Subject: [PATCH 03/97] Manifest Tweaks --- app/App/HomeController.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/App/HomeController.php b/app/App/HomeController.php index 64d2865ec..641b84fa8 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -148,13 +148,13 @@ class HomeController extends Controller public function manifest() { $manifest = [ - "name" => config('app.name' | 'BookStack'), + "name" => (config('app.name' | 'BookStack') ??'BookStack' ), "short_name" => "bookstack", - "start_url" => "/", - "scope" => "/", + "start_url" => "./", + "scope" => ".", "display" => "standalone", "background_color" => setting('app-color'), - "description" => config('app.name' | 'BookStack'), + "description" =>( config('app.name' | 'BookStack') ??'BookStack'), "categories" => [ "productivity", "lifestyle" From 2b604b5af9e7a7973f693f7ef7a5d964c0bcadeb Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Thu, 10 Aug 2023 17:02:31 +0200 Subject: [PATCH 04/97] Move Manifest Definition to Separate Config File --- app/App/HomeController.php | 57 ++------------------------------------ app/Config/manifest.php | 55 ++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 54 deletions(-) create mode 100644 app/Config/manifest.php diff --git a/app/App/HomeController.php b/app/App/HomeController.php index 641b84fa8..d971247df 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -147,61 +147,10 @@ class HomeController extends Controller */ public function manifest() { - $manifest = [ - "name" => (config('app.name' | 'BookStack') ??'BookStack' ), - "short_name" => "bookstack", - "start_url" => "./", - "scope" => ".", - "display" => "standalone", - "background_color" => setting('app-color'), - "description" =>( config('app.name' | 'BookStack') ??'BookStack'), - "categories" => [ - "productivity", - "lifestyle" - ], - "launch_handler" => [ - "client_mode" => "focus-existing" - ], - "orientation" => "portrait", - "icons" => [ - [ - "src" => "/icon-64.png", - "sizes" => "64x64", - "type" => "image/png" - ], - [ - "src" => "/icon-32.png", - "sizes" => "32x32", - "type" => "image/png" - ], - [ - "src" => "/icon-128.png", - "sizes" => "128x128", - "type" => "image/png" - ], - [ - "src" => "icon-180.png", - "sizes" => "180x180", - "type" => "image/png" - ], - [ - "src" => "icon.png", - "sizes" => "256x256", - "type" => "image/png" - ], - [ - "src" => "icon.ico", - "sizes" => "48x48", - "type" => "image/vnd.microsoft.icon" - ], - [ - "src" => "favicon.ico", - "sizes" => "48x48", - "type" => "image/vnd.microsoft.icon" - ], - ], - ]; + $manifest = config('manifest'); + $manifest["background_color"] = setting('app-color'); + return response()->json($manifest); } } diff --git a/app/Config/manifest.php b/app/Config/manifest.php new file mode 100644 index 000000000..640ba70e6 --- /dev/null +++ b/app/Config/manifest.php @@ -0,0 +1,55 @@ + (env('APP_NAME' | 'BookStack') ??'BookStack' ), + "short_name" => "bookstack", + "start_url" => "./", + "scope" => ".", + "display" => "standalone", + "background_color" => "#fff", + "description" =>( env('APP_NAME' | 'BookStack') ??'BookStack'), + "categories" => [ + "productivity", + "lifestyle" + ], + "launch_handler" => [ + "client_mode" => "focus-existing" + ], + "orientation" => "portrait", + "icons" => [ + [ + "src" => "/icon-64.png", + "sizes" => "64x64", + "type" => "image/png" + ], + [ + "src" => "/icon-32.png", + "sizes" => "32x32", + "type" => "image/png" + ], + [ + "src" => "/icon-128.png", + "sizes" => "128x128", + "type" => "image/png" + ], + [ + "src" => "icon-180.png", + "sizes" => "180x180", + "type" => "image/png" + ], + [ + "src" => "icon.png", + "sizes" => "256x256", + "type" => "image/png" + ], + [ + "src" => "icon.ico", + "sizes" => "48x48", + "type" => "image/vnd.microsoft.icon" + ], + [ + "src" => "favicon.ico", + "sizes" => "48x48", + "type" => "image/vnd.microsoft.icon" + ], + ], +]; \ No newline at end of file From a8b5652210bf2258847e5880d6c6866fdfdc04a3 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 8 Sep 2023 14:16:09 +0100 Subject: [PATCH 05/97] Started aligning app-wide outbound http calling behaviour --- app/Access/Oidc/OidcOAuthProvider.php | 13 ++--- app/Access/Oidc/OidcProviderSettings.php | 2 +- app/Access/Oidc/OidcService.php | 8 ++-- app/Activity/DispatchWebhookJob.php | 30 +++++++----- app/App/Providers/AppServiceProvider.php | 12 ++--- app/Http/HttpClientHistory.php | 28 +++++++++++ app/Http/HttpRequestService.php | 61 ++++++++++++++++++++++++ tests/Actions/WebhookCallTest.php | 50 ++++++++----------- tests/Auth/OidcTest.php | 24 ++++------ tests/TestCase.php | 24 +++------- tests/ThemeTest.php | 14 ++---- 11 files changed, 159 insertions(+), 107 deletions(-) create mode 100644 app/Http/HttpClientHistory.php create mode 100644 app/Http/HttpRequestService.php diff --git a/app/Access/Oidc/OidcOAuthProvider.php b/app/Access/Oidc/OidcOAuthProvider.php index 2ed8cd5c9..d2dc829b7 100644 --- a/app/Access/Oidc/OidcOAuthProvider.php +++ b/app/Access/Oidc/OidcOAuthProvider.php @@ -20,15 +20,8 @@ class OidcOAuthProvider extends AbstractProvider { use BearerAuthorizationTrait; - /** - * @var string - */ - protected $authorizationEndpoint; - - /** - * @var string - */ - protected $tokenEndpoint; + protected string $authorizationEndpoint; + protected string $tokenEndpoint; /** * Scopes to use for the OIDC authorization call. @@ -60,7 +53,7 @@ class OidcOAuthProvider extends AbstractProvider } /** - * Add an additional scope to this provider upon the default. + * Add another scope to this provider upon the default. */ public function addScope(string $scope): void { diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php index 9c8b1b264..fa3f579b1 100644 --- a/app/Access/Oidc/OidcProviderSettings.php +++ b/app/Access/Oidc/OidcProviderSettings.php @@ -59,7 +59,7 @@ class OidcProviderSettings } } - if (strpos($this->issuer, 'https://') !== 0) { + if (!str_starts_with($this->issuer, 'https://')) { throw new InvalidArgumentException('Issuer value must start with https://'); } } diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 6d13fe8f1..d22b26eec 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -9,13 +9,13 @@ use BookStack\Exceptions\JsonDebugException; use BookStack\Exceptions\StoppedAuthenticationException; use BookStack\Exceptions\UserRegistrationException; use BookStack\Facades\Theme; +use BookStack\Http\HttpRequestService; use BookStack\Theming\ThemeEvents; use BookStack\Users\Models\User; use Illuminate\Support\Arr; use Illuminate\Support\Facades\Cache; use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider; use League\OAuth2\Client\Provider\Exception\IdentityProviderException; -use Psr\Http\Client\ClientInterface as HttpClient; /** * Class OpenIdConnectService @@ -26,7 +26,7 @@ class OidcService public function __construct( protected RegistrationService $registrationService, protected LoginService $loginService, - protected HttpClient $httpClient, + protected HttpRequestService $http, protected GroupSyncService $groupService ) { } @@ -94,7 +94,7 @@ class OidcService // Run discovery if ($config['discover'] ?? false) { try { - $settings->discoverFromIssuer($this->httpClient, Cache::store(null), 15); + $settings->discoverFromIssuer($this->http->buildClient(5), Cache::store(null), 15); } catch (OidcIssuerDiscoveryException $exception) { throw new OidcException('OIDC Discovery Error: ' . $exception->getMessage()); } @@ -111,7 +111,7 @@ class OidcService protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvider { $provider = new OidcOAuthProvider($settings->arrayForProvider(), [ - 'httpClient' => $this->httpClient, + 'httpClient' => $this->http->buildClient(5), 'optionProvider' => new HttpBasicAuthOptionProvider(), ]); diff --git a/app/Activity/DispatchWebhookJob.php b/app/Activity/DispatchWebhookJob.php index 405bca49c..09fa12785 100644 --- a/app/Activity/DispatchWebhookJob.php +++ b/app/Activity/DispatchWebhookJob.php @@ -6,6 +6,7 @@ use BookStack\Activity\Models\Loggable; use BookStack\Activity\Models\Webhook; use BookStack\Activity\Tools\WebhookFormatter; use BookStack\Facades\Theme; +use BookStack\Http\HttpRequestService; use BookStack\Theming\ThemeEvents; use BookStack\Users\Models\User; use BookStack\Util\SsrUrlValidator; @@ -14,8 +15,8 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; -use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log; +use Psr\Http\Client\ClientExceptionInterface; class DispatchWebhookJob implements ShouldQueue { @@ -49,25 +50,28 @@ class DispatchWebhookJob implements ShouldQueue * * @return void */ - public function handle() + public function handle(HttpRequestService $http) { $lastError = null; try { (new SsrUrlValidator())->ensureAllowed($this->webhook->endpoint); - $response = Http::asJson() - ->withOptions(['allow_redirects' => ['strict' => true]]) - ->timeout($this->webhook->timeout) - ->post($this->webhook->endpoint, $this->webhookData); - } catch (\Exception $exception) { - $lastError = $exception->getMessage(); - Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\""); - } + $client = $http->buildClient($this->webhook->timeout, [ + 'connect_timeout' => 10, + 'allow_redirects' => ['strict' => true], + ]); - if (isset($response) && $response->failed()) { - $lastError = "Response status from endpoint was {$response->status()}"; - Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$response->status()}"); + $response = $client->sendRequest($http->jsonRequest('POST', $this->webhook->endpoint, $this->webhookData)); + $statusCode = $response->getStatusCode(); + + if ($statusCode >= 400) { + $lastError = "Response status from endpoint was {$statusCode}"; + Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$statusCode}"); + } + } catch (ClientExceptionInterface $error) { + $lastError = $error->getMessage(); + Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\""); } $this->webhook->last_called_at = now(); diff --git a/app/App/Providers/AppServiceProvider.php b/app/App/Providers/AppServiceProvider.php index deb664ba6..0275a5489 100644 --- a/app/App/Providers/AppServiceProvider.php +++ b/app/App/Providers/AppServiceProvider.php @@ -9,16 +9,15 @@ use BookStack\Entities\Models\Bookshelf; use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Page; use BookStack\Exceptions\BookStackExceptionHandlerPage; +use BookStack\Http\HttpRequestService; use BookStack\Permissions\PermissionApplicator; use BookStack\Settings\SettingService; use BookStack\Util\CspService; -use GuzzleHttp\Client; use Illuminate\Contracts\Foundation\ExceptionRenderer; use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Support\Facades\Schema; use Illuminate\Support\Facades\URL; use Illuminate\Support\ServiceProvider; -use Psr\Http\Client\ClientInterface as HttpClientInterface; class AppServiceProvider extends ServiceProvider { @@ -39,6 +38,7 @@ class AppServiceProvider extends ServiceProvider SettingService::class => SettingService::class, SocialAuthService::class => SocialAuthService::class, CspService::class => CspService::class, + HttpRequestService::class => HttpRequestService::class, ]; /** @@ -51,7 +51,7 @@ class AppServiceProvider extends ServiceProvider // Set root URL $appUrl = config('app.url'); if ($appUrl) { - $isHttps = (strpos($appUrl, 'https://') === 0); + $isHttps = str_starts_with($appUrl, 'https://'); URL::forceRootUrl($appUrl); URL::forceScheme($isHttps ? 'https' : 'http'); } @@ -75,12 +75,6 @@ class AppServiceProvider extends ServiceProvider */ public function register() { - $this->app->bind(HttpClientInterface::class, function ($app) { - return new Client([ - 'timeout' => 3, - ]); - }); - $this->app->singleton(PermissionApplicator::class, function ($app) { return new PermissionApplicator(null); }); diff --git a/app/Http/HttpClientHistory.php b/app/Http/HttpClientHistory.php new file mode 100644 index 000000000..7d019d77c --- /dev/null +++ b/app/Http/HttpClientHistory.php @@ -0,0 +1,28 @@ +container); + } + + public function requestAt(int $index): ?GuzzleRequest + { + return $this->container[$index]['request'] ?? null; + } + + public function latestRequest(): ?GuzzleRequest + { + return $this->requestAt($this->requestCount() - 1); + } +} diff --git a/app/Http/HttpRequestService.php b/app/Http/HttpRequestService.php new file mode 100644 index 000000000..8318474aa --- /dev/null +++ b/app/Http/HttpRequestService.php @@ -0,0 +1,61 @@ + $timeout, + 'handler' => $this->handler, + ]; + + return new Client(array_merge($options, $defaultOptions)); + } + + /** + * Create a new JSON http request for use with a client. + */ + public function jsonRequest(string $method, string $uri, array $data): GuzzleRequest + { + $headers = ['Content-Type' => 'application/json']; + return new GuzzleRequest($method, $uri, $headers, json_encode($data)); + } + + /** + * Mock any http clients built from this service, and response with the given responses. + * Returns history which can then be queried. + * @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware + */ + public function mockClient(array $responses = []): HttpClientHistory + { + $container = []; + $history = Middleware::history($container); + $mock = new MockHandler($responses); + $this->handler = HandlerStack::create($mock); + $this->handler->push($history, 'history'); + + return new HttpClientHistory($container); + } + + /** + * Clear mocking that has been set up for clients. + */ + public function clearMocking(): void + { + $this->handler = null; + } +} diff --git a/tests/Actions/WebhookCallTest.php b/tests/Actions/WebhookCallTest.php index 0746aa3a1..81bd7e7e8 100644 --- a/tests/Actions/WebhookCallTest.php +++ b/tests/Actions/WebhookCallTest.php @@ -7,11 +7,10 @@ use BookStack\Activity\DispatchWebhookJob; use BookStack\Activity\Models\Webhook; use BookStack\Activity\Tools\ActivityLogger; use BookStack\Api\ApiToken; -use BookStack\Entities\Models\PageRevision; use BookStack\Users\Models\User; -use Illuminate\Http\Client\Request; +use GuzzleHttp\Exception\ConnectException; +use GuzzleHttp\Psr7\Response; use Illuminate\Support\Facades\Bus; -use Illuminate\Support\Facades\Http; use Tests\TestCase; class WebhookCallTest extends TestCase @@ -50,10 +49,10 @@ class WebhookCallTest extends TestCase public function test_webhook_runs_for_delete_actions() { + // This test must not fake the queue/bus since this covers an issue + // around handling and serialization of items now deleted from the database. $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); - Http::fake([ - '*' => Http::response('', 500), - ]); + $this->mockHttpClient([new Response(500)]); $user = $this->users->newUser(); $resp = $this->asAdmin()->delete($user->getEditUrl()); @@ -69,9 +68,7 @@ class WebhookCallTest extends TestCase public function test_failed_webhook_call_logs_error() { $logger = $this->withTestLogger(); - Http::fake([ - '*' => Http::response('', 500), - ]); + $this->mockHttpClient([new Response(500)]); $webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); $this->assertNull($webhook->last_errored_at); @@ -86,7 +83,7 @@ class WebhookCallTest extends TestCase public function test_webhook_call_exception_is_caught_and_logged() { - Http::shouldReceive('asJson')->andThrow(new \Exception('Failed to perform request')); + $this->mockHttpClient([new ConnectException('Failed to perform request', new \GuzzleHttp\Psr7\Request('GET', ''))]); $logger = $this->withTestLogger(); $webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); @@ -104,11 +101,11 @@ class WebhookCallTest extends TestCase public function test_webhook_uses_ssr_hosts_option_if_set() { config()->set('app.ssr_hosts', 'https://*.example.com'); - $http = Http::fake(); + $responses = $this->mockHttpClient(); $webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.co.uk'], ['all']); $this->runEvent(ActivityType::ROLE_CREATE); - $http->assertNothingSent(); + $this->assertEquals(0, $responses->requestCount()); $webhook->refresh(); $this->assertEquals('The URL does not match the configured allowed SSR hosts', $webhook->last_error); @@ -117,29 +114,24 @@ class WebhookCallTest extends TestCase public function test_webhook_call_data_format() { - Http::fake([ - '*' => Http::response('', 200), - ]); + $responses = $this->mockHttpClient([new Response(200, [], '')]); $webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); $page = $this->entities->page(); $editor = $this->users->editor(); $this->runEvent(ActivityType::PAGE_UPDATE, $page, $editor); - Http::assertSent(function (Request $request) use ($editor, $page, $webhook) { - $reqData = $request->data(); - - return $request->isJson() - && $reqData['event'] === 'page_update' - && $reqData['text'] === ($editor->name . ' updated page "' . $page->name . '"') - && is_string($reqData['triggered_at']) - && $reqData['triggered_by']['name'] === $editor->name - && $reqData['triggered_by_profile_url'] === $editor->getProfileUrl() - && $reqData['webhook_id'] === $webhook->id - && $reqData['webhook_name'] === $webhook->name - && $reqData['url'] === $page->getUrl() - && $reqData['related_item']['name'] === $page->name; - }); + $request = $responses->latestRequest(); + $reqData = json_decode($request->getBody(), true); + $this->assertEquals('page_update', $reqData['event']); + $this->assertEquals(($editor->name . ' updated page "' . $page->name . '"'), $reqData['text']); + $this->assertIsString($reqData['triggered_at']); + $this->assertEquals($editor->name, $reqData['triggered_by']['name']); + $this->assertEquals($editor->getProfileUrl(), $reqData['triggered_by_profile_url']); + $this->assertEquals($webhook->id, $reqData['webhook_id']); + $this->assertEquals($webhook->name, $reqData['webhook_name']); + $this->assertEquals($page->getUrl(), $reqData['url']); + $this->assertEquals($page->name, $reqData['related_item']['name']); } protected function runEvent(string $event, $detail = '', ?User $user = null) diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index 191a25f88..367e84816 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -7,7 +7,6 @@ use BookStack\Facades\Theme; use BookStack\Theming\ThemeEvents; use BookStack\Users\Models\Role; use BookStack\Users\Models\User; -use GuzzleHttp\Psr7\Request; use GuzzleHttp\Psr7\Response; use Illuminate\Testing\TestResponse; use Tests\Helpers\OidcJwtHelper; @@ -137,7 +136,7 @@ class OidcTest extends TestCase $this->post('/oidc/login'); $state = session()->get('oidc_state'); - $transactions = &$this->mockHttpClient([$this->getMockAuthorizationResponse([ + $transactions = $this->mockHttpClient([$this->getMockAuthorizationResponse([ 'email' => 'benny@example.com', 'sub' => 'benny1010101', ])]); @@ -146,9 +145,8 @@ class OidcTest extends TestCase // App calls token endpoint to get id token $resp = $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); $resp->assertRedirect('/'); - $this->assertCount(1, $transactions); - /** @var Request $tokenRequest */ - $tokenRequest = $transactions[0]['request']; + $this->assertEquals(1, $transactions->requestCount()); + $tokenRequest = $transactions->latestRequest(); $this->assertEquals('https://oidc.local/token', (string) $tokenRequest->getUri()); $this->assertEquals('POST', $tokenRequest->getMethod()); $this->assertEquals('Basic ' . base64_encode(OidcJwtHelper::defaultClientId() . ':testpass'), $tokenRequest->getHeader('Authorization')[0]); @@ -279,7 +277,7 @@ class OidcTest extends TestCase { $this->withAutodiscovery(); - $transactions = &$this->mockHttpClient([ + $transactions = $this->mockHttpClient([ $this->getAutoDiscoveryResponse(), $this->getJwksResponse(), ]); @@ -289,11 +287,9 @@ class OidcTest extends TestCase $this->runLogin(); $this->assertTrue(auth()->check()); - /** @var Request $discoverRequest */ - $discoverRequest = $transactions[0]['request']; - /** @var Request $discoverRequest */ - $keysRequest = $transactions[1]['request']; + $discoverRequest = $transactions->requestAt(0); + $keysRequest = $transactions->requestAt(1); $this->assertEquals('GET', $keysRequest->getMethod()); $this->assertEquals('GET', $discoverRequest->getMethod()); $this->assertEquals(OidcJwtHelper::defaultIssuer() . '/.well-known/openid-configuration', $discoverRequest->getUri()); @@ -316,7 +312,7 @@ class OidcTest extends TestCase { $this->withAutodiscovery(); - $transactions = &$this->mockHttpClient([ + $transactions = $this->mockHttpClient([ $this->getAutoDiscoveryResponse(), $this->getJwksResponse(), $this->getAutoDiscoveryResponse([ @@ -327,15 +323,15 @@ class OidcTest extends TestCase // Initial run $this->post('/oidc/login'); - $this->assertCount(2, $transactions); + $this->assertEquals(2, $transactions->requestCount()); // Second run, hits cache $this->post('/oidc/login'); - $this->assertCount(2, $transactions); + $this->assertEquals(2, $transactions->requestCount()); // Third run, different issuer, new cache key config()->set(['oidc.issuer' => 'https://auto.example.com']); $this->post('/oidc/login'); - $this->assertCount(4, $transactions); + $this->assertEquals(4, $transactions->requestCount()); } public function test_auth_login_with_autodiscovery_with_keys_that_do_not_have_alg_property() diff --git a/tests/TestCase.php b/tests/TestCase.php index 0ab0792bd..e3c47cd89 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -3,13 +3,11 @@ namespace Tests; use BookStack\Entities\Models\Entity; +use BookStack\Http\HttpClientHistory; +use BookStack\Http\HttpRequestService; use BookStack\Settings\SettingService; use BookStack\Uploads\HttpFetcher; use BookStack\Users\Models\User; -use GuzzleHttp\Client; -use GuzzleHttp\Handler\MockHandler; -use GuzzleHttp\HandlerStack; -use GuzzleHttp\Middleware; use Illuminate\Contracts\Console\Kernel; use Illuminate\Foundation\Testing\DatabaseTransactions; use Illuminate\Foundation\Testing\TestCase as BaseTestCase; @@ -21,7 +19,6 @@ use Illuminate\Testing\Assert as PHPUnit; use Mockery; use Monolog\Handler\TestHandler; use Monolog\Logger; -use Psr\Http\Client\ClientInterface; use Ssddanbrown\AssertHtml\TestsHtml; use Tests\Helpers\EntityProvider; use Tests\Helpers\FileProvider; @@ -115,6 +112,7 @@ abstract class TestCase extends BaseTestCase */ protected function mockHttpFetch($returnData, int $times = 1) { + // TODO - Remove $mockHttp = Mockery::mock(HttpFetcher::class); $this->app[HttpFetcher::class] = $mockHttp; $mockHttp->shouldReceive('fetch') @@ -123,21 +121,11 @@ abstract class TestCase extends BaseTestCase } /** - * Mock the http client used in BookStack. - * Returns a reference to the container which holds all history of http transactions. - * - * @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware + * Mock the http client used in BookStack http calls. */ - protected function &mockHttpClient(array $responses = []): array + protected function mockHttpClient(array $responses = []): HttpClientHistory { - $container = []; - $history = Middleware::history($container); - $mock = new MockHandler($responses); - $handlerStack = new HandlerStack($mock); - $handlerStack->push($history); - $this->app[ClientInterface::class] = new Client(['handler' => $handlerStack]); - - return $container; + return $this->app->make(HttpRequestService::class)->mockClient($responses); } /** diff --git a/tests/ThemeTest.php b/tests/ThemeTest.php index 6976f2384..08c99d297 100644 --- a/tests/ThemeTest.php +++ b/tests/ThemeTest.php @@ -12,13 +12,10 @@ use BookStack\Facades\Theme; use BookStack\Theming\ThemeEvents; use BookStack\Users\Models\User; use Illuminate\Console\Command; -use Illuminate\Http\Client\Request as HttpClientRequest; use Illuminate\Http\Request; use Illuminate\Http\Response; use Illuminate\Support\Facades\Artisan; use Illuminate\Support\Facades\File; -use Illuminate\Support\Facades\Http; -use League\CommonMark\ConfigurableEnvironmentInterface; use League\CommonMark\Environment\Environment; class ThemeTest extends TestCase @@ -177,9 +174,7 @@ class ThemeTest extends TestCase }; Theme::listen(ThemeEvents::WEBHOOK_CALL_BEFORE, $callback); - Http::fake([ - '*' => Http::response('', 200), - ]); + $responses = $this->mockHttpClient([new \GuzzleHttp\Psr7\Response(200, [], '')]); $webhook = new Webhook(['name' => 'Test webhook', 'endpoint' => 'https://example.com']); $webhook->save(); @@ -193,9 +188,10 @@ class ThemeTest extends TestCase $this->assertEquals($webhook->id, $args[1]->id); $this->assertEquals($detail->id, $args[2]->id); - Http::assertSent(function (HttpClientRequest $request) { - return $request->isJson() && $request->data()['test'] === 'hello!'; - }); + $this->assertEquals(1, $responses->requestCount()); + $request = $responses->latestRequest(); + $reqData = json_decode($request->getBody(), true); + $this->assertEquals('hello!', $reqData['test']); } public function test_event_activity_logged() From 06490f624c3923e945bf86a2930ff85c062a0bad Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 8 Sep 2023 17:16:57 +0100 Subject: [PATCH 06/97] Removed use of HttpFetcher - Fixed some existing issues in new aligned process. - Manually tested each external call scenario. --- app/Activity/DispatchWebhookJob.php | 3 +- app/Http/HttpClientHistory.php | 5 +++ app/Http/HttpRequestService.php | 13 +++++- app/Uploads/HttpFetcher.php | 38 ------------------ app/Uploads/UserAvatars.php | 22 +++++----- tests/TestCase.php | 15 ------- tests/Uploads/AvatarTest.php | 62 ++++++++++++----------------- 7 files changed, 54 insertions(+), 104 deletions(-) delete mode 100644 app/Uploads/HttpFetcher.php diff --git a/app/Activity/DispatchWebhookJob.php b/app/Activity/DispatchWebhookJob.php index 09fa12785..e1771b114 100644 --- a/app/Activity/DispatchWebhookJob.php +++ b/app/Activity/DispatchWebhookJob.php @@ -16,7 +16,6 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Log; -use Psr\Http\Client\ClientExceptionInterface; class DispatchWebhookJob implements ShouldQueue { @@ -69,7 +68,7 @@ class DispatchWebhookJob implements ShouldQueue $lastError = "Response status from endpoint was {$statusCode}"; Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$statusCode}"); } - } catch (ClientExceptionInterface $error) { + } catch (\Exception $error) { $lastError = $error->getMessage(); Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\""); } diff --git a/app/Http/HttpClientHistory.php b/app/Http/HttpClientHistory.php index 7d019d77c..f224b1779 100644 --- a/app/Http/HttpClientHistory.php +++ b/app/Http/HttpClientHistory.php @@ -25,4 +25,9 @@ class HttpClientHistory { return $this->requestAt($this->requestCount() - 1); } + + public function all(): array + { + return $this->container; + } } diff --git a/app/Http/HttpRequestService.php b/app/Http/HttpRequestService.php index 8318474aa..f59c298a6 100644 --- a/app/Http/HttpRequestService.php +++ b/app/Http/HttpRequestService.php @@ -7,6 +7,7 @@ use GuzzleHttp\Handler\MockHandler; use GuzzleHttp\HandlerStack; use GuzzleHttp\Middleware; use GuzzleHttp\Psr7\Request as GuzzleRequest; +use GuzzleHttp\Psr7\Response; use Psr\Http\Client\ClientInterface; class HttpRequestService @@ -16,7 +17,7 @@ class HttpRequestService /** * Build a new http client for sending requests on. */ - public function buildClient(int $timeout, array $options): ClientInterface + public function buildClient(int $timeout, array $options = []): ClientInterface { $defaultOptions = [ 'timeout' => $timeout, @@ -40,8 +41,16 @@ class HttpRequestService * Returns history which can then be queried. * @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware */ - public function mockClient(array $responses = []): HttpClientHistory + public function mockClient(array $responses = [], bool $pad = true): HttpClientHistory { + // By default, we pad out the responses with 10 successful values so that requests will be + // properly recorded for inspection. Otherwise, we can't later check if we're received + // too many requests. + if ($pad) { + $response = new Response(200, [], 'success'); + $responses = array_merge($responses, array_fill(0, 10, $response)); + } + $container = []; $history = Middleware::history($container); $mock = new MockHandler($responses); diff --git a/app/Uploads/HttpFetcher.php b/app/Uploads/HttpFetcher.php deleted file mode 100644 index fcb4147e9..000000000 --- a/app/Uploads/HttpFetcher.php +++ /dev/null @@ -1,38 +0,0 @@ - $uri, - CURLOPT_RETURNTRANSFER => 1, - CURLOPT_CONNECTTIMEOUT => 5, - ]); - - $data = curl_exec($ch); - $err = curl_error($ch); - curl_close($ch); - - if ($err) { - $errno = curl_errno($ch); - throw new HttpFetchException($err, $errno); - } - - return $data; - } -} diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index 3cd37812a..9692b3f38 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -3,20 +3,20 @@ namespace BookStack\Uploads; use BookStack\Exceptions\HttpFetchException; +use BookStack\Http\HttpRequestService; use BookStack\Users\Models\User; use Exception; +use GuzzleHttp\Psr7\Request; use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; +use Psr\Http\Client\ClientExceptionInterface; class UserAvatars { - protected $imageService; - protected $http; - - public function __construct(ImageService $imageService, HttpFetcher $http) - { - $this->imageService = $imageService; - $this->http = $http; + public function __construct( + protected ImageService $imageService, + protected HttpRequestService $http + ) { } /** @@ -112,8 +112,10 @@ class UserAvatars protected function getAvatarImageData(string $url): string { try { - $imageData = $this->http->fetch($url); - } catch (HttpFetchException $exception) { + $client = $this->http->buildClient(5); + $response = $client->sendRequest(new Request('GET', $url)); + $imageData = (string) $response->getBody(); + } catch (ClientExceptionInterface $exception) { throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url]), $exception->getCode(), $exception); } @@ -127,7 +129,7 @@ class UserAvatars { $fetchUrl = $this->getAvatarUrl(); - return is_string($fetchUrl) && strpos($fetchUrl, 'http') === 0; + return str_starts_with($fetchUrl, 'http'); } /** diff --git a/tests/TestCase.php b/tests/TestCase.php index e3c47cd89..f8f59977a 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -6,7 +6,6 @@ use BookStack\Entities\Models\Entity; use BookStack\Http\HttpClientHistory; use BookStack\Http\HttpRequestService; use BookStack\Settings\SettingService; -use BookStack\Uploads\HttpFetcher; use BookStack\Users\Models\User; use Illuminate\Contracts\Console\Kernel; use Illuminate\Foundation\Testing\DatabaseTransactions; @@ -16,7 +15,6 @@ use Illuminate\Support\Env; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use Illuminate\Testing\Assert as PHPUnit; -use Mockery; use Monolog\Handler\TestHandler; use Monolog\Logger; use Ssddanbrown\AssertHtml\TestsHtml; @@ -107,19 +105,6 @@ abstract class TestCase extends BaseTestCase } } - /** - * Mock the HttpFetcher service and return the given data on fetch. - */ - protected function mockHttpFetch($returnData, int $times = 1) - { - // TODO - Remove - $mockHttp = Mockery::mock(HttpFetcher::class); - $this->app[HttpFetcher::class] = $mockHttp; - $mockHttp->shouldReceive('fetch') - ->times($times) - ->andReturn($returnData); - } - /** * Mock the http client used in BookStack http calls. */ diff --git a/tests/Uploads/AvatarTest.php b/tests/Uploads/AvatarTest.php index 363c1fa95..f5b49a9fc 100644 --- a/tests/Uploads/AvatarTest.php +++ b/tests/Uploads/AvatarTest.php @@ -3,9 +3,11 @@ namespace Tests\Uploads; use BookStack\Exceptions\HttpFetchException; -use BookStack\Uploads\HttpFetcher; use BookStack\Uploads\UserAvatars; use BookStack\Users\Models\User; +use GuzzleHttp\Exception\ConnectException; +use GuzzleHttp\Psr7\Request; +use GuzzleHttp\Psr7\Response; use Tests\TestCase; class AvatarTest extends TestCase @@ -22,27 +24,16 @@ class AvatarTest extends TestCase return User::query()->where('email', '=', $user->email)->first(); } - protected function assertImageFetchFrom(string $url) - { - $http = $this->mock(HttpFetcher::class); - - $http->shouldReceive('fetch') - ->once()->with($url) - ->andReturn($this->files->pngImageData()); - } - - protected function deleteUserImage(User $user) + protected function deleteUserImage(User $user): void { $this->files->deleteAtRelativePath($user->avatar->path); } public function test_gravatar_fetched_on_user_create() { - config()->set([ - 'services.disable_services' => false, - ]); + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); + config()->set(['services.disable_services' => false]); $user = User::factory()->make(); - $this->assertImageFetchFrom('https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'); $user = $this->createUserRequest($user); $this->assertDatabaseHas('images', [ @@ -50,6 +41,9 @@ class AvatarTest extends TestCase 'created_by' => $user->id, ]); $this->deleteUserImage($user); + + $expectedUri = 'https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'; + $this->assertEquals($expectedUri, $requests->latestRequest()->getUri()); } public function test_custom_url_used_if_set() @@ -61,24 +55,22 @@ class AvatarTest extends TestCase $user = User::factory()->make(); $url = 'https://example.com/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500'; - $this->assertImageFetchFrom($url); + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); $user = $this->createUserRequest($user); + $this->assertEquals($url, $requests->latestRequest()->getUri()); $this->deleteUserImage($user); } public function test_avatar_not_fetched_if_no_custom_url_and_services_disabled() { - config()->set([ - 'services.disable_services' => true, - ]); - + config()->set(['services.disable_services' => true]); $user = User::factory()->make(); - - $http = $this->mock(HttpFetcher::class); - $http->shouldNotReceive('fetch'); + $requests = $this->mockHttpClient([new Response()]); $this->createUserRequest($user); + + $this->assertEquals(0, $requests->requestCount()); } public function test_avatar_not_fetched_if_avatar_url_option_set_to_false() @@ -89,21 +81,18 @@ class AvatarTest extends TestCase ]); $user = User::factory()->make(); - - $http = $this->mock(HttpFetcher::class); - $http->shouldNotReceive('fetch'); + $requests = $this->mockHttpClient([new Response()]); $this->createUserRequest($user); + + $this->assertEquals(0, $requests->requestCount()); } public function test_no_failure_but_error_logged_on_failed_avatar_fetch() { - config()->set([ - 'services.disable_services' => false, - ]); + config()->set(['services.disable_services' => false]); - $http = $this->mock(HttpFetcher::class); - $http->shouldReceive('fetch')->andThrow(new HttpFetchException()); + $this->mockHttpClient([new ConnectException('Failed to connect', new Request('GET', ''))]); $logger = $this->withTestLogger(); @@ -122,17 +111,16 @@ class AvatarTest extends TestCase $user = User::factory()->make(); $avatar = app()->make(UserAvatars::class); - $url = 'http_malformed_url/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500'; $logger = $this->withTestLogger(); + $this->mockHttpClient([new ConnectException('Could not resolve host http_malformed_url', new Request('GET', ''))]); $avatar->fetchAndAssignToUser($user); + $url = 'http_malformed_url/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500'; $this->assertTrue($logger->hasError('Failed to save user avatar image')); $exception = $logger->getRecords()[0]['context']['exception']; - $this->assertEquals(new HttpFetchException( - 'Cannot get image from ' . $url, - 6, - (new HttpFetchException('Could not resolve host: http_malformed_url', 6)) - ), $exception); + $this->assertInstanceOf(HttpFetchException::class, $exception); + $this->assertEquals('Cannot get image from ' . $url, $exception->getMessage()); + $this->assertEquals('Could not resolve host http_malformed_url', $exception->getPrevious()->getMessage()); } } From 3928cbac1850fab85ac1c848ee867c8e20055b78 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 9 Sep 2023 12:39:01 +0100 Subject: [PATCH 07/97] Mail: changed default "MAIL_FROM" address Used an "example.com" address so we're using a propoer reserved domain, and to avoid these trying to be delivered to the main bookstackapp domain. Closes #4518 --- .env.example.complete | 2 +- app/Config/mail.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index 547e81818..0853bd1fe 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -72,7 +72,7 @@ MYSQL_ATTR_SSL_CA="/path/to/ca.pem" # Mail configuration # Refer to https://www.bookstackapp.com/docs/admin/email-webhooks/#email-configuration MAIL_DRIVER=smtp -MAIL_FROM=mail@bookstackapp.com +MAIL_FROM=bookstack@example.com MAIL_FROM_NAME=BookStack MAIL_HOST=localhost diff --git a/app/Config/mail.php b/app/Config/mail.php index 6400211e8..2906d769a 100644 --- a/app/Config/mail.php +++ b/app/Config/mail.php @@ -22,7 +22,7 @@ return [ // Global "From" address & name 'from' => [ - 'address' => env('MAIL_FROM', 'mail@bookstackapp.com'), + 'address' => env('MAIL_FROM', 'bookstack@example.com'), 'name' => env('MAIL_FROM_NAME', 'BookStack'), ], From 2fbf5527c70d5d3eadb2767ca5301ad05f7f28c8 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 10 Sep 2023 15:18:31 +0100 Subject: [PATCH 08/97] Simplified and aligned handling of mixed entity endpoints Fixes #4444 --- .../Controllers/FavouriteController.php | 53 +++++-------------- app/Activity/Controllers/WatchController.php | 43 ++------------- .../Tools/MixedEntityRequestHelper.php | 39 ++++++++++++++ .../views/entities/favourite-action.blade.php | 2 +- .../views/entities/watch-action.blade.php | 2 +- .../views/entities/watch-controls.blade.php | 2 +- tests/Activity/WatchTest.php | 6 +-- tests/FavouriteTest.php | 8 +-- 8 files changed, 67 insertions(+), 88 deletions(-) create mode 100644 app/Entities/Tools/MixedEntityRequestHelper.php diff --git a/app/Activity/Controllers/FavouriteController.php b/app/Activity/Controllers/FavouriteController.php index 1b88ffd64..d2534ddfe 100644 --- a/app/Activity/Controllers/FavouriteController.php +++ b/app/Activity/Controllers/FavouriteController.php @@ -6,11 +6,17 @@ use BookStack\Activity\Models\Favouritable; use BookStack\App\Model; use BookStack\Entities\Models\Entity; use BookStack\Entities\Queries\TopFavourites; +use BookStack\Entities\Tools\MixedEntityRequestHelper; use BookStack\Http\Controller; use Illuminate\Http\Request; class FavouriteController extends Controller { + public function __construct( + protected MixedEntityRequestHelper $entityHelper, + ) { + } + /** * Show a listing of all favourite items for the current user. */ @@ -36,13 +42,14 @@ class FavouriteController extends Controller */ public function add(Request $request) { - $favouritable = $this->getValidatedModelFromRequest($request); - $favouritable->favourites()->firstOrCreate([ + $modelInfo = $this->validate($request, $this->entityHelper->validationRules()); + $entity = $this->entityHelper->getVisibleEntityFromRequestData($modelInfo); + $entity->favourites()->firstOrCreate([ 'user_id' => user()->id, ]); $this->showSuccessNotification(trans('activities.favourite_add_notification', [ - 'name' => $favouritable->name, + 'name' => $entity->name, ])); return redirect()->back(); @@ -53,48 +60,16 @@ class FavouriteController extends Controller */ public function remove(Request $request) { - $favouritable = $this->getValidatedModelFromRequest($request); - $favouritable->favourites()->where([ + $modelInfo = $this->validate($request, $this->entityHelper->validationRules()); + $entity = $this->entityHelper->getVisibleEntityFromRequestData($modelInfo); + $entity->favourites()->where([ 'user_id' => user()->id, ])->delete(); $this->showSuccessNotification(trans('activities.favourite_remove_notification', [ - 'name' => $favouritable->name, + 'name' => $entity->name, ])); return redirect()->back(); } - - /** - * @throws \Illuminate\Validation\ValidationException - * @throws \Exception - */ - protected function getValidatedModelFromRequest(Request $request): Entity - { - $modelInfo = $this->validate($request, [ - 'type' => ['required', 'string'], - 'id' => ['required', 'integer'], - ]); - - if (!class_exists($modelInfo['type'])) { - throw new \Exception('Model not found'); - } - - /** @var Model $model */ - $model = new $modelInfo['type'](); - if (!$model instanceof Favouritable) { - throw new \Exception('Model not favouritable'); - } - - $modelInstance = $model->newQuery() - ->where('id', '=', $modelInfo['id']) - ->first(['id', 'name', 'owned_by']); - - $inaccessibleEntity = ($modelInstance instanceof Entity && !userCan('view', $modelInstance)); - if (is_null($modelInstance) || $inaccessibleEntity) { - throw new \Exception('Model instance not found'); - } - - return $modelInstance; - } } diff --git a/app/Activity/Controllers/WatchController.php b/app/Activity/Controllers/WatchController.php index 3d7e18116..d63918fb3 100644 --- a/app/Activity/Controllers/WatchController.php +++ b/app/Activity/Controllers/WatchController.php @@ -3,25 +3,23 @@ namespace BookStack\Activity\Controllers; use BookStack\Activity\Tools\UserEntityWatchOptions; -use BookStack\App\Model; -use BookStack\Entities\Models\Entity; +use BookStack\Entities\Tools\MixedEntityRequestHelper; use BookStack\Http\Controller; -use Exception; use Illuminate\Http\Request; -use Illuminate\Validation\ValidationException; class WatchController extends Controller { - public function update(Request $request) + public function update(Request $request, MixedEntityRequestHelper $entityHelper) { $this->checkPermission('receive-notifications'); $this->preventGuestAccess(); $requestData = $this->validate($request, [ 'level' => ['required', 'string'], + ...$entityHelper->validationRules() ]); - $watchable = $this->getValidatedModelFromRequest($request); + $watchable = $entityHelper->getVisibleEntityFromRequestData($requestData); $watchOptions = new UserEntityWatchOptions(user(), $watchable); $watchOptions->updateLevelByName($requestData['level']); @@ -29,37 +27,4 @@ class WatchController extends Controller return redirect()->back(); } - - /** - * @throws ValidationException - * @throws Exception - */ - protected function getValidatedModelFromRequest(Request $request): Entity - { - $modelInfo = $this->validate($request, [ - 'type' => ['required', 'string'], - 'id' => ['required', 'integer'], - ]); - - if (!class_exists($modelInfo['type'])) { - throw new Exception('Model not found'); - } - - /** @var Model $model */ - $model = new $modelInfo['type'](); - if (!$model instanceof Entity) { - throw new Exception('Model not an entity'); - } - - $modelInstance = $model->newQuery() - ->where('id', '=', $modelInfo['id']) - ->first(['id', 'name', 'owned_by']); - - $inaccessibleEntity = ($modelInstance instanceof Entity && !userCan('view', $modelInstance)); - if (is_null($modelInstance) || $inaccessibleEntity) { - throw new Exception('Model instance not found'); - } - - return $modelInstance; - } } diff --git a/app/Entities/Tools/MixedEntityRequestHelper.php b/app/Entities/Tools/MixedEntityRequestHelper.php new file mode 100644 index 000000000..8319f6aa0 --- /dev/null +++ b/app/Entities/Tools/MixedEntityRequestHelper.php @@ -0,0 +1,39 @@ +entities->get($requestData['type']); + + return $entityType->newQuery()->scopes(['visible'])->findOrFail($requestData['id']); + } + + /** + * Get the validation rules for an abstract entity request. + * @return array{type: string[], id: string[]} + */ + public function validationRules(): array + { + return [ + 'type' => ['required', 'string'], + 'id' => ['required', 'integer'], + ]; + } +} diff --git a/resources/views/entities/favourite-action.blade.php b/resources/views/entities/favourite-action.blade.php index 35189044b..e596fbdce 100644 --- a/resources/views/entities/favourite-action.blade.php +++ b/resources/views/entities/favourite-action.blade.php @@ -3,7 +3,7 @@ @endphp
{{ csrf_field() }} - + +
+ +

  • +
  • + + @icon('user-preferences') +
    {{ trans('preferences.preferences') }}
    +
    +
  • +
  • + @include('common.dark-mode-toggle', ['classes' => 'icon-item']) +
  • + + \ No newline at end of file diff --git a/resources/views/common/header.blade.php b/resources/views/common/header.blade.php index 97a411d84..86ad3563d 100644 --- a/resources/views/common/header.blade.php +++ b/resources/views/common/header.blade.php @@ -18,7 +18,7 @@
    - @if (hasAppAccess()) + @if (user()->hasAppAccess()) - -

  • -
  • - - @icon('user-preferences') -
    {{ trans('preferences.preferences') }}
    -
    -
  • -
  • - @include('common.dark-mode-toggle', ['classes' => 'icon-item']) -
  • - -
    + @if(!user()->isGuest()) + @include('common.header-user-menu', ['user' => user()]) @endif diff --git a/resources/views/errors/404.blade.php b/resources/views/errors/404.blade.php index a4e5a0dd4..27d66b30b 100644 --- a/resources/views/errors/404.blade.php +++ b/resources/views/errors/404.blade.php @@ -1,55 +1,55 @@ @extends('layouts.simple') @section('content') -
    +
    -
    -
    -
    - @include('errors.parts.not-found-text', [ - 'title' => $message ?? trans('errors.404_page_not_found'), - 'subtitle' => $subtitle ?? trans('errors.sorry_page_not_found'), - 'details' => $details ?? trans('errors.sorry_page_not_found_permission_warning'), - ]) -
    -
    - @if(!signedInUser()) - {{ trans('auth.log_in') }} - @endif - {{ trans('errors.return_home') }} +
    +
    +
    + @include('errors.parts.not-found-text', [ + 'title' => $message ?? trans('errors.404_page_not_found'), + 'subtitle' => $subtitle ?? trans('errors.sorry_page_not_found'), + 'details' => $details ?? trans('errors.sorry_page_not_found_permission_warning'), + ]) +
    +
    + @if(user()->isGuest()) + {{ trans('auth.log_in') }} + @endif + {{ trans('errors.return_home') }} +
    +
    + @if (setting('app-public') || !user()->isGuest()) +
    +
    +
    +

    {{ trans('entities.pages_popular') }}

    +
    + @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['page']), 'style' => 'compact']) +
    +
    +
    +
    +
    +

    {{ trans('entities.books_popular') }}

    +
    + @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['book']), 'style' => 'compact']) +
    +
    +
    +
    +
    +

    {{ trans('entities.chapters_popular') }}

    +
    + @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['chapter']), 'style' => 'compact']) +
    +
    +
    +
    + @endif
    - @if (setting('app-public') || !user()->isDefault()) -
    -
    -
    -

    {{ trans('entities.pages_popular') }}

    -
    - @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['page']), 'style' => 'compact']) -
    -
    -
    -
    -
    -

    {{ trans('entities.books_popular') }}

    -
    - @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['book']), 'style' => 'compact']) -
    -
    -
    -
    -
    -

    {{ trans('entities.chapters_popular') }}

    -
    - @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['chapter']), 'style' => 'compact']) -
    -
    -
    -
    - @endif -
    - @stop \ No newline at end of file diff --git a/resources/views/pages/show.blade.php b/resources/views/pages/show.blade.php index 1cbb81980..5d70f7c28 100644 --- a/resources/views/pages/show.blade.php +++ b/resources/views/pages/show.blade.php @@ -188,7 +188,7 @@ @if($watchOptions->canWatch() && !$watchOptions->isWatching()) @include('entities.watch-action', ['entity' => $page]) @endif - @if(signedInUser()) + @if(!user()->isGuest()) @include('entities.favourite-action', ['entity' => $page]) @endif @if(userCan('content-export')) diff --git a/resources/views/search/all.blade.php b/resources/views/search/all.blade.php index 96b14f6e5..64325d4c1 100644 --- a/resources/views/search/all.blade.php +++ b/resources/views/search/all.blade.php @@ -32,7 +32,7 @@
    {{ trans('entities.search_tags') }}
    @include('search.parts.term-list', ['type' => 'tags', 'currentList' => $options->tags]) - @if(signedInUser()) + @if(!user()->isGuest())
    {{ trans('entities.search_options') }}
    @component('search.parts.boolean-filter', ['filters' => $options->filters, 'name' => 'viewed_by_me', 'value' => null]) diff --git a/resources/views/shelves/show.blade.php b/resources/views/shelves/show.blade.php index 86dd6326d..8694ce86d 100644 --- a/resources/views/shelves/show.blade.php +++ b/resources/views/shelves/show.blade.php @@ -143,7 +143,7 @@ @endif - @if(signedInUser()) + @if(!user()->isGuest())
    @include('entities.favourite-action', ['entity' => $shelf]) @endif diff --git a/resources/views/users/preferences/index.blade.php b/resources/views/users/preferences/index.blade.php index 712fdb288..f8576ed9e 100644 --- a/resources/views/users/preferences/index.blade.php +++ b/resources/views/users/preferences/index.blade.php @@ -13,7 +13,7 @@
    - @if(signedInUser() && userCan('receive-notifications')) + @if(!user()->isGuest() && userCan('receive-notifications'))

    {{ trans('preferences.notifications') }}

    @@ -25,7 +25,7 @@
    @endif - @if(signedInUser()) + @if(!user()->isGuest())

    {{ trans('settings.users_edit_profile') }}

    diff --git a/tests/Entity/PageRevisionTest.php b/tests/Entity/PageRevisionTest.php index 97d5a6664..a272dc38b 100644 --- a/tests/Entity/PageRevisionTest.php +++ b/tests/Entity/PageRevisionTest.php @@ -136,7 +136,7 @@ class PageRevisionTest extends TestCase $page = $this->entities->page(); $this->createRevisions($page, 2); - $pageView = $this->get($page->getUrl()); + $pageView = $this->asViewer()->get($page->getUrl()); $pageView->assertSee('Revision #' . $page->revision_count); } diff --git a/tests/Helpers/UserRoleProvider.php b/tests/Helpers/UserRoleProvider.php index 3b2da369d..fe19cad4a 100644 --- a/tests/Helpers/UserRoleProvider.php +++ b/tests/Helpers/UserRoleProvider.php @@ -55,7 +55,7 @@ class UserRoleProvider */ public function guest(): User { - return User::getDefault(); + return User::getGuest(); } /** diff --git a/tests/PublicActionTest.php b/tests/PublicActionTest.php index 1e4dcbfb7..875b279a8 100644 --- a/tests/PublicActionTest.php +++ b/tests/PublicActionTest.php @@ -103,7 +103,7 @@ class PublicActionTest extends TestCase $resp = $this->post($chapter->getUrl('/create-guest-page'), ['name' => 'My guest page']); $resp->assertRedirect($chapter->book->getUrl('/page/my-guest-page/edit')); - $user = User::getDefault(); + $user = $this->users->guest(); $this->assertDatabaseHas('pages', [ 'name' => 'My guest page', 'chapter_id' => $chapter->id, @@ -197,7 +197,7 @@ class PublicActionTest extends TestCase public function test_public_view_can_take_on_other_roles() { $this->setSettings(['app-public' => 'true']); - $newRole = $this->users->attachNewRole(User::getDefault(), []); + $newRole = $this->users->attachNewRole($this->users->guest(), []); $page = $this->entities->page(); $this->permissions->disableEntityInheritedPermissions($page); $this->permissions->addEntityPermission($page, ['view', 'update'], $newRole); diff --git a/tests/TestCase.php b/tests/TestCase.php index f8f59977a..c59f843e9 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -42,7 +42,6 @@ abstract class TestCase extends BaseTestCase $this->permissions = new PermissionsProvider($this->users); $this->files = new FileProvider(); - User::clearDefault(); parent::setUp(); // We can uncomment the below to run tests with failings upon deprecations. diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php index df60bede6..a6d869b2f 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -191,7 +191,7 @@ class UserManagementTest extends TestCase public function test_guest_profile_shows_limited_form() { - $guest = User::getDefault(); + $guest = $this->users->guest(); $resp = $this->asAdmin()->get('/settings/users/' . $guest->id); $resp->assertSee('Guest'); $this->withHtml($resp)->assertElementNotExists('#password'); @@ -199,7 +199,7 @@ class UserManagementTest extends TestCase public function test_guest_profile_cannot_be_deleted() { - $guestUser = User::getDefault(); + $guestUser = $this->users->guest(); $resp = $this->asAdmin()->get('/settings/users/' . $guestUser->id . '/delete'); $resp->assertSee('Delete User'); $resp->assertSee('Guest'); diff --git a/tests/User/UserSearchTest.php b/tests/User/UserSearchTest.php index 1387311ce..76efbf4af 100644 --- a/tests/User/UserSearchTest.php +++ b/tests/User/UserSearchTest.php @@ -57,8 +57,7 @@ class UserSearchTest extends TestCase public function test_select_requires_logged_in_user() { $this->setSettings(['app-public' => true]); - $defaultUser = User::getDefault(); - $this->permissions->grantUserRolePermissions($defaultUser, ['users-manage']); + $this->permissions->grantUserRolePermissions($this->users->guest(), ['users-manage']); $resp = $this->get('/search/users/select?search=a'); $this->assertPermissionError($resp); From e16bdf443ce68e6363ea40124d66eb15c05f2e8b Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Sep 2023 13:49:03 +0100 Subject: [PATCH 24/97] Removed redundant null check --- app/Activity/Models/View.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Activity/Models/View.php b/app/Activity/Models/View.php index b593a7d27..30ead1193 100644 --- a/app/Activity/Models/View.php +++ b/app/Activity/Models/View.php @@ -41,7 +41,7 @@ class View extends Model public static function incrementFor(Viewable $viewable): int { $user = user(); - if (is_null($user) || $user->isGuest()) { + if ($user->isGuest()) { return 0; } From b292cf7090f8039f6b7d1a2200772ced34dd7c1e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Sep 2023 18:25:08 +0100 Subject: [PATCH 25/97] Extracted icon helper, aligned container resolution Also updated breadcrumb view composer to current standards. Closes #4553 --- app/Access/Controllers/ThrottlesLogins.php | 2 +- app/App/HomeController.php | 4 +- .../Providers/ViewTweaksServiceProvider.php | 2 +- app/App/helpers.php | 39 ++----------------- app/Entities/BreadcrumbsViewComposer.php | 20 +++------- app/Entities/Tools/TrashCan.php | 2 +- app/Users/Models/User.php | 2 +- app/Util/SvgIcon.php | 39 +++++++++++++++++++ 8 files changed, 54 insertions(+), 56 deletions(-) create mode 100644 app/Util/SvgIcon.php diff --git a/app/Access/Controllers/ThrottlesLogins.php b/app/Access/Controllers/ThrottlesLogins.php index 2a066dab5..25c3452f2 100644 --- a/app/Access/Controllers/ThrottlesLogins.php +++ b/app/Access/Controllers/ThrottlesLogins.php @@ -71,7 +71,7 @@ trait ThrottlesLogins */ protected function limiter(): RateLimiter { - return app(RateLimiter::class); + return app()->make(RateLimiter::class); } /** diff --git a/app/App/HomeController.php b/app/App/HomeController.php index 667af80d3..24b7c3ed8 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -78,14 +78,14 @@ class HomeController extends Controller } if ($homepageOption === 'bookshelves') { - $shelves = app(BookshelfRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); + $shelves = app()->make(BookshelfRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); $data = array_merge($commonData, ['shelves' => $shelves]); return view('home.shelves', $data); } if ($homepageOption === 'books') { - $books = app(BookRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); + $books = app()->make(BookRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); $data = array_merge($commonData, ['books' => $books]); return view('home.books', $data); diff --git a/app/App/Providers/ViewTweaksServiceProvider.php b/app/App/Providers/ViewTweaksServiceProvider.php index 16b5c1f97..10593ac8b 100644 --- a/app/App/Providers/ViewTweaksServiceProvider.php +++ b/app/App/Providers/ViewTweaksServiceProvider.php @@ -25,7 +25,7 @@ class ViewTweaksServiceProvider extends ServiceProvider // Custom blade view directives Blade::directive('icon', function ($expression) { - return ""; + return "toHtml(); ?>"; }); } } diff --git a/app/App/helpers.php b/app/App/helpers.php index b59a63448..af6dbcfc3 100644 --- a/app/App/helpers.php +++ b/app/App/helpers.php @@ -49,7 +49,7 @@ function userCan(string $permission, Model $ownable = null): bool } // Check permission on ownable item - $permissions = app(PermissionApplicator::class); + $permissions = app()->make(PermissionApplicator::class); return $permissions->checkOwnableUserAccess($ownable, $permission); } @@ -60,7 +60,7 @@ function userCan(string $permission, Model $ownable = null): bool */ function userCanOnAny(string $action, string $entityClass = ''): bool { - $permissions = app(PermissionApplicator::class); + $permissions = app()->make(PermissionApplicator::class); return $permissions->checkUserHasEntityPermissionOnAny($action, $entityClass); } @@ -72,7 +72,7 @@ function userCanOnAny(string $action, string $entityClass = ''): bool */ function setting(string $key = null, $default = null) { - $settingService = resolve(SettingService::class); + $settingService = app()->make(SettingService::class); if (is_null($key)) { return $settingService; @@ -97,39 +97,6 @@ function theme_path(string $path = ''): ?string return base_path('themes/' . $theme . ($path ? DIRECTORY_SEPARATOR . $path : $path)); } -/** - * Get fetch an SVG icon as a string. - * Checks for icons defined within a custom theme before defaulting back - * to the 'resources/assets/icons' folder. - * - * Returns an empty string if icon file not found. - */ -function icon(string $name, array $attrs = []): string -{ - $attrs = array_merge([ - 'class' => 'svg-icon', - 'data-icon' => $name, - 'role' => 'presentation', - ], $attrs); - $attrString = ' '; - foreach ($attrs as $attrName => $attr) { - $attrString .= $attrName . '="' . $attr . '" '; - } - - $iconPath = resource_path('icons/' . $name . '.svg'); - $themeIconPath = theme_path('icons/' . $name . '.svg'); - - if ($themeIconPath && file_exists($themeIconPath)) { - $iconPath = $themeIconPath; - } elseif (!file_exists($iconPath)) { - return ''; - } - - $fileContents = file_get_contents($iconPath); - - return str_replace('entityContextManager = $entityContextManager; + public function __construct( + protected ShelfContext $shelfContext + ) { } /** * Modify data when the view is composed. - * - * @param View $view */ - public function compose(View $view) + public function compose(View $view): void { $crumbs = $view->getData()['crumbs']; $firstCrumb = $crumbs[0] ?? null; + if ($firstCrumb instanceof Book) { - $shelf = $this->entityContextManager->getContextualShelfForBook($firstCrumb); + $shelf = $this->shelfContext->getContextualShelfForBook($firstCrumb); if ($shelf) { array_unshift($crumbs, $shelf); $view->with('crumbs', $crumbs); diff --git a/app/Entities/Tools/TrashCan.php b/app/Entities/Tools/TrashCan.php index 3c497024f..08276230c 100644 --- a/app/Entities/Tools/TrashCan.php +++ b/app/Entities/Tools/TrashCan.php @@ -197,7 +197,7 @@ class TrashCan $page->allRevisions()->delete(); // Delete Attached Files - $attachmentService = app(AttachmentService::class); + $attachmentService = app()->make(AttachmentService::class); foreach ($page->attachments as $attachment) { $attachmentService->deleteFile($attachment); } diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 1eeacfe2e..232ea8832 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -374,7 +374,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon */ public function refreshSlug(): string { - $this->slug = app(SlugGenerator::class)->generate($this); + $this->slug = app()->make(SlugGenerator::class)->generate($this); return $this->slug; } diff --git a/app/Util/SvgIcon.php b/app/Util/SvgIcon.php new file mode 100644 index 000000000..ce6e1c23e --- /dev/null +++ b/app/Util/SvgIcon.php @@ -0,0 +1,39 @@ + 'svg-icon', + 'data-icon' => $this->name, + 'role' => 'presentation', + ], $this->attrs); + + $attrString = ' '; + foreach ($attrs as $attrName => $attr) { + $attrString .= $attrName . '="' . $attr . '" '; + } + + $iconPath = resource_path('icons/' . $this->name . '.svg'); + $themeIconPath = theme_path('icons/' . $this->name . '.svg'); + + if ($themeIconPath && file_exists($themeIconPath)) { + $iconPath = $themeIconPath; + } elseif (!file_exists($iconPath)) { + return ''; + } + + $fileContents = file_get_contents($iconPath); + + return str_replace(' Date: Sun, 17 Sep 2023 13:29:06 +0100 Subject: [PATCH 26/97] Locales: Performed cleanup and alignment of locale handling - Reduced app settings down to what's required. - Used new view-shared $locale object instead of using globals via config. - Aligned language used to default on "locale" instead of mixing locale/language. For #4501 --- app/Config/app.php | 9 +- app/Http/Middleware/Localization.php | 31 ++----- app/Translation/LocaleDefinition.php | 45 ++++++++++ ...{LanguageManager.php => LocaleManager.php} | 84 ++++++++++--------- app/Users/Models/User.php | 4 +- resources/views/layouts/base.blade.php | 4 +- resources/views/layouts/export.blade.php | 2 +- resources/views/layouts/plain.blade.php | 4 +- .../pages/parts/markdown-editor.blade.php | 2 +- .../pages/parts/wysiwyg-editor.blade.php | 4 +- resources/views/users/create.blade.php | 2 +- resources/views/users/edit.blade.php | 2 +- .../vendor/notifications/email.blade.php | 2 +- tests/LanguageTest.php | 1 + 14 files changed, 116 insertions(+), 80 deletions(-) create mode 100644 app/Translation/LocaleDefinition.php rename app/Translation/{LanguageManager.php => LocaleManager.php} (70%) diff --git a/app/Config/app.php b/app/Config/app.php index 3a843c512..dcd3ffc31 100644 --- a/app/Config/app.php +++ b/app/Config/app.php @@ -83,10 +83,10 @@ return [ 'timezone' => env('APP_TIMEZONE', 'UTC'), // Default locale to use + // A default variant is also stored since Laravel can overwrite + // app.locale when dynamically setting the locale in-app. 'locale' => env('APP_LANG', 'en'), - - // Locales available - 'locales' => ['en', 'ar', 'bg', 'bs', 'ca', 'cs', 'cy', 'da', 'de', 'de_informal', 'el', 'es', 'es_AR', 'et', 'eu', 'fa', 'fr', 'he', 'hr', 'hu', 'id', 'it', 'ja', 'ka', 'ko', 'lt', 'lv', 'nl', 'nb', 'pt', 'pt_BR', 'sk', 'sl', 'sv', 'pl', 'ro', 'ru', 'tr', 'uk', 'uz', 'vi', 'zh_CN', 'zh_TW'], + 'default_locale' => env('APP_LANG', 'en'), // Application Fallback Locale 'fallback_locale' => 'en', @@ -94,9 +94,6 @@ return [ // Faker Locale 'faker_locale' => 'en_GB', - // Enable right-to-left text control. - 'rtl' => false, - // Auto-detect the locale for public users // For public users their locale can be guessed by headers sent by their // browser. This is usually set by users in their browser settings. diff --git a/app/Http/Middleware/Localization.php b/app/Http/Middleware/Localization.php index 47723e242..0be0b77eb 100644 --- a/app/Http/Middleware/Localization.php +++ b/app/Http/Middleware/Localization.php @@ -2,17 +2,14 @@ namespace BookStack\Http\Middleware; -use BookStack\Translation\LanguageManager; -use Carbon\Carbon; +use BookStack\Translation\LocaleManager; use Closure; class Localization { - protected LanguageManager $languageManager; - - public function __construct(LanguageManager $languageManager) - { - $this->languageManager = $languageManager; + public function __construct( + protected LocaleManager $localeManager + ) { } /** @@ -25,22 +22,12 @@ class Localization */ public function handle($request, Closure $next) { - // Get and record the default language in the config - $defaultLang = config('app.locale'); - config()->set('app.default_locale', $defaultLang); + // Share details of the user's locale for use in views + $userLocale = $this->localeManager->getForUser(user()); + view()->share('locale', $userLocale); - // Get the user's language and record that in the config for use in views - $userLang = $this->languageManager->getUserLanguage($request, $defaultLang); - config()->set('app.lang', str_replace('_', '-', $this->languageManager->getIsoName($userLang))); - - // Set text direction - if ($this->languageManager->isRTL($userLang)) { - config()->set('app.rtl', true); - } - - app()->setLocale($userLang); - Carbon::setLocale($userLang); - $this->languageManager->setPhpDateTimeLocale($userLang); + // Set locale for system components + $this->localeManager->setAppLocale($userLocale); return $next($request); } diff --git a/app/Translation/LocaleDefinition.php b/app/Translation/LocaleDefinition.php new file mode 100644 index 000000000..fe8640109 --- /dev/null +++ b/app/Translation/LocaleDefinition.php @@ -0,0 +1,45 @@ +appName; + } + + /** + * Provide the ISO-aligned locale name. + */ + public function isoLocale(): string + { + return $this->isoName; + } + + /** + * Returns a string suitable for the HTML "lang" attribute. + */ + public function htmlLang(): string + { + return str_replace('_', '-', $this->isoName); + } + + /** + * Returns a string suitable for the HTML "dir" attribute. + */ + public function htmlDirection(): string + { + return $this->isRtl ? 'rtl' : 'ltr'; + } +} diff --git a/app/Translation/LanguageManager.php b/app/Translation/LocaleManager.php similarity index 70% rename from app/Translation/LanguageManager.php rename to app/Translation/LocaleManager.php index f3432d038..171555293 100644 --- a/app/Translation/LanguageManager.php +++ b/app/Translation/LocaleManager.php @@ -3,17 +3,18 @@ namespace BookStack\Translation; use BookStack\Users\Models\User; +use Carbon\Carbon; use Illuminate\Http\Request; -class LanguageManager +class LocaleManager { /** - * Array of right-to-left language options. + * Array of right-to-left locale options. */ - protected array $rtlLanguages = ['ar', 'fa', 'he']; + protected array $rtlLocales = ['ar', 'fa', 'he']; /** - * Map of BookStack language names to best-estimate ISO and windows locale names. + * Map of BookStack locale names to best-estimate ISO and windows locale names. * Locales can often be found by running `locale -a` on a linux system. * Windows locales can be found at: * https://docs.microsoft.com/en-us/cpp/c-runtime-library/language-strings?view=msvc-170. @@ -29,8 +30,8 @@ class LanguageManager 'da' => ['iso' => 'da_DK', 'windows' => 'Danish'], 'de' => ['iso' => 'de_DE', 'windows' => 'German'], 'de_informal' => ['iso' => 'de_DE', 'windows' => 'German'], - 'en' => ['iso' => 'en_GB', 'windows' => 'English'], 'el' => ['iso' => 'el_GR', 'windows' => 'Greek'], + 'en' => ['iso' => 'en_GB', 'windows' => 'English'], 'es' => ['iso' => 'es_ES', 'windows' => 'Spanish'], 'es_AR' => ['iso' => 'es_AR', 'windows' => 'Spanish'], 'et' => ['iso' => 'et_EE', 'windows' => 'Estonian'], @@ -46,8 +47,8 @@ class LanguageManager 'ko' => ['iso' => 'ko_KR', 'windows' => 'Korean'], 'lt' => ['iso' => 'lt_LT', 'windows' => 'Lithuanian'], 'lv' => ['iso' => 'lv_LV', 'windows' => 'Latvian'], - 'nl' => ['iso' => 'nl_NL', 'windows' => 'Dutch'], 'nb' => ['iso' => 'nb_NO', 'windows' => 'Norwegian (Bokmal)'], + 'nl' => ['iso' => 'nl_NL', 'windows' => 'Dutch'], 'pl' => ['iso' => 'pl_PL', 'windows' => 'Polish'], 'pt' => ['iso' => 'pt_PT', 'windows' => 'Portuguese'], 'pt_BR' => ['iso' => 'pt_BR', 'windows' => 'Portuguese'], @@ -56,47 +57,40 @@ class LanguageManager 'sk' => ['iso' => 'sk_SK', 'windows' => 'Slovak'], 'sl' => ['iso' => 'sl_SI', 'windows' => 'Slovenian'], 'sv' => ['iso' => 'sv_SE', 'windows' => 'Swedish'], + 'tr' => ['iso' => 'tr_TR', 'windows' => 'Turkish'], 'uk' => ['iso' => 'uk_UA', 'windows' => 'Ukrainian'], 'uz' => ['iso' => 'uz_UZ', 'windows' => 'Uzbek'], 'vi' => ['iso' => 'vi_VN', 'windows' => 'Vietnamese'], 'zh_CN' => ['iso' => 'zh_CN', 'windows' => 'Chinese (Simplified)'], 'zh_TW' => ['iso' => 'zh_TW', 'windows' => 'Chinese (Traditional)'], - 'tr' => ['iso' => 'tr_TR', 'windows' => 'Turkish'], ]; /** - * Get the language specifically for the currently logged-in user if available. + * Get the BookStack locale string for the given user. */ - public function getUserLanguage(Request $request, string $default): string + protected function getLocaleForUser(User $user): string { - try { - $user = user(); - } catch (\Exception $exception) { - return $default; - } + $default = config('app.default_locale'); if ($user->isGuest() && config('app.auto_detect_locale')) { - return $this->autoDetectLocale($request, $default); + return $this->autoDetectLocale(request(), $default); } return setting()->getUser($user, 'language', $default); } /** - * Get the language for the given user. + * Get a locale definition for the current user. */ - public function getLanguageForUser(User $user): string + public function getForUser(User $user): LocaleDefinition { - $default = config('app.locale'); - return setting()->getUser($user, 'language', $default); - } + $localeString = $this->getLocaleForUser($user); - /** - * Check if the given BookStack language value is a right-to-left language. - */ - public function isRTL(string $language): bool - { - return in_array($language, $this->rtlLanguages); + return new LocaleDefinition( + $localeString, + $this->getIsoName($localeString), + in_array($localeString, $this->rtlLocales), + ); } /** @@ -105,7 +99,8 @@ class LanguageManager */ protected function autoDetectLocale(Request $request, string $default): string { - $availableLocales = config('app.locales'); + $availableLocales = array_keys($this->localeMap); + foreach ($request->getLanguages() as $lang) { if (in_array($lang, $availableLocales)) { return $lang; @@ -116,29 +111,40 @@ class LanguageManager } /** - * Get the ISO version of a BookStack language name. + * Get the ISO version of a BookStack locale. */ - public function getIsoName(string $language): string + protected function getIsoName(string $locale): string { - return $this->localeMap[$language]['iso'] ?? $language; + return $this->localeMap[$locale]['iso'] ?? $locale; + } + + /** + * Sets the active locale for system level components. + */ + public function setAppLocale(LocaleDefinition $locale): void + { + app()->setLocale($locale->appLocale()); + Carbon::setLocale($locale->isoLocale()); + $this->setPhpDateTimeLocale($locale); } /** * Set the system date locale for localized date formatting. * Will try both the standard locale name and the UTF8 variant. */ - public function setPhpDateTimeLocale(string $language): void + public function setPhpDateTimeLocale(LocaleDefinition $locale): void { - $isoLang = $this->localeMap[$language]['iso'] ?? ''; - $isoLangPrefix = explode('_', $isoLang)[0]; + $appLocale = $locale->appLocale(); + $isoLocale = $this->localeMap[$appLocale]['iso'] ?? ''; + $isoLocalePrefix = explode('_', $isoLocale)[0]; $locales = array_values(array_filter([ - $isoLang ? $isoLang . '.utf8' : false, - $isoLang ?: false, - $isoLang ? str_replace('_', '-', $isoLang) : false, - $isoLang ? $isoLangPrefix . '.UTF-8' : false, - $this->localeMap[$language]['windows'] ?? false, - $language, + $isoLocale ? $isoLocale . '.utf8' : false, + $isoLocale ?: false, + $isoLocale ? str_replace('_', '-', $isoLocale) : false, + $isoLocale ? $isoLocalePrefix . '.UTF-8' : false, + $this->localeMap[$appLocale]['windows'] ?? false, + $appLocale, ])); if (!empty($locales)) { diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 232ea8832..78411e0d4 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -12,7 +12,7 @@ use BookStack\Api\ApiToken; use BookStack\App\Model; use BookStack\App\Sluggable; use BookStack\Entities\Tools\SlugGenerator; -use BookStack\Translation\LanguageManager; +use BookStack\Translation\LocaleManager; use BookStack\Uploads\Image; use Carbon\Carbon; use Exception; @@ -346,7 +346,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon */ public function getLanguage(): string { - return app()->make(LanguageManager::class)->getLanguageForUser($this); + return app()->make(LocaleManager::class)->getForUser($this)->appLocale(); } /** diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index 8875788a6..0fd12b70f 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -1,6 +1,6 @@ - {{ isset($pageTitle) ? $pageTitle . ' | ' : '' }}{{ setting('app-name') }} diff --git a/resources/views/layouts/export.blade.php b/resources/views/layouts/export.blade.php index e041d8dea..eb2397a75 100644 --- a/resources/views/layouts/export.blade.php +++ b/resources/views/layouts/export.blade.php @@ -1,5 +1,5 @@ - + @yield('title') diff --git a/resources/views/layouts/plain.blade.php b/resources/views/layouts/plain.blade.php index 043d8aa48..360c404c1 100644 --- a/resources/views/layouts/plain.blade.php +++ b/resources/views/layouts/plain.blade.php @@ -1,6 +1,6 @@ - {{ isset($pageTitle) ? $pageTitle . ' | ' : '' }}{{ setting('app-name') }} diff --git a/resources/views/pages/parts/markdown-editor.blade.php b/resources/views/pages/parts/markdown-editor.blade.php index c488f0e11..18a418f10 100644 --- a/resources/views/pages/parts/markdown-editor.blade.php +++ b/resources/views/pages/parts/markdown-editor.blade.php @@ -1,6 +1,6 @@
    diff --git a/resources/views/pages/parts/wysiwyg-editor.blade.php b/resources/views/pages/parts/wysiwyg-editor.blade.php index b7cd1bdaa..ca6b6da8a 100644 --- a/resources/views/pages/parts/wysiwyg-editor.blade.php +++ b/resources/views/pages/parts/wysiwyg-editor.blade.php @@ -3,9 +3,9 @@ @endpush
    diff --git a/resources/views/users/create.blade.php b/resources/views/users/create.blade.php index 540d7bd6a..0edae1d82 100644 --- a/resources/views/users/create.blade.php +++ b/resources/views/users/create.blade.php @@ -14,7 +14,7 @@
    @include('users.parts.form') - @include('users.parts.language-option-row', ['value' => old('setting.language') ?? config('app.default_locale')]) + @include('users.parts.language-option-row', ['value' => old('setting.language') ?? config('app.locale')])
    diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 4e31e785d..23cf87684 100644 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -33,7 +33,7 @@
    - @include('users.parts.language-option-row', ['value' => setting()->getUser($user, 'language', config('app.default_locale'))]) + @include('users.parts.language-option-row', ['value' => $user->getLanguage())])
    diff --git a/resources/views/vendor/notifications/email.blade.php b/resources/views/vendor/notifications/email.blade.php index f5d9c328d..1b81c6fc6 100644 --- a/resources/views/vendor/notifications/email.blade.php +++ b/resources/views/vendor/notifications/email.blade.php @@ -1,5 +1,5 @@ - + diff --git a/tests/LanguageTest.php b/tests/LanguageTest.php index a66227ff2..b6a7d1e87 100644 --- a/tests/LanguageTest.php +++ b/tests/LanguageTest.php @@ -78,6 +78,7 @@ class LanguageTest extends TestCase public function test_rtl_config_set_if_lang_is_rtl() { $this->asEditor(); + // TODO - Alter $this->assertFalse(config('app.rtl'), 'App RTL config should be false by default'); setting()->putUser($this->users->editor(), 'language', 'ar'); $this->get('/'); From 8994c1b9d9dca616c74cdcdb17cc4722203e3299 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 Sep 2023 16:20:21 +0100 Subject: [PATCH 27/97] Locales: More use of locale objects, Addressed failing tests --- .../Notifications/UserInviteNotification.php | 12 +++++------ .../Messages/BaseActivityNotification.php | 7 ++++--- .../Messages/CommentCreationNotification.php | 18 ++++++++--------- .../Messages/PageCreationNotification.php | 16 +++++++-------- .../Messages/PageUpdateNotification.php | 18 ++++++++--------- app/App/MailNotification.php | 5 +++-- app/Translation/LocaleDefinition.php | 8 ++++++++ app/Translation/LocaleManager.php | 12 ++++++++++- app/Users/Models/User.php | 7 ++++--- resources/views/layouts/base.blade.php | 4 ++-- resources/views/layouts/export.blade.php | 2 +- resources/views/layouts/plain.blade.php | 4 ++-- resources/views/users/create.blade.php | 2 +- resources/views/users/edit.blade.php | 17 ++++++++++------ .../vendor/notifications/email.blade.php | 4 ++-- tests/LanguageTest.php | 20 +++++++++---------- tests/User/UserManagementTest.php | 2 +- 17 files changed, 92 insertions(+), 66 deletions(-) diff --git a/app/Access/Notifications/UserInviteNotification.php b/app/Access/Notifications/UserInviteNotification.php index b453fc95d..dacce574f 100644 --- a/app/Access/Notifications/UserInviteNotification.php +++ b/app/Access/Notifications/UserInviteNotification.php @@ -16,12 +16,12 @@ class UserInviteNotification extends MailNotification public function toMail(User $notifiable): MailMessage { $appName = ['appName' => setting('app-name')]; - $language = $notifiable->getLanguage(); + $locale = $notifiable->getLocale(); - return $this->newMailMessage($language) - ->subject(trans('auth.user_invite_email_subject', $appName, $language)) - ->greeting(trans('auth.user_invite_email_greeting', $appName, $language)) - ->line(trans('auth.user_invite_email_text', [], $language)) - ->action(trans('auth.user_invite_email_action', [], $language), url('/register/invite/' . $this->token)); + return $this->newMailMessage($locale) + ->subject($locale->trans('auth.user_invite_email_subject', $appName)) + ->greeting($locale->trans('auth.user_invite_email_greeting', $appName)) + ->line($locale->trans('auth.user_invite_email_text')) + ->action($locale->trans('auth.user_invite_email_action'), url('/register/invite/' . $this->token)); } } diff --git a/app/Activity/Notifications/Messages/BaseActivityNotification.php b/app/Activity/Notifications/Messages/BaseActivityNotification.php index 414859091..322df5d94 100644 --- a/app/Activity/Notifications/Messages/BaseActivityNotification.php +++ b/app/Activity/Notifications/Messages/BaseActivityNotification.php @@ -5,6 +5,7 @@ namespace BookStack\Activity\Notifications\Messages; use BookStack\Activity\Models\Loggable; use BookStack\Activity\Notifications\MessageParts\LinkedMailMessageLine; use BookStack\App\MailNotification; +use BookStack\Translation\LocaleDefinition; use BookStack\Users\Models\User; use Illuminate\Bus\Queueable; @@ -35,12 +36,12 @@ abstract class BaseActivityNotification extends MailNotification /** * Build the common reason footer line used in mail messages. */ - protected function buildReasonFooterLine(string $language): LinkedMailMessageLine + protected function buildReasonFooterLine(LocaleDefinition $locale): LinkedMailMessageLine { return new LinkedMailMessageLine( url('/preferences/notifications'), - trans('notifications.footer_reason', [], $language), - trans('notifications.footer_reason_link', [], $language), + $locale->trans('notifications.footer_reason'), + $locale->trans('notifications.footer_reason_link'), ); } } diff --git a/app/Activity/Notifications/Messages/CommentCreationNotification.php b/app/Activity/Notifications/Messages/CommentCreationNotification.php index 5db61b04b..094ab30b7 100644 --- a/app/Activity/Notifications/Messages/CommentCreationNotification.php +++ b/app/Activity/Notifications/Messages/CommentCreationNotification.php @@ -17,17 +17,17 @@ class CommentCreationNotification extends BaseActivityNotification /** @var Page $page */ $page = $comment->entity; - $language = $notifiable->getLanguage(); + $locale = $notifiable->getLocale(); - return $this->newMailMessage($language) - ->subject(trans('notifications.new_comment_subject', ['pageName' => $page->getShortName()], $language)) - ->line(trans('notifications.new_comment_intro', ['appName' => setting('app-name')], $language)) + return $this->newMailMessage($locale) + ->subject($locale->trans('notifications.new_comment_subject', ['pageName' => $page->getShortName()])) + ->line($locale->trans('notifications.new_comment_intro', ['appName' => setting('app-name')])) ->line(new ListMessageLine([ - trans('notifications.detail_page_name', [], $language) => $page->name, - trans('notifications.detail_commenter', [], $language) => $this->user->name, - trans('notifications.detail_comment', [], $language) => strip_tags($comment->html), + $locale->trans('notifications.detail_page_name') => $page->name, + $locale->trans('notifications.detail_commenter') => $this->user->name, + $locale->trans('notifications.detail_comment') => strip_tags($comment->html), ])) - ->action(trans('notifications.action_view_comment', [], $language), $page->getUrl('#comment' . $comment->local_id)) - ->line($this->buildReasonFooterLine($language)); + ->action($locale->trans('notifications.action_view_comment'), $page->getUrl('#comment' . $comment->local_id)) + ->line($this->buildReasonFooterLine($locale)); } } diff --git a/app/Activity/Notifications/Messages/PageCreationNotification.php b/app/Activity/Notifications/Messages/PageCreationNotification.php index 110829469..da028aa8c 100644 --- a/app/Activity/Notifications/Messages/PageCreationNotification.php +++ b/app/Activity/Notifications/Messages/PageCreationNotification.php @@ -14,16 +14,16 @@ class PageCreationNotification extends BaseActivityNotification /** @var Page $page */ $page = $this->detail; - $language = $notifiable->getLanguage(); + $locale = $notifiable->getLocale(); - return $this->newMailMessage($language) - ->subject(trans('notifications.new_page_subject', ['pageName' => $page->getShortName()], $language)) - ->line(trans('notifications.new_page_intro', ['appName' => setting('app-name')], $language)) + return $this->newMailMessage($locale) + ->subject($locale->trans('notifications.new_page_subject', ['pageName' => $page->getShortName()])) + ->line($locale->trans('notifications.new_page_intro', ['appName' => setting('app-name')], $locale)) ->line(new ListMessageLine([ - trans('notifications.detail_page_name', [], $language) => $page->name, - trans('notifications.detail_created_by', [], $language) => $this->user->name, + $locale->trans('notifications.detail_page_name') => $page->name, + $locale->trans('notifications.detail_created_by') => $this->user->name, ])) - ->action(trans('notifications.action_view_page', [], $language), $page->getUrl()) - ->line($this->buildReasonFooterLine($language)); + ->action($locale->trans('notifications.action_view_page'), $page->getUrl()) + ->line($this->buildReasonFooterLine($locale)); } } diff --git a/app/Activity/Notifications/Messages/PageUpdateNotification.php b/app/Activity/Notifications/Messages/PageUpdateNotification.php index 8e2d27fa4..1c8155d29 100644 --- a/app/Activity/Notifications/Messages/PageUpdateNotification.php +++ b/app/Activity/Notifications/Messages/PageUpdateNotification.php @@ -14,17 +14,17 @@ class PageUpdateNotification extends BaseActivityNotification /** @var Page $page */ $page = $this->detail; - $language = $notifiable->getLanguage(); + $locale = $notifiable->getLocale(); - return $this->newMailMessage($language) - ->subject(trans('notifications.updated_page_subject', ['pageName' => $page->getShortName()], $language)) - ->line(trans('notifications.updated_page_intro', ['appName' => setting('app-name')], $language)) + return $this->newMailMessage($locale) + ->subject($locale->trans('notifications.updated_page_subject', ['pageName' => $page->getShortName()])) + ->line($locale->trans('notifications.updated_page_intro', ['appName' => setting('app-name')])) ->line(new ListMessageLine([ - trans('notifications.detail_page_name', [], $language) => $page->name, - trans('notifications.detail_updated_by', [], $language) => $this->user->name, + $locale->trans('notifications.detail_page_name') => $page->name, + $locale->trans('notifications.detail_updated_by') => $this->user->name, ])) - ->line(trans('notifications.updated_page_debounce', [], $language)) - ->action(trans('notifications.action_view_page', [], $language), $page->getUrl()) - ->line($this->buildReasonFooterLine($language)); + ->line($locale->trans('notifications.updated_page_debounce')) + ->action($locale->trans('notifications.action_view_page'), $page->getUrl()) + ->line($this->buildReasonFooterLine($locale)); } } diff --git a/app/App/MailNotification.php b/app/App/MailNotification.php index 8c57b5621..50b7f69a7 100644 --- a/app/App/MailNotification.php +++ b/app/App/MailNotification.php @@ -2,6 +2,7 @@ namespace BookStack\App; +use BookStack\Translation\LocaleDefinition; use BookStack\Users\Models\User; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; @@ -32,9 +33,9 @@ abstract class MailNotification extends Notification implements ShouldQueue /** * Create a new mail message. */ - protected function newMailMessage(string $language = ''): MailMessage + protected function newMailMessage(?LocaleDefinition $locale = null): MailMessage { - $data = ['language' => $language ?: null]; + $data = ['locale' => $locale ?? user()->getLocale()]; return (new MailMessage())->view([ 'html' => 'vendor.notifications.email', diff --git a/app/Translation/LocaleDefinition.php b/app/Translation/LocaleDefinition.php index fe8640109..85d36afa0 100644 --- a/app/Translation/LocaleDefinition.php +++ b/app/Translation/LocaleDefinition.php @@ -42,4 +42,12 @@ class LocaleDefinition { return $this->isRtl ? 'rtl' : 'ltr'; } + + /** + * Translate using this locate. + */ + public function trans(string $key, array $replace = []): string + { + return trans($key, $replace, $this->appLocale()); + } } diff --git a/app/Translation/LocaleManager.php b/app/Translation/LocaleManager.php index 171555293..cf93aff06 100644 --- a/app/Translation/LocaleManager.php +++ b/app/Translation/LocaleManager.php @@ -27,6 +27,7 @@ class LocaleManager 'bs' => ['iso' => 'bs_BA', 'windows' => 'Bosnian (Latin)'], 'ca' => ['iso' => 'ca', 'windows' => 'Catalan'], 'cs' => ['iso' => 'cs_CZ', 'windows' => 'Czech'], + 'cy' => ['iso' => 'cy_GB', 'windows' => 'Welsh'], 'da' => ['iso' => 'da_DK', 'windows' => 'Danish'], 'de' => ['iso' => 'de_DE', 'windows' => 'German'], 'de_informal' => ['iso' => 'de_DE', 'windows' => 'German'], @@ -44,6 +45,7 @@ class LocaleManager 'id' => ['iso' => 'id_ID', 'windows' => 'Indonesian'], 'it' => ['iso' => 'it_IT', 'windows' => 'Italian'], 'ja' => ['iso' => 'ja', 'windows' => 'Japanese'], + 'ka' => ['iso' => 'ka_GE', 'windows' => 'Georgian'], 'ko' => ['iso' => 'ko_KR', 'windows' => 'Korean'], 'lt' => ['iso' => 'lt_LT', 'windows' => 'Lithuanian'], 'lv' => ['iso' => 'lv_LV', 'windows' => 'Latvian'], @@ -99,7 +101,7 @@ class LocaleManager */ protected function autoDetectLocale(Request $request, string $default): string { - $availableLocales = array_keys($this->localeMap); + $availableLocales = $this->getAllAppLocales(); foreach ($request->getLanguages() as $lang) { if (in_array($lang, $availableLocales)) { @@ -151,4 +153,12 @@ class LocaleManager setlocale(LC_TIME, $locales[0], ...array_slice($locales, 1)); } } + + /** + * Get all the available app-specific level locale strings. + */ + public function getAllAppLocales(): array + { + return array_keys($this->localeMap); + } } diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 78411e0d4..39236c7e4 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -12,6 +12,7 @@ use BookStack\Api\ApiToken; use BookStack\App\Model; use BookStack\App\Sluggable; use BookStack\Entities\Tools\SlugGenerator; +use BookStack\Translation\LocaleDefinition; use BookStack\Translation\LocaleManager; use BookStack\Uploads\Image; use Carbon\Carbon; @@ -342,11 +343,11 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon } /** - * Get the system language for this user. + * Get the locale for this user. */ - public function getLanguage(): string + public function getLocale(): LocaleDefinition { - return app()->make(LocaleManager::class)->getForUser($this)->appLocale(); + return app()->make(LocaleManager::class)->getForUser($this); } /** diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index 0fd12b70f..ca8570d36 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -1,6 +1,6 @@ - {{ isset($pageTitle) ? $pageTitle . ' | ' : '' }}{{ setting('app-name') }} diff --git a/resources/views/layouts/export.blade.php b/resources/views/layouts/export.blade.php index eb2397a75..c2c3880e4 100644 --- a/resources/views/layouts/export.blade.php +++ b/resources/views/layouts/export.blade.php @@ -1,5 +1,5 @@ - + @yield('title') diff --git a/resources/views/layouts/plain.blade.php b/resources/views/layouts/plain.blade.php index 360c404c1..7ce9078e2 100644 --- a/resources/views/layouts/plain.blade.php +++ b/resources/views/layouts/plain.blade.php @@ -1,6 +1,6 @@ - {{ isset($pageTitle) ? $pageTitle . ' | ' : '' }}{{ setting('app-name') }} diff --git a/resources/views/users/create.blade.php b/resources/views/users/create.blade.php index 0edae1d82..dafc623e1 100644 --- a/resources/views/users/create.blade.php +++ b/resources/views/users/create.blade.php @@ -14,7 +14,7 @@
    @include('users.parts.form') - @include('users.parts.language-option-row', ['value' => old('setting.language') ?? config('app.locale')]) + @include('users.parts.language-option-row', ['value' => old('language') ?? config('app.default_locale')])
    diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 23cf87684..832186930 100644 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -16,7 +16,8 @@
    - +

    {{ trans('settings.users_avatar_desc') }}

    @@ -33,13 +34,15 @@
    - @include('users.parts.language-option-row', ['value' => $user->getLanguage())]) + @include('users.parts.language-option-row', ['value' => old('language') ?? $user->getLocale()->appLocale()])
    @@ -60,7 +63,8 @@
    @@ -84,7 +88,8 @@ class="button small outline">{{ trans('settings.users_social_disconnect') }} @else - {{ trans('settings.users_social_connect') }} @endif
    diff --git a/resources/views/vendor/notifications/email.blade.php b/resources/views/vendor/notifications/email.blade.php index 1b81c6fc6..8e922fba5 100644 --- a/resources/views/vendor/notifications/email.blade.php +++ b/resources/views/vendor/notifications/email.blade.php @@ -159,7 +159,7 @@ $style = [

    - {{ trans('common.email_action_help', ['actionText' => $actionText], $language) }} + {{ $locale->trans('common.email_action_help', ['actionText' => $actionText]) }}

    @@ -187,7 +187,7 @@ $style = [

    © {{ date('Y') }} {{ setting('app-name') }}. - {{ trans('common.email_rights', [], $language) }} + {{ $locale->trans('common.email_rights') }}

    diff --git a/tests/LanguageTest.php b/tests/LanguageTest.php index b6a7d1e87..6b6856184 100644 --- a/tests/LanguageTest.php +++ b/tests/LanguageTest.php @@ -3,6 +3,7 @@ namespace Tests; use BookStack\Activity\ActivityType; +use BookStack\Translation\LocaleManager; class LanguageTest extends TestCase { @@ -17,12 +18,12 @@ class LanguageTest extends TestCase $this->langs = array_diff(scandir(lang_path('')), ['..', '.']); } - public function test_locales_config_key_set_properly() + public function test_locales_list_set_properly() { - $configLocales = config('app.locales'); - sort($configLocales); + $appLocales = $this->app->make(LocaleManager::class)->getAllAppLocales(); + sort($appLocales); sort($this->langs); - $this->assertEquals(implode(':', $configLocales), implode(':', $this->langs), 'app.locales configuration variable does not match those found in lang files'); + $this->assertEquals(implode(':', $this->langs), implode(':', $appLocales), 'app.locales configuration variable does not match those found in lang files'); } // Not part of standard phpunit test runs since we sometimes expect non-added langs. @@ -75,14 +76,13 @@ class LanguageTest extends TestCase } } - public function test_rtl_config_set_if_lang_is_rtl() + public function test_views_use_rtl_if_rtl_language_is_set() { - $this->asEditor(); - // TODO - Alter - $this->assertFalse(config('app.rtl'), 'App RTL config should be false by default'); + $this->asEditor()->withHtml($this->get('/'))->assertElementExists('html[dir="ltr"]'); + setting()->putUser($this->users->editor(), 'language', 'ar'); - $this->get('/'); - $this->assertTrue(config('app.rtl'), 'App RTL config should have been set to true by middleware'); + + $this->withHtml($this->get('/'))->assertElementExists('html[dir="rtl"]'); } public function test_unknown_lang_does_not_break_app() diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php index a6d869b2f..93d35f5d0 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -215,7 +215,7 @@ class UserManagementTest extends TestCase { $langs = ['en', 'fr', 'hr']; foreach ($langs as $lang) { - config()->set('app.locale', $lang); + config()->set('app.default_locale', $lang); $resp = $this->asAdmin()->get('/settings/users/create'); $this->withHtml($resp)->assertElementExists('select[name="language"] option[value="' . $lang . '"][selected]'); } From b42e8cdb638c711a42d7d7df52238f92c537f2dc Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 Sep 2023 17:35:00 +0100 Subject: [PATCH 28/97] Locales: Fixed errors occuring for PHP < 8.2 --- app/Entities/Tools/ExportFormatter.php | 39 ++++++++++++------------ resources/views/layouts/base.blade.php | 4 +-- resources/views/layouts/export.blade.php | 2 +- resources/views/layouts/plain.blade.php | 4 +-- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/app/Entities/Tools/ExportFormatter.php b/app/Entities/Tools/ExportFormatter.php index 9e4d63cf7..6779797d1 100644 --- a/app/Entities/Tools/ExportFormatter.php +++ b/app/Entities/Tools/ExportFormatter.php @@ -16,18 +16,11 @@ use Throwable; class ExportFormatter { - protected ImageService $imageService; - protected PdfGenerator $pdfGenerator; - protected CspService $cspService; - - /** - * ExportService constructor. - */ - public function __construct(ImageService $imageService, PdfGenerator $pdfGenerator, CspService $cspService) - { - $this->imageService = $imageService; - $this->pdfGenerator = $pdfGenerator; - $this->cspService = $cspService; + public function __construct( + protected ImageService $imageService, + protected PdfGenerator $pdfGenerator, + protected CspService $cspService + ) { } /** @@ -36,13 +29,14 @@ class ExportFormatter * * @throws Throwable */ - public function pageToContainedHtml(Page $page) + public function pageToContainedHtml(Page $page): string { $page->html = (new PageContent($page))->render(); $pageHtml = view('exports.page', [ 'page' => $page, 'format' => 'html', 'cspContent' => $this->cspService->getCspMetaTagValue(), + 'locale' => user()->getLocale(), ])->render(); return $this->containHtml($pageHtml); @@ -53,7 +47,7 @@ class ExportFormatter * * @throws Throwable */ - public function chapterToContainedHtml(Chapter $chapter) + public function chapterToContainedHtml(Chapter $chapter): string { $pages = $chapter->getVisiblePages(); $pages->each(function ($page) { @@ -64,6 +58,7 @@ class ExportFormatter 'pages' => $pages, 'format' => 'html', 'cspContent' => $this->cspService->getCspMetaTagValue(), + 'locale' => user()->getLocale(), ])->render(); return $this->containHtml($html); @@ -74,7 +69,7 @@ class ExportFormatter * * @throws Throwable */ - public function bookToContainedHtml(Book $book) + public function bookToContainedHtml(Book $book): string { $bookTree = (new BookContents($book))->getTree(false, true); $html = view('exports.book', [ @@ -82,6 +77,7 @@ class ExportFormatter 'bookChildren' => $bookTree, 'format' => 'html', 'cspContent' => $this->cspService->getCspMetaTagValue(), + 'locale' => user()->getLocale(), ])->render(); return $this->containHtml($html); @@ -92,13 +88,14 @@ class ExportFormatter * * @throws Throwable */ - public function pageToPdf(Page $page) + public function pageToPdf(Page $page): string { $page->html = (new PageContent($page))->render(); $html = view('exports.page', [ 'page' => $page, 'format' => 'pdf', 'engine' => $this->pdfGenerator->getActiveEngine(), + 'locale' => user()->getLocale(), ])->render(); return $this->htmlToPdf($html); @@ -109,7 +106,7 @@ class ExportFormatter * * @throws Throwable */ - public function chapterToPdf(Chapter $chapter) + public function chapterToPdf(Chapter $chapter): string { $pages = $chapter->getVisiblePages(); $pages->each(function ($page) { @@ -121,6 +118,7 @@ class ExportFormatter 'pages' => $pages, 'format' => 'pdf', 'engine' => $this->pdfGenerator->getActiveEngine(), + 'locale' => user()->getLocale(), ])->render(); return $this->htmlToPdf($html); @@ -131,7 +129,7 @@ class ExportFormatter * * @throws Throwable */ - public function bookToPdf(Book $book) + public function bookToPdf(Book $book): string { $bookTree = (new BookContents($book))->getTree(false, true); $html = view('exports.book', [ @@ -139,6 +137,7 @@ class ExportFormatter 'bookChildren' => $bookTree, 'format' => 'pdf', 'engine' => $this->pdfGenerator->getActiveEngine(), + 'locale' => user()->getLocale(), ])->render(); return $this->htmlToPdf($html); @@ -194,7 +193,7 @@ class ExportFormatter /** @var DOMElement $iframe */ foreach ($iframes as $iframe) { $link = $iframe->getAttribute('src'); - if (strpos($link, '//') === 0) { + if (str_starts_with($link, '//')) { $link = 'https:' . $link; } @@ -240,7 +239,7 @@ class ExportFormatter foreach ($linksOutput[0] as $index => $linkMatch) { $oldLinkString = $linkMatch; $srcString = $linksOutput[2][$index]; - if (strpos(trim($srcString), 'http') !== 0) { + if (!str_starts_with(trim($srcString), 'http')) { $newSrcString = url($srcString); $newLinkString = str_replace($srcString, $newSrcString, $oldLinkString); $htmlContent = str_replace($oldLinkString, $newLinkString, $htmlContent); diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index ca8570d36..f303aff26 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -1,6 +1,6 @@ - {{ isset($pageTitle) ? $pageTitle . ' | ' : '' }}{{ setting('app-name') }} diff --git a/resources/views/layouts/export.blade.php b/resources/views/layouts/export.blade.php index c2c3880e4..eb2397a75 100644 --- a/resources/views/layouts/export.blade.php +++ b/resources/views/layouts/export.blade.php @@ -1,5 +1,5 @@ - + @yield('title') diff --git a/resources/views/layouts/plain.blade.php b/resources/views/layouts/plain.blade.php index 7ce9078e2..a3ee74143 100644 --- a/resources/views/layouts/plain.blade.php +++ b/resources/views/layouts/plain.blade.php @@ -1,6 +1,6 @@ - {{ isset($pageTitle) ? $pageTitle . ' | ' : '' }}{{ setting('app-name') }} From baa957d98049380ff2278f222600852f70b79712 Mon Sep 17 00:00:00 2001 From: Tushar Nain <100490977+tusharnain4578@users.noreply.github.com> Date: Sun, 17 Sep 2023 23:31:01 +0530 Subject: [PATCH 29/97] Update UserPreferencesTest.php Added Testcases for preferences menu of Comment Notifications visibility when comments are enabled/disabled. --- tests/User/UserPreferencesTest.php | 31 ++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/User/UserPreferencesTest.php b/tests/User/UserPreferencesTest.php index f5dae3e76..30e7bb540 100644 --- a/tests/User/UserPreferencesTest.php +++ b/tests/User/UserPreferencesTest.php @@ -318,4 +318,35 @@ class UserPreferencesTest extends TestCase $resp = $this->get($page->getUrl('/edit')); $resp->assertSee('option:code-editor:favourites="javascript,ruby"', false); } + + public function test_comment_notifications_hidden_when_comments_disabled() + { + $editor = $this->users->editor(); + + + setting()->putUser($editor, 'app-disable-comments', true); + + $settingLabel1 = trans('preferences.notifications_opt_own_page_comments'); + $settingLabel2 = trans('preferences.notifications_opt_comment_replies'); + + $resp = $this->actingAs($editor)->get('/preferences/notifications'); + + $resp->assertDontSee($settingLabel1, true); + $resp->assertDontSee($settingLabel2, true); + } + + public function test_comment_notifications_visible_when_comments_enabled() + { + $editor = $this->users->editor(); + + setting()->putUser($editor, 'app-disable-comments', false); + + $settingLabel1 = trans('preferences.notifications_opt_own_page_comments'); + $settingLabel2 = trans('preferences.notifications_opt_comment_replies'); + + $resp = $this->actingAs($editor)->get('/preferences/notifications'); + + $resp->assertSee($settingLabel1, true); + $resp->assertSee($settingLabel2, true); + } } From 78bf11cf653ea9a9268ccb013d444ff60fd375c5 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 Sep 2023 22:02:12 +0100 Subject: [PATCH 30/97] Locales: Removed a lot of existing locale handling There was a lot of locale handling to get correct/expected date formatting within the app. Carbon now has built-in locale content rather than us needing to target specific system locales. This also removes setting locale via Carbon directly. Carbon registers its own Laravel service provider which seems to accurately pull the correct locale from the app. For #4555 --- app/Http/Middleware/Localization.php | 2 +- app/Translation/LocaleManager.php | 137 +++++++++------------------ 2 files changed, 47 insertions(+), 92 deletions(-) diff --git a/app/Http/Middleware/Localization.php b/app/Http/Middleware/Localization.php index 0be0b77eb..94d06a627 100644 --- a/app/Http/Middleware/Localization.php +++ b/app/Http/Middleware/Localization.php @@ -27,7 +27,7 @@ class Localization view()->share('locale', $userLocale); // Set locale for system components - $this->localeManager->setAppLocale($userLocale); + app()->setLocale($userLocale->appLocale()); return $next($request); } diff --git a/app/Translation/LocaleManager.php b/app/Translation/LocaleManager.php index cf93aff06..825eae7a4 100644 --- a/app/Translation/LocaleManager.php +++ b/app/Translation/LocaleManager.php @@ -3,7 +3,6 @@ namespace BookStack\Translation; use BookStack\Users\Models\User; -use Carbon\Carbon; use Illuminate\Http\Request; class LocaleManager @@ -14,57 +13,55 @@ class LocaleManager protected array $rtlLocales = ['ar', 'fa', 'he']; /** - * Map of BookStack locale names to best-estimate ISO and windows locale names. + * Map of BookStack locale names to best-estimate ISO locale names. * Locales can often be found by running `locale -a` on a linux system. - * Windows locales can be found at: - * https://docs.microsoft.com/en-us/cpp/c-runtime-library/language-strings?view=msvc-170. * - * @var array + * @var array */ protected array $localeMap = [ - 'ar' => ['iso' => 'ar', 'windows' => 'Arabic'], - 'bg' => ['iso' => 'bg_BG', 'windows' => 'Bulgarian'], - 'bs' => ['iso' => 'bs_BA', 'windows' => 'Bosnian (Latin)'], - 'ca' => ['iso' => 'ca', 'windows' => 'Catalan'], - 'cs' => ['iso' => 'cs_CZ', 'windows' => 'Czech'], - 'cy' => ['iso' => 'cy_GB', 'windows' => 'Welsh'], - 'da' => ['iso' => 'da_DK', 'windows' => 'Danish'], - 'de' => ['iso' => 'de_DE', 'windows' => 'German'], - 'de_informal' => ['iso' => 'de_DE', 'windows' => 'German'], - 'el' => ['iso' => 'el_GR', 'windows' => 'Greek'], - 'en' => ['iso' => 'en_GB', 'windows' => 'English'], - 'es' => ['iso' => 'es_ES', 'windows' => 'Spanish'], - 'es_AR' => ['iso' => 'es_AR', 'windows' => 'Spanish'], - 'et' => ['iso' => 'et_EE', 'windows' => 'Estonian'], - 'eu' => ['iso' => 'eu_ES', 'windows' => 'Basque'], - 'fa' => ['iso' => 'fa_IR', 'windows' => 'Persian'], - 'fr' => ['iso' => 'fr_FR', 'windows' => 'French'], - 'he' => ['iso' => 'he_IL', 'windows' => 'Hebrew'], - 'hr' => ['iso' => 'hr_HR', 'windows' => 'Croatian'], - 'hu' => ['iso' => 'hu_HU', 'windows' => 'Hungarian'], - 'id' => ['iso' => 'id_ID', 'windows' => 'Indonesian'], - 'it' => ['iso' => 'it_IT', 'windows' => 'Italian'], - 'ja' => ['iso' => 'ja', 'windows' => 'Japanese'], - 'ka' => ['iso' => 'ka_GE', 'windows' => 'Georgian'], - 'ko' => ['iso' => 'ko_KR', 'windows' => 'Korean'], - 'lt' => ['iso' => 'lt_LT', 'windows' => 'Lithuanian'], - 'lv' => ['iso' => 'lv_LV', 'windows' => 'Latvian'], - 'nb' => ['iso' => 'nb_NO', 'windows' => 'Norwegian (Bokmal)'], - 'nl' => ['iso' => 'nl_NL', 'windows' => 'Dutch'], - 'pl' => ['iso' => 'pl_PL', 'windows' => 'Polish'], - 'pt' => ['iso' => 'pt_PT', 'windows' => 'Portuguese'], - 'pt_BR' => ['iso' => 'pt_BR', 'windows' => 'Portuguese'], - 'ro' => ['iso' => 'ro_RO', 'windows' => 'Romanian'], - 'ru' => ['iso' => 'ru', 'windows' => 'Russian'], - 'sk' => ['iso' => 'sk_SK', 'windows' => 'Slovak'], - 'sl' => ['iso' => 'sl_SI', 'windows' => 'Slovenian'], - 'sv' => ['iso' => 'sv_SE', 'windows' => 'Swedish'], - 'tr' => ['iso' => 'tr_TR', 'windows' => 'Turkish'], - 'uk' => ['iso' => 'uk_UA', 'windows' => 'Ukrainian'], - 'uz' => ['iso' => 'uz_UZ', 'windows' => 'Uzbek'], - 'vi' => ['iso' => 'vi_VN', 'windows' => 'Vietnamese'], - 'zh_CN' => ['iso' => 'zh_CN', 'windows' => 'Chinese (Simplified)'], - 'zh_TW' => ['iso' => 'zh_TW', 'windows' => 'Chinese (Traditional)'], + 'ar' => 'ar', + 'bg' => 'bg_BG', + 'bs' => 'bs_BA', + 'ca' => 'ca', + 'cs' => 'cs_CZ', + 'cy' => 'cy_GB', + 'da' => 'da_DK', + 'de' => 'de_DE', + 'de_informal' => 'de_DE', + 'el' => 'el_GR', + 'en' => 'en_GB', + 'es' => 'es_ES', + 'es_AR' => 'es_AR', + 'et' => 'et_EE', + 'eu' => 'eu_ES', + 'fa' => 'fa_IR', + 'fr' => 'fr_FR', + 'he' => 'he_IL', + 'hr' => 'hr_HR', + 'hu' => 'hu_HU', + 'id' => 'id_ID', + 'it' => 'it_IT', + 'ja' => 'ja', + 'ka' => 'ka_GE', + 'ko' => 'ko_KR', + 'lt' => 'lt_LT', + 'lv' => 'lv_LV', + 'nb' => 'nb_NO', + 'nl' => 'nl_NL', + 'pl' => 'pl_PL', + 'pt' => 'pt_PT', + 'pt_BR' => 'pt_BR', + 'ro' => 'ro_RO', + 'ru' => 'ru', + 'sk' => 'sk_SK', + 'sl' => 'sl_SI', + 'sv' => 'sv_SE', + 'tr' => 'tr_TR', + 'uk' => 'uk_UA', + 'uz' => 'uz_UZ', + 'vi' => 'vi_VN', + 'zh_CN' => 'zh_CN', + 'zh_TW' => 'zh_TW', ]; /** @@ -90,7 +87,7 @@ class LocaleManager return new LocaleDefinition( $localeString, - $this->getIsoName($localeString), + $this->localeMap[$localeString] ?? $localeString, in_array($localeString, $this->rtlLocales), ); } @@ -112,48 +109,6 @@ class LocaleManager return $default; } - /** - * Get the ISO version of a BookStack locale. - */ - protected function getIsoName(string $locale): string - { - return $this->localeMap[$locale]['iso'] ?? $locale; - } - - /** - * Sets the active locale for system level components. - */ - public function setAppLocale(LocaleDefinition $locale): void - { - app()->setLocale($locale->appLocale()); - Carbon::setLocale($locale->isoLocale()); - $this->setPhpDateTimeLocale($locale); - } - - /** - * Set the system date locale for localized date formatting. - * Will try both the standard locale name and the UTF8 variant. - */ - public function setPhpDateTimeLocale(LocaleDefinition $locale): void - { - $appLocale = $locale->appLocale(); - $isoLocale = $this->localeMap[$appLocale]['iso'] ?? ''; - $isoLocalePrefix = explode('_', $isoLocale)[0]; - - $locales = array_values(array_filter([ - $isoLocale ? $isoLocale . '.utf8' : false, - $isoLocale ?: false, - $isoLocale ? str_replace('_', '-', $isoLocale) : false, - $isoLocale ? $isoLocalePrefix . '.UTF-8' : false, - $this->localeMap[$appLocale]['windows'] ?? false, - $appLocale, - ])); - - if (!empty($locales)) { - setlocale(LC_TIME, $locales[0], ...array_slice($locales, 1)); - } - } - /** * Get all the available app-specific level locale strings. */ From c42cd29ed3e2fe1eca4c89e679197b76db699e03 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 Sep 2023 22:26:51 +0100 Subject: [PATCH 31/97] Notifications: Updated comment notif. prefs. test Combined testcases, updated to use actual text strings, and set comments setting via correct method. Made during review of #4552 --- tests/User/UserPreferencesTest.php | 44 +++++++++--------------------- 1 file changed, 13 insertions(+), 31 deletions(-) diff --git a/tests/User/UserPreferencesTest.php b/tests/User/UserPreferencesTest.php index 30e7bb540..4a6cba7b3 100644 --- a/tests/User/UserPreferencesTest.php +++ b/tests/User/UserPreferencesTest.php @@ -156,6 +156,19 @@ class UserPreferencesTest extends TestCase $this->assertPermissionError($resp); } + public function test_notification_comment_options_only_exist_if_comments_active() + { + $resp = $this->asEditor()->get('/preferences/notifications'); + $resp->assertSee('Notify upon comments'); + $resp->assertSee('Notify upon replies'); + + setting()->put('app-disable-comments', true); + + $resp = $this->get('/preferences/notifications'); + $resp->assertDontSee('Notify upon comments'); + $resp->assertDontSee('Notify upon replies'); + } + public function test_update_sort_preference() { $editor = $this->users->editor(); @@ -318,35 +331,4 @@ class UserPreferencesTest extends TestCase $resp = $this->get($page->getUrl('/edit')); $resp->assertSee('option:code-editor:favourites="javascript,ruby"', false); } - - public function test_comment_notifications_hidden_when_comments_disabled() - { - $editor = $this->users->editor(); - - - setting()->putUser($editor, 'app-disable-comments', true); - - $settingLabel1 = trans('preferences.notifications_opt_own_page_comments'); - $settingLabel2 = trans('preferences.notifications_opt_comment_replies'); - - $resp = $this->actingAs($editor)->get('/preferences/notifications'); - - $resp->assertDontSee($settingLabel1, true); - $resp->assertDontSee($settingLabel2, true); - } - - public function test_comment_notifications_visible_when_comments_enabled() - { - $editor = $this->users->editor(); - - setting()->putUser($editor, 'app-disable-comments', false); - - $settingLabel1 = trans('preferences.notifications_opt_own_page_comments'); - $settingLabel2 = trans('preferences.notifications_opt_comment_replies'); - - $resp = $this->actingAs($editor)->get('/preferences/notifications'); - - $resp->assertSee($settingLabel1, true); - $resp->assertSee($settingLabel2, true); - } } From 95b9ea1a210495b7ba1a71776e08c8871b4c595a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 Sep 2023 23:41:02 +0100 Subject: [PATCH 32/97] Dev: Reviewed and expanded on PHP testing docs --- dev/docs/development.md | 8 ++-- dev/docs/php-testing.md | 87 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 dev/docs/php-testing.md diff --git a/dev/docs/development.md b/dev/docs/development.md index 44a077fa8..3c7a6e9d2 100644 --- a/dev/docs/development.md +++ b/dev/docs/development.md @@ -23,13 +23,13 @@ npm run production npm run dev ``` -BookStack has many integration tests that use Laravel's built-in testing capabilities which makes use of PHPUnit. There is a `mysql_testing` database defined within the app config which is what is used by PHPUnit. This database is set with the database name, username and password all defined as `bookstack-test`. You will have to create that database and that set of credentials before testing. +Further details about the BookStack JavaScript codebase can be found in the [javascript-code.md document](javascript-code.md). -The testing database will also need migrating and seeding beforehand. This can be done by running `composer refresh-test-database`. +## Automated App Testing -Once done you can run `composer test` in the application root directory to run all tests. Tests can be ran in parallel by running them via `composer t`. This will use Laravel's built-in parallel testing functionality, and attempt to create and seed a database instance for each testing thread. If required these parallel testing instances can be reset, before testing again, by running `composer t-reset`. +BookStack has a large suite of PHP tests to cover application functionality. We try to ensure that all additions and changes to the platform are covered with testing. -If the codebase needs to be tested with deprecations, this can be done via uncommenting the relevant line within the TestCase@setUp function. +For details about setting-up, running and writing tests please see the [php-testing.md document](php-testing.md). ## Code Standards diff --git a/dev/docs/php-testing.md b/dev/docs/php-testing.md new file mode 100644 index 000000000..1bd4a414f --- /dev/null +++ b/dev/docs/php-testing.md @@ -0,0 +1,87 @@ +# BookStack PHP Testing + +BookStack has many test cases defined within the `tests/` directory of the app. These are built upon [PHPUnit](https://phpunit.de/) along with Laravel's own test framework additions, and a bunch of custom helper classes. + +## Setup + +The application tests are mostly functional, rather than unit tests, meaning they simulate user actions and system components and therefore these require use of the database. To avoid potential conflicts within your development environment, the tests use a separate database. This is defined via a specific `mysql_testing` database connection in our configuration, and expects to use the following database access details: + +- Host: `127.0.0.1` +- Username: `bookstack-test` +- Password: `bookstack-test` +- Database: `bookstack-test` + +You will need to create a database, with access for these credentials, to allow the system to connect when running tests. Alternatively, if those don't suit, you can define a `TEST_DATABASE_URL` option in your `.env` file, or environment, with connection details like so: + +```bash +TEST_DATABASE_URL="mysql://username:password@host-name:port/database-name" +``` + +The testing database will need migrating and seeding with test data beforehand. This can be done by running `composer refresh-test-database`. + +## Running Tests + +You can run all tests via composer with `composer test` in the application root directory. +Alternatively, you can run PHPUnit directly with `php vendor/bin/phpunit`. + +Some editors, like PHPStorm, have in-built support for running tests on a per file, directory or class basis. +Otherwise, you can run PHPUnit with specified tests and/or filter to limit the tests ran: + +```bash +# Run all test in the "./tests/HomepageTest.php" file +php vendor/bin/phpunit ./tests/HomepageTest.php + +# Run all test in the "./tests/User" directory +php vendor/bin/phpunit ./tests/User + +# Filter to a particular test method name +php vendor/bin/phpunit --filter test_default_homepage_visible + +# Filter to a particular test class name +php vendor/bin/phpunit --filter HomepageTest +``` + +If the codebase needs to be tested with deprecations, this can be done via uncommenting the relevant line within the `TestCase@setUp` function. This is not expected for most PRs to the project, but instead used for maintenance tasks like dependency & PHP upgrades. + +## Writing Tests + +To understand how tests are written & used, it's advised you read through existing test cases similar to what you need to write. Tests are written in a rather scrappy manner, compared to the core app codebase, which is fine and expected since there's often hoops to jump through for various functionality. Scrappy tests are better than no tests. + +Test classes have to be within the `tests/` folder, and be named ending in `Test`. These should always extend the `Tests\TestCase` class. +Test methods should be written in snake_case, start with `test_`, and be public methods. + +Here are some general rules & patterns we follow in the tests: + +- All external remote system resources, like HTTP calls and LDAP connections, are mocked. +- We prefer to hard-code expected text & URLs to better detect potential changes in the system rather than use dynamic references. This provides higher sensitivity to changes, and has never been much of a maintenance issue. +- Only test with an admin user if needed, otherwise keep to less privileged users to ensure permission systems are active and exercised within tests. +- If testing for the lack of something (e.g. `$this->assertDontSee('TextAfterChange')`) then this should be accompanied by some form of positive confirmation (e.g. `$this->assertSee('TextBeforeChange')`). + +### Test Helpers + +Our default `TestCase` is bloated with helpers to assist in testing scenarios. Some of these shown below, but you should jump through and explore these in your IDE/editor to explore their full capabilities and options: + +```php +// Run the test as a logged-in-user at a certain privilege level +$this->asAdmin(); +$this->asEditor(); +$this->asViewer(); + +// Provides a bunch of entity (shelf/book/chapter/page) content and actions +$this->entities; + +// Provides various user & role abilities +$this->users; + +// Provides many helpful actions relate to system & content permissions +$this->permissions; + +// Provides a range of methods for dealing with files & uploads in tests +$this->files; + +// Parse HTML of a response to assert HTML-based conditions +// Uses https://github.com/ssddanbrown/asserthtml library. +$this->withHtml($resp); +// Example: +$this->withHtml($this->get('/'))->assertElementContains('p[id="top"]', 'Hello!'); +``` \ No newline at end of file From ea7592509f61605b84959d39d74757e962501609 Mon Sep 17 00:00:00 2001 From: Marc Hagen Date: Mon, 18 Sep 2023 19:07:30 +0200 Subject: [PATCH 33/97] feat: Artisan command for updating avatars for existing users --- app/Console/Commands/RefreshAvatarCommand.php | 155 +++++++++ tests/Commands/RefreshAvatarCommandTest.php | 317 ++++++++++++++++++ 2 files changed, 472 insertions(+) create mode 100644 app/Console/Commands/RefreshAvatarCommand.php create mode 100644 tests/Commands/RefreshAvatarCommandTest.php diff --git a/app/Console/Commands/RefreshAvatarCommand.php b/app/Console/Commands/RefreshAvatarCommand.php new file mode 100644 index 000000000..ca78d3860 --- /dev/null +++ b/app/Console/Commands/RefreshAvatarCommand.php @@ -0,0 +1,155 @@ +option('force'); + + if ($this->option('users-without-avatars')) { + return $this->handleUpdateWithoutAvatars($userAvatar, $dryRun); + } + + if ($this->option('all')) { + return $this->handleUpdateAllAvatars($userAvatar, $dryRun); + } + + return $this->handleSingleUserUpdate($userAvatar); + } + + private function handleUpdateWithoutAvatars(UserAvatars $userAvatar, bool $dryRun): int + { + $users = User::query()->where('image_id', '=', 0)->get(); + $this->info(count($users) . ' user(s) found without avatars.'); + + if (!$dryRun) { + $proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to refresh avatars of users that do not have one?'); + if (!$proceed) { + return self::SUCCESS; + } + } + + return $this->processUsers($users, $userAvatar, $dryRun); + } + + private function handleUpdateAllAvatars(UserAvatars $userAvatar, bool $dryRun): int + { + $users = User::query()->get(); + $this->info(count($users) . ' user(s) found.'); + + if (!$dryRun) { + $proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to refresh avatars for ALL USERS?'); + if (!$proceed) { + return self::SUCCESS; + } + } + + return $this->processUsers($users, $userAvatar, $dryRun); + } + + private function processUsers(Collection $users, UserAvatars $userAvatar, bool $dryRun): int + { + $exitCode = self::SUCCESS; + foreach ($users as $user) { + $this->getOutput()->write("ID {$user->id} - ", false); + + if ($dryRun) { + $this->warn('Not updated'); + continue; + } + + if ($this->fetchAvatar($userAvatar, $user)) { + $this->info('Updated'); + } else { + $this->error('Not updated'); + $exitCode = self::FAILURE; + } + } + + $this->getOutput()->newLine(); + if ($dryRun) { + $this->comment('Dry run, no avatars have been updated'); + $this->comment('Run with -f or --force to perform the update'); + } + + return $exitCode; + } + + + private function handleSingleUserUpdate(UserAvatars $userAvatar): int + { + $id = $this->option('id'); + $email = $this->option('email'); + if (!$id && !$email) { + $this->error('Either a --id= or --email= option must be provided.'); + $this->error('Run with `--help` to more options'); + + return self::FAILURE; + } + + $field = $id ? 'id' : 'email'; + $value = $id ?: $email; + + $user = User::query() + ->where($field, '=', $value) + ->first(); + + if (!$user) { + $this->error("A user where {$field}={$value} could not be found."); + + return self::FAILURE; + } + + $this->info("This will refresh the avatar for user: \n- ID: {$user->id}\n- Name: {$user->name}\n- Email: {$user->email}\n"); + $confirm = $this->confirm('Are you sure you want to proceed?'); + if ($confirm) { + if ($this->fetchAvatar($userAvatar, $user)) { + $this->info('User avatar has been updated.'); + return self::SUCCESS; + } + + $this->info('Could not update avatar please review logs.'); + } + + return self::FAILURE; + } + + private function fetchAvatar(UserAvatars $userAvatar, User $user): bool + { + $oldId = $user->avatar->id ?? 0; + + $userAvatar->fetchAndAssignToUser($user); + + $user->refresh(); + $newId = $user->avatar->id ?? $oldId; + return $oldId !== $newId; + } +} diff --git a/tests/Commands/RefreshAvatarCommandTest.php b/tests/Commands/RefreshAvatarCommandTest.php new file mode 100644 index 000000000..d625097ef --- /dev/null +++ b/tests/Commands/RefreshAvatarCommandTest.php @@ -0,0 +1,317 @@ +artisan(RefreshAvatarCommand::class) + ->expectsOutput('Either a --id= or --email= option must be provided.') + ->assertExitCode(Command::FAILURE); + } + + public function test_command_runs_with_provided_email() + { + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); + config()->set(['services.disable_services' => false]); + + /** @var User $user */ + $user = User::query()->first(); + + /** @var UserAvatars $avatar */ + $avatar = app()->make(UserAvatars::class); + $avatar->destroyAllForUser($user); + + $this->assertFalse($user->avatar()->exists()); + $this->artisan(RefreshAvatarCommand::class, ['--email' => $user->email]) + ->expectsOutputToContain("- ID: {$user->id}") + ->expectsQuestion('Are you sure you want to proceed?', true) + ->expectsOutput('User avatar has been updated.') + ->assertExitCode(Command::SUCCESS); + + $expectedUri = 'https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'; + $this->assertEquals($expectedUri, $requests->latestRequest()->getUri()); + + $user->refresh(); + $this->assertTrue($user->avatar()->exists()); + } + + public function test_command_runs_with_provided_id() + { + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); + config()->set(['services.disable_services' => false]); + + /** @var User $user */ + $user = User::query()->first(); + + /** @var UserAvatars $avatar */ + $avatar = app()->make(UserAvatars::class); + $avatar->destroyAllForUser($user); + + $this->assertFalse($user->avatar()->exists()); + $this->artisan(RefreshAvatarCommand::class, ['--id' => $user->id]) + ->expectsOutputToContain("- ID: {$user->id}") + ->expectsQuestion('Are you sure you want to proceed?', true) + ->expectsOutput('User avatar has been updated.') + ->assertExitCode(Command::SUCCESS); + + $expectedUri = 'https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'; + $this->assertEquals($expectedUri, $requests->latestRequest()->getUri()); + + $user->refresh(); + $this->assertTrue($user->avatar()->exists()); + } + + public function test_command_runs_with_provided_id_error_upstream() + { + $requests = $this->mockHttpClient([new Response(404)]); + config()->set(['services.disable_services' => false]); + + /** @var User $user */ + $user = User::query()->first(); + /** @var UserAvatars $avatar */ + $avatar = app()->make(UserAvatars::class); + $avatar->assignToUserFromExistingData($user, $this->files->pngImageData(), 'png'); + + $oldId = $user->avatar->id ?? 0; + + $this->artisan(RefreshAvatarCommand::class, ['--id' => $user->id]) + ->expectsOutputToContain("- ID: {$user->id}") + ->expectsQuestion('Are you sure you want to proceed?', true) + ->expectsOutput('Could not update avatar please review logs.') + ->assertExitCode(Command::FAILURE); + + $this->assertEquals(1, $requests->requestCount()); + + $user->refresh(); + $newId = $user->avatar->id ?? $oldId; + $this->assertEquals($oldId, $newId); + } + + public function test_saying_no_to_confirmation_does_not_refresh_avatar() + { + /** @var User $user */ + $user = User::query()->first(); + + $this->assertFalse($user->avatar()->exists()); + $this->artisan(RefreshAvatarCommand::class, ['--id' => $user->id]) + ->expectsQuestion('Are you sure you want to proceed?', false) + ->assertExitCode(Command::FAILURE); + $this->assertFalse($user->avatar()->exists()); + } + + public function test_giving_non_existing_user_shows_error_message() + { + $this->artisan(RefreshAvatarCommand::class, ['--email' => 'donkeys@example.com']) + ->expectsOutput('A user where email=donkeys@example.com could not be found.') + ->assertExitCode(Command::FAILURE); + } + + public function test_command_runs_all_users_without_avatars_dry_run() + { + $users = User::query()->where('image_id', '=', 0)->get(); + + $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true]) + ->expectsOutput(count($users) . ' user(s) found without avatars.') + ->expectsOutput("ID {$users[0]->id} - ") + ->expectsOutput('Not updated') + ->expectsOutput('Dry run, no avatars have been updated') + ->assertExitCode(Command::SUCCESS); + } + + public function test_command_runs_all_users_without_avatars_non_to_update() + { + config()->set(['services.disable_services' => false]); + + /** @var UserAvatars $avatar */ + $avatar = app()->make(UserAvatars::class); + + /** @var Collection|User[] $users */ + $users = User::query()->get(); + $responses = []; + foreach ($users as $user) { + $avatar->fetchAndAssignToUser($user); + $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); + } + $requests = $this->mockHttpClient($responses); + + $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true, '-f' => true]) + ->expectsOutput('0 user(s) found without avatars.') + ->expectsQuestion('Are you sure you want to refresh avatars of users that do not have one?', true) + ->assertExitCode(Command::SUCCESS); + + $userWithAvatars = User::query()->where('image_id', '==', 0)->count(); + $this->assertEquals(0, $userWithAvatars); + $this->assertEquals(0, $requests->requestCount()); + } + + public function test_command_runs_all_users_without_avatars() + { + config()->set(['services.disable_services' => false]); + + /** @var UserAvatars $avatar */ + $avatar = app()->make(UserAvatars::class); + + /** @var Collection|User[] $users */ + $users = User::query()->get(); + foreach ($users as $user) { + $avatar->destroyAllForUser($user); + } + + /** @var Collection|User[] $users */ + $users = User::query()->where('image_id', '=', 0)->get(); + + $pendingCommand = $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true, '-f' => true]); + $pendingCommand + ->expectsOutput($users->count() . ' user(s) found without avatars.') + ->expectsQuestion('Are you sure you want to refresh avatars of users that do not have one?', true); + + $responses = []; + foreach ($users as $user) { + $pendingCommand->expectsOutput("ID {$user->id} - "); + $pendingCommand->expectsOutput('Updated'); + $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); + } + $requests = $this->mockHttpClient($responses); + + $pendingCommand->assertExitCode(Command::SUCCESS); + $pendingCommand->run(); + + $userWithAvatars = User::query()->where('image_id', '!=', 0)->count(); + $this->assertEquals($users->count(), $userWithAvatars); + $this->assertEquals($users->count(), $requests->requestCount()); + } + + public function test_saying_no_to_confirmation_all_users_without_avatars() + { + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); + config()->set(['services.disable_services' => false]); + + /** @var UserAvatars $avatar */ + $avatar = app()->make(UserAvatars::class); + + /** @var Collection|User[] $users */ + $users = User::query()->get(); + foreach ($users as $user) { + $avatar->destroyAllForUser($user); + } + + $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true, '-f' => true]) + ->expectsQuestion('Are you sure you want to refresh avatars of users that do not have one?', false) + ->assertExitCode(Command::SUCCESS); + + $userWithAvatars = User::query()->where('image_id', '=', 0)->count(); + $this->assertEquals($users->count(), $userWithAvatars); + $this->assertEquals(0, $requests->requestCount()); + } + + public function test_command_runs_all_users_dry_run() + { + $users = User::query()->where('image_id', '=', 0)->get(); + + $this->artisan(RefreshAvatarCommand::class, ['--all' => true]) + ->expectsOutput(count($users) . ' user(s) found.') + ->expectsOutput("ID {$users[0]->id} - ") + ->expectsOutput('Not updated') + ->expectsOutput('Dry run, no avatars have been updated') + ->assertExitCode(Command::SUCCESS); + } + + public function test_command_runs_update_all_users_avatar() + { + config()->set(['services.disable_services' => false]); + + /** @var Collection|User[] $users */ + $users = User::query()->get(); + + $pendingCommand = $this->artisan(RefreshAvatarCommand::class, ['--all' => true, '-f' => true]); + $pendingCommand + ->expectsOutput($users->count() . ' user(s) found.') + ->expectsQuestion('Are you sure you want to refresh avatars for ALL USERS?', true); + + $responses = []; + foreach ($users as $user) { + $pendingCommand->expectsOutput("ID {$user->id} - "); + $pendingCommand->expectsOutput('Updated'); + $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); + } + $requests = $this->mockHttpClient($responses); + + $pendingCommand->assertExitCode(Command::SUCCESS); + $pendingCommand->run(); + + $userWithAvatars = User::query()->where('image_id', '!=', 0)->count(); + $this->assertEquals($users->count(), $userWithAvatars); + $this->assertEquals($users->count(), $requests->requestCount()); + } + + public function test_command_runs_update_all_users_avatar_errors() + { + config()->set(['services.disable_services' => false]); + + /** @var Collection|User[] $users */ + $users = User::query()->get(); + + $pendingCommand = $this->artisan(RefreshAvatarCommand::class, ['--all' => true, '-f' => true]); + $pendingCommand + ->expectsOutput($users->count() . ' user(s) found.') + ->expectsQuestion('Are you sure you want to refresh avatars for ALL USERS?', true); + + $responses = []; + foreach ($users as $key => $user) { + $pendingCommand->expectsOutput("ID {$user->id} - "); + + if ($key == 1) { + $pendingCommand->expectsOutput('Not updated'); + $responses[] = new Response(404); + continue; + } + + $pendingCommand->expectsOutput('Updated'); + $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); + } + + $requests = $this->mockHttpClient($responses); + + $pendingCommand->assertExitCode(Command::FAILURE); + $pendingCommand->run(); + + $userWithAvatars = User::query()->where('image_id', '!=', 0)->count(); + $this->assertEquals($users->count() - 1, $userWithAvatars); + $this->assertEquals($users->count(), $requests->requestCount()); + } + + public function test_saying_no_to_confirmation_update_all_users_avatar() + { + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); + config()->set(['services.disable_services' => false]); + + /** @var UserAvatars $avatar */ + $avatar = app()->make(UserAvatars::class); + + /** @var Collection|User[] $users */ + $users = User::query()->get(); + foreach ($users as $user) { + $avatar->destroyAllForUser($user); + } + + $this->artisan(RefreshAvatarCommand::class, ['--all' => true, '-f' => true]) + ->expectsQuestion('Are you sure you want to refresh avatars for ALL USERS?', false) + ->assertExitCode(Command::SUCCESS); + + $userWithAvatars = User::query()->where('image_id', '=', 0)->count(); + $this->assertEquals($users->count(), $userWithAvatars); + $this->assertEquals(0, $requests->requestCount()); + } +} From ca98155373e759b65d2e62558c97a68f7158c704 Mon Sep 17 00:00:00 2001 From: Marc Hagen Date: Mon, 18 Sep 2023 20:04:59 +0200 Subject: [PATCH 34/97] fix: Actually check if we have correct data --- app/Uploads/UserAvatars.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index 9692b3f38..0cda31a1c 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -56,7 +56,7 @@ class UserAvatars /** * Destroy all user avatars uploaded to the given user. */ - public function destroyAllForUser(User $user) + public function destroyAllForUser(User $user): void { $profileImages = Image::query()->where('type', '=', 'user') ->where('uploaded_to', '=', $user->id) @@ -70,7 +70,7 @@ class UserAvatars /** * Save an avatar image from an external service. * - * @throws Exception + * @throws HttpFetchException */ protected function saveAvatarImage(User $user, int $size = 500): Image { @@ -114,12 +114,14 @@ class UserAvatars try { $client = $this->http->buildClient(5); $response = $client->sendRequest(new Request('GET', $url)); - $imageData = (string) $response->getBody(); + if ($response->getStatusCode() !== 200) { + throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url])); + } + + return (string) $response->getBody(); } catch (ClientExceptionInterface $exception) { throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url]), $exception->getCode(), $exception); } - - return $imageData; } /** From 588ed785d26ce66af0ba709e71a5c1323519c29e Mon Sep 17 00:00:00 2001 From: lawsssscat Date: Tue, 19 Sep 2023 22:12:33 +0800 Subject: [PATCH 35/97] fix Sidebar scrolling at mid-range sceen --- resources/sass/_layout.scss | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/resources/sass/_layout.scss b/resources/sass/_layout.scss index 503298fcc..ac0e96a90 100644 --- a/resources/sass/_layout.scss +++ b/resources/sass/_layout.scss @@ -391,6 +391,18 @@ body.flexbox { position: sticky; top: $-m; } + .tri-layout-left-contents { + position: sticky; + top: 0; + max-height: 100vh; + overflow-y: scroll; + height: 100%; + scrollbar-width: none; + -ms-overflow-style: none; + &::-webkit-scrollbar { + display: none; + } + } } @include larger-than($xxl) { .tri-layout-left-contents, .tri-layout-right-contents { From 4b4d8ba2a18fd6daa0face55742d50dc0230a4b4 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 19 Sep 2023 15:53:01 +0100 Subject: [PATCH 36/97] Avatar Commend: Simplified and updated during review During review of #4560. - Simplified command to share as much log as possible across different run options. - Extracted out user handling to share with MFA command. - Added specific handling for disabled avatar fetching. - Added mention of avatar endpoint, to make it clear where these avatars are coming from (Protect against user expectation of LDAP avatar sync). - Simplified a range of the testing. - Tweaked wording and code formatting. --- app/Console/Commands/HandlesSingleUser.php | 40 +++ app/Console/Commands/RefreshAvatarCommand.php | 121 +++----- app/Console/Commands/ResetMfaCommand.php | 27 +- app/Uploads/UserAvatars.php | 4 +- tests/Commands/RefreshAvatarCommandTest.php | 265 +++++++----------- 5 files changed, 187 insertions(+), 270 deletions(-) create mode 100644 app/Console/Commands/HandlesSingleUser.php diff --git a/app/Console/Commands/HandlesSingleUser.php b/app/Console/Commands/HandlesSingleUser.php new file mode 100644 index 000000000..d3014aab1 --- /dev/null +++ b/app/Console/Commands/HandlesSingleUser.php @@ -0,0 +1,40 @@ +option('id'); + $email = $this->option('email'); + if (!$id && !$email) { + throw new Exception("Either a --id= or --email= option must be provided.\nRun this command with `--help` to show more options."); + } + + $field = $id ? 'id' : 'email'; + $value = $id ?: $email; + + $user = User::query() + ->where($field, '=', $value) + ->first(); + + if (!$user) { + throw new Exception("A user where {$field}={$value} could not be found."); + } + + return $user; + } +} diff --git a/app/Console/Commands/RefreshAvatarCommand.php b/app/Console/Commands/RefreshAvatarCommand.php index ca78d3860..e402285e7 100644 --- a/app/Console/Commands/RefreshAvatarCommand.php +++ b/app/Console/Commands/RefreshAvatarCommand.php @@ -1,16 +1,16 @@ option('force'); + if (!$userAvatar->avatarFetchEnabled()) { + $this->error("Avatar fetching is disabled on this instance."); + return self::FAILURE; + } if ($this->option('users-without-avatars')) { - return $this->handleUpdateWithoutAvatars($userAvatar, $dryRun); + return $this->processUsers(User::query()->whereDoesntHave('avatar')->get()->all(), $userAvatar); } if ($this->option('all')) { - return $this->handleUpdateAllAvatars($userAvatar, $dryRun); + return $this->processUsers(User::query()->get()->all(), $userAvatar); } - return $this->handleSingleUserUpdate($userAvatar); + try { + $user = $this->fetchProvidedUser(); + return $this->processUsers([$user], $userAvatar); + } catch (Exception $exception) { + $this->error($exception->getMessage()); + return self::FAILURE; + } } - private function handleUpdateWithoutAvatars(UserAvatars $userAvatar, bool $dryRun): int + /** + * @param User[] $users + */ + private function processUsers(array $users, UserAvatars $userAvatar): int { - $users = User::query()->where('image_id', '=', 0)->get(); - $this->info(count($users) . ' user(s) found without avatars.'); + $dryRun = !$this->option('force'); + $this->info(count($users) . " user(s) found to update avatars for."); + + if (count($users) === 0) { + return self::SUCCESS; + } if (!$dryRun) { - $proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to refresh avatars of users that do not have one?'); + $fetchHost = parse_url($userAvatar->getAvatarUrl(), PHP_URL_HOST); + $this->warn("This will destroy any existing avatar images these users have, and attempt to fetch new avatar images from {$fetchHost}."); + $proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to proceed?'); if (!$proceed) { return self::SUCCESS; } } - return $this->processUsers($users, $userAvatar, $dryRun); - } + $this->info(""); - private function handleUpdateAllAvatars(UserAvatars $userAvatar, bool $dryRun): int - { - $users = User::query()->get(); - $this->info(count($users) . ' user(s) found.'); - - if (!$dryRun) { - $proceed = !$this->input->isInteractive() || $this->confirm('Are you sure you want to refresh avatars for ALL USERS?'); - if (!$proceed) { - return self::SUCCESS; - } - } - - return $this->processUsers($users, $userAvatar, $dryRun); - } - - private function processUsers(Collection $users, UserAvatars $userAvatar, bool $dryRun): int - { $exitCode = self::SUCCESS; foreach ($users as $user) { - $this->getOutput()->write("ID {$user->id} - ", false); + $linePrefix = "[ID: {$user->id}] $user->email -"; if ($dryRun) { - $this->warn('Not updated'); + $this->warn("{$linePrefix} Not updated"); continue; } if ($this->fetchAvatar($userAvatar, $user)) { - $this->info('Updated'); + $this->info("{$linePrefix} Updated"); } else { - $this->error('Not updated'); + $this->error("{$linePrefix} Not updated"); $exitCode = self::FAILURE; } } - $this->getOutput()->newLine(); if ($dryRun) { - $this->comment('Dry run, no avatars have been updated'); - $this->comment('Run with -f or --force to perform the update'); + $this->comment(""); + $this->comment("Dry run, no avatars were updated."); + $this->comment('Run with -f or --force to perform the update.'); } return $exitCode; } - - private function handleSingleUserUpdate(UserAvatars $userAvatar): int - { - $id = $this->option('id'); - $email = $this->option('email'); - if (!$id && !$email) { - $this->error('Either a --id= or --email= option must be provided.'); - $this->error('Run with `--help` to more options'); - - return self::FAILURE; - } - - $field = $id ? 'id' : 'email'; - $value = $id ?: $email; - - $user = User::query() - ->where($field, '=', $value) - ->first(); - - if (!$user) { - $this->error("A user where {$field}={$value} could not be found."); - - return self::FAILURE; - } - - $this->info("This will refresh the avatar for user: \n- ID: {$user->id}\n- Name: {$user->name}\n- Email: {$user->email}\n"); - $confirm = $this->confirm('Are you sure you want to proceed?'); - if ($confirm) { - if ($this->fetchAvatar($userAvatar, $user)) { - $this->info('User avatar has been updated.'); - return self::SUCCESS; - } - - $this->info('Could not update avatar please review logs.'); - } - - return self::FAILURE; - } - private function fetchAvatar(UserAvatars $userAvatar, User $user): bool { $oldId = $user->avatar->id ?? 0; diff --git a/app/Console/Commands/ResetMfaCommand.php b/app/Console/Commands/ResetMfaCommand.php index b8076d2d6..2b0801e39 100644 --- a/app/Console/Commands/ResetMfaCommand.php +++ b/app/Console/Commands/ResetMfaCommand.php @@ -2,11 +2,13 @@ namespace BookStack\Console\Commands; -use BookStack\Users\Models\User; +use Exception; use Illuminate\Console\Command; class ResetMfaCommand extends Command { + use HandlesSingleUser; + /** * The name and signature of the console command. * @@ -29,25 +31,10 @@ class ResetMfaCommand extends Command */ public function handle(): int { - $id = $this->option('id'); - $email = $this->option('email'); - if (!$id && !$email) { - $this->error('Either a --id= or --email= option must be provided.'); - - return 1; - } - - $field = $id ? 'id' : 'email'; - $value = $id ?: $email; - - /** @var User $user */ - $user = User::query() - ->where($field, '=', $value) - ->first(); - - if (!$user) { - $this->error("A user where {$field}={$value} could not be found."); - + try { + $user = $this->fetchProvidedUser(); + } catch (Exception $exception) { + $this->error($exception->getMessage()); return 1; } diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index 0cda31a1c..c62324735 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -127,7 +127,7 @@ class UserAvatars /** * Check if fetching external avatars is enabled. */ - protected function avatarFetchEnabled(): bool + public function avatarFetchEnabled(): bool { $fetchUrl = $this->getAvatarUrl(); @@ -137,7 +137,7 @@ class UserAvatars /** * Get the URL to fetch avatars from. */ - protected function getAvatarUrl(): string + public function getAvatarUrl(): string { $configOption = config('services.avatar_url'); if ($configOption === false) { diff --git a/tests/Commands/RefreshAvatarCommandTest.php b/tests/Commands/RefreshAvatarCommandTest.php index d625097ef..6126f21a8 100644 --- a/tests/Commands/RefreshAvatarCommandTest.php +++ b/tests/Commands/RefreshAvatarCommandTest.php @@ -1,47 +1,55 @@ set([ + 'services.disable_services' => false, + 'services.avatar_url' => 'https://avatars.example.com?a=b', + ]); + } + + public function test_command_errors_if_avatar_fetch_disabled() + { + config()->set(['services.avatar_url' => false]); + + $this->artisan('bookstack:refresh-avatar') + ->expectsOutputToContain("Avatar fetching is disabled on this instance") + ->assertExitCode(1); + } + public function test_command_requires_email_or_id_option() { - $this->artisan(RefreshAvatarCommand::class) - ->expectsOutput('Either a --id= or --email= option must be provided.') - ->assertExitCode(Command::FAILURE); + $this->artisan('bookstack:refresh-avatar') + ->expectsOutputToContain("Either a --id= or --email= option must be provided") + ->assertExitCode(1); } public function test_command_runs_with_provided_email() { $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); - config()->set(['services.disable_services' => false]); - - /** @var User $user */ - $user = User::query()->first(); - - /** @var UserAvatars $avatar */ - $avatar = app()->make(UserAvatars::class); - $avatar->destroyAllForUser($user); + $user = $this->users->viewer(); $this->assertFalse($user->avatar()->exists()); - $this->artisan(RefreshAvatarCommand::class, ['--email' => $user->email]) - ->expectsOutputToContain("- ID: {$user->id}") - ->expectsQuestion('Are you sure you want to proceed?', true) - ->expectsOutput('User avatar has been updated.') - ->assertExitCode(Command::SUCCESS); - $expectedUri = 'https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'; - $this->assertEquals($expectedUri, $requests->latestRequest()->getUri()); + $this->artisan("bookstack:refresh-avatar --email={$user->email} -f") + ->expectsQuestion('Are you sure you want to proceed?', true) + ->expectsOutput("[ID: {$user->id}] {$user->email} - Updated") + ->expectsOutputToContain('This will destroy any existing avatar images these users have, and attempt to fetch new avatar images from avatars.example.com') + ->assertExitCode(0); + + $this->assertEquals('https://avatars.example.com?a=b', $requests->latestRequest()->getUri()); $user->refresh(); $this->assertTrue($user->avatar()->exists()); @@ -50,24 +58,16 @@ final class RefreshAvatarCommandTest extends TestCase public function test_command_runs_with_provided_id() { $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); - config()->set(['services.disable_services' => false]); - - /** @var User $user */ - $user = User::query()->first(); - - /** @var UserAvatars $avatar */ - $avatar = app()->make(UserAvatars::class); - $avatar->destroyAllForUser($user); + $user = $this->users->viewer(); $this->assertFalse($user->avatar()->exists()); - $this->artisan(RefreshAvatarCommand::class, ['--id' => $user->id]) - ->expectsOutputToContain("- ID: {$user->id}") - ->expectsQuestion('Are you sure you want to proceed?', true) - ->expectsOutput('User avatar has been updated.') - ->assertExitCode(Command::SUCCESS); - $expectedUri = 'https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'; - $this->assertEquals($expectedUri, $requests->latestRequest()->getUri()); + $this->artisan("bookstack:refresh-avatar --id={$user->id} -f") + ->expectsQuestion('Are you sure you want to proceed?', true) + ->expectsOutput("[ID: {$user->id}] {$user->email} - Updated") + ->assertExitCode(0); + + $this->assertEquals('https://avatars.example.com?a=b', $requests->latestRequest()->getUri()); $user->refresh(); $this->assertTrue($user->avatar()->exists()); @@ -76,143 +76,93 @@ final class RefreshAvatarCommandTest extends TestCase public function test_command_runs_with_provided_id_error_upstream() { $requests = $this->mockHttpClient([new Response(404)]); - config()->set(['services.disable_services' => false]); - /** @var User $user */ - $user = User::query()->first(); - /** @var UserAvatars $avatar */ - $avatar = app()->make(UserAvatars::class); - $avatar->assignToUserFromExistingData($user, $this->files->pngImageData(), 'png'); + $user = $this->users->viewer(); + $this->assertFalse($user->avatar()->exists()); - $oldId = $user->avatar->id ?? 0; - - $this->artisan(RefreshAvatarCommand::class, ['--id' => $user->id]) - ->expectsOutputToContain("- ID: {$user->id}") + $this->artisan("bookstack:refresh-avatar --id={$user->id} -f") ->expectsQuestion('Are you sure you want to proceed?', true) - ->expectsOutput('Could not update avatar please review logs.') - ->assertExitCode(Command::FAILURE); + ->expectsOutput("[ID: {$user->id}] {$user->email} - Not updated") + ->assertExitCode(1); $this->assertEquals(1, $requests->requestCount()); - - $user->refresh(); - $newId = $user->avatar->id ?? $oldId; - $this->assertEquals($oldId, $newId); + $this->assertFalse($user->avatar()->exists()); } public function test_saying_no_to_confirmation_does_not_refresh_avatar() { - /** @var User $user */ - $user = User::query()->first(); + $user = $this->users->viewer(); $this->assertFalse($user->avatar()->exists()); - $this->artisan(RefreshAvatarCommand::class, ['--id' => $user->id]) + $this->artisan("bookstack:refresh-avatar --id={$user->id} -f") ->expectsQuestion('Are you sure you want to proceed?', false) - ->assertExitCode(Command::FAILURE); + ->assertExitCode(0); $this->assertFalse($user->avatar()->exists()); } public function test_giving_non_existing_user_shows_error_message() { - $this->artisan(RefreshAvatarCommand::class, ['--email' => 'donkeys@example.com']) + $this->artisan('bookstack:refresh-avatar --email=donkeys@example.com') ->expectsOutput('A user where email=donkeys@example.com could not be found.') - ->assertExitCode(Command::FAILURE); + ->assertExitCode(1); } public function test_command_runs_all_users_without_avatars_dry_run() { $users = User::query()->where('image_id', '=', 0)->get(); - $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true]) - ->expectsOutput(count($users) . ' user(s) found without avatars.') - ->expectsOutput("ID {$users[0]->id} - ") - ->expectsOutput('Not updated') - ->expectsOutput('Dry run, no avatars have been updated') - ->assertExitCode(Command::SUCCESS); + $this->artisan('bookstack:refresh-avatar --users-without-avatars') + ->expectsOutput(count($users) . ' user(s) found to update avatars for.') + ->expectsOutput("[ID: {$users[0]->id}] {$users[0]->email} - Not updated") + ->expectsOutput('Dry run, no avatars were updated.') + ->assertExitCode(0); } - public function test_command_runs_all_users_without_avatars_non_to_update() + public function test_command_runs_all_users_without_avatars_with_none_to_update() { - config()->set(['services.disable_services' => false]); + $requests = $this->mockHttpClient(); + $image = Image::factory()->create(); + User::query()->update(['image_id' => $image->id]); - /** @var UserAvatars $avatar */ - $avatar = app()->make(UserAvatars::class); + $this->artisan('bookstack:refresh-avatar --users-without-avatars -f') + ->expectsOutput('0 user(s) found to update avatars for.') + ->assertExitCode(0); - /** @var Collection|User[] $users */ - $users = User::query()->get(); - $responses = []; - foreach ($users as $user) { - $avatar->fetchAndAssignToUser($user); - $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); - } - $requests = $this->mockHttpClient($responses); - - $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true, '-f' => true]) - ->expectsOutput('0 user(s) found without avatars.') - ->expectsQuestion('Are you sure you want to refresh avatars of users that do not have one?', true) - ->assertExitCode(Command::SUCCESS); - - $userWithAvatars = User::query()->where('image_id', '==', 0)->count(); - $this->assertEquals(0, $userWithAvatars); $this->assertEquals(0, $requests->requestCount()); } public function test_command_runs_all_users_without_avatars() { - config()->set(['services.disable_services' => false]); - - /** @var UserAvatars $avatar */ - $avatar = app()->make(UserAvatars::class); - - /** @var Collection|User[] $users */ - $users = User::query()->get(); - foreach ($users as $user) { - $avatar->destroyAllForUser($user); - } - /** @var Collection|User[] $users */ $users = User::query()->where('image_id', '=', 0)->get(); - $pendingCommand = $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true, '-f' => true]); + $pendingCommand = $this->artisan('bookstack:refresh-avatar --users-without-avatars -f'); $pendingCommand - ->expectsOutput($users->count() . ' user(s) found without avatars.') - ->expectsQuestion('Are you sure you want to refresh avatars of users that do not have one?', true); + ->expectsOutput($users->count() . ' user(s) found to update avatars for.') + ->expectsQuestion('Are you sure you want to proceed?', true); $responses = []; foreach ($users as $user) { - $pendingCommand->expectsOutput("ID {$user->id} - "); - $pendingCommand->expectsOutput('Updated'); + $pendingCommand->expectsOutput("[ID: {$user->id}] {$user->email} - Updated"); $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); } $requests = $this->mockHttpClient($responses); - $pendingCommand->assertExitCode(Command::SUCCESS); + $pendingCommand->assertExitCode(0); $pendingCommand->run(); - $userWithAvatars = User::query()->where('image_id', '!=', 0)->count(); - $this->assertEquals($users->count(), $userWithAvatars); + $this->assertEquals(0, User::query()->where('image_id', '=', 0)->count()); $this->assertEquals($users->count(), $requests->requestCount()); } public function test_saying_no_to_confirmation_all_users_without_avatars() { - $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); - config()->set(['services.disable_services' => false]); + $requests = $this->mockHttpClient(); - /** @var UserAvatars $avatar */ - $avatar = app()->make(UserAvatars::class); + $this->artisan('bookstack:refresh-avatar --users-without-avatars -f') + ->expectsQuestion('Are you sure you want to proceed?', false) + ->assertExitCode(0); - /** @var Collection|User[] $users */ - $users = User::query()->get(); - foreach ($users as $user) { - $avatar->destroyAllForUser($user); - } - - $this->artisan(RefreshAvatarCommand::class, ['--users-without-avatars' => true, '-f' => true]) - ->expectsQuestion('Are you sure you want to refresh avatars of users that do not have one?', false) - ->assertExitCode(Command::SUCCESS); - - $userWithAvatars = User::query()->where('image_id', '=', 0)->count(); - $this->assertEquals($users->count(), $userWithAvatars); $this->assertEquals(0, $requests->requestCount()); } @@ -220,98 +170,77 @@ final class RefreshAvatarCommandTest extends TestCase { $users = User::query()->where('image_id', '=', 0)->get(); - $this->artisan(RefreshAvatarCommand::class, ['--all' => true]) - ->expectsOutput(count($users) . ' user(s) found.') - ->expectsOutput("ID {$users[0]->id} - ") - ->expectsOutput('Not updated') - ->expectsOutput('Dry run, no avatars have been updated') - ->assertExitCode(Command::SUCCESS); + $this->artisan('bookstack:refresh-avatar --all') + ->expectsOutput(count($users) . ' user(s) found to update avatars for.') + ->expectsOutput("[ID: {$users[0]->id}] {$users[0]->email} - Not updated") + ->expectsOutput('Dry run, no avatars were updated.') + ->assertExitCode(0); } public function test_command_runs_update_all_users_avatar() { - config()->set(['services.disable_services' => false]); - /** @var Collection|User[] $users */ $users = User::query()->get(); - $pendingCommand = $this->artisan(RefreshAvatarCommand::class, ['--all' => true, '-f' => true]); + $pendingCommand = $this->artisan('bookstack:refresh-avatar --all -f'); $pendingCommand - ->expectsOutput($users->count() . ' user(s) found.') - ->expectsQuestion('Are you sure you want to refresh avatars for ALL USERS?', true); + ->expectsOutput($users->count() . ' user(s) found to update avatars for.') + ->expectsQuestion('Are you sure you want to proceed?', true); $responses = []; foreach ($users as $user) { - $pendingCommand->expectsOutput("ID {$user->id} - "); - $pendingCommand->expectsOutput('Updated'); + $pendingCommand->expectsOutput("[ID: {$user->id}] {$user->email} - Updated"); $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); } $requests = $this->mockHttpClient($responses); - $pendingCommand->assertExitCode(Command::SUCCESS); + $pendingCommand->assertExitCode(0); $pendingCommand->run(); - $userWithAvatars = User::query()->where('image_id', '!=', 0)->count(); - $this->assertEquals($users->count(), $userWithAvatars); + $this->assertEquals(0, User::query()->where('image_id', '=', 0)->count()); $this->assertEquals($users->count(), $requests->requestCount()); } public function test_command_runs_update_all_users_avatar_errors() { - config()->set(['services.disable_services' => false]); - /** @var Collection|User[] $users */ - $users = User::query()->get(); + $users = array_values(User::query()->get()->all()); - $pendingCommand = $this->artisan(RefreshAvatarCommand::class, ['--all' => true, '-f' => true]); + $pendingCommand = $this->artisan('bookstack:refresh-avatar --all -f'); $pendingCommand - ->expectsOutput($users->count() . ' user(s) found.') - ->expectsQuestion('Are you sure you want to refresh avatars for ALL USERS?', true); + ->expectsOutput(count($users) . ' user(s) found to update avatars for.') + ->expectsQuestion('Are you sure you want to proceed?', true); $responses = []; - foreach ($users as $key => $user) { - $pendingCommand->expectsOutput("ID {$user->id} - "); - - if ($key == 1) { - $pendingCommand->expectsOutput('Not updated'); + foreach ($users as $index => $user) { + if ($index === 0) { + $pendingCommand->expectsOutput("[ID: {$user->id}] {$user->email} - Not updated"); $responses[] = new Response(404); continue; } - $pendingCommand->expectsOutput('Updated'); + $pendingCommand->expectsOutput("[ID: {$user->id}] {$user->email} - Updated"); $responses[] = new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData()); } $requests = $this->mockHttpClient($responses); - $pendingCommand->assertExitCode(Command::FAILURE); + $pendingCommand->assertExitCode(1); $pendingCommand->run(); $userWithAvatars = User::query()->where('image_id', '!=', 0)->count(); - $this->assertEquals($users->count() - 1, $userWithAvatars); - $this->assertEquals($users->count(), $requests->requestCount()); + $this->assertEquals(count($users) - 1, $userWithAvatars); + $this->assertEquals(count($users), $requests->requestCount()); } public function test_saying_no_to_confirmation_update_all_users_avatar() { $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); - config()->set(['services.disable_services' => false]); - /** @var UserAvatars $avatar */ - $avatar = app()->make(UserAvatars::class); + $this->artisan('bookstack:refresh-avatar --all -f') + ->expectsQuestion('Are you sure you want to proceed?', false) + ->assertExitCode(0); - /** @var Collection|User[] $users */ - $users = User::query()->get(); - foreach ($users as $user) { - $avatar->destroyAllForUser($user); - } - - $this->artisan(RefreshAvatarCommand::class, ['--all' => true, '-f' => true]) - ->expectsQuestion('Are you sure you want to refresh avatars for ALL USERS?', false) - ->assertExitCode(Command::SUCCESS); - - $userWithAvatars = User::query()->where('image_id', '=', 0)->count(); - $this->assertEquals($users->count(), $userWithAvatars); $this->assertEquals(0, $requests->requestCount()); } } From 89645759731bfd23cbaea3bb6d971f32579d5416 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 19 Sep 2023 20:09:33 +0100 Subject: [PATCH 37/97] Search: Added support for escaped exact terms Also prevented use of empty exact matches. Prevents issues when attempting to use exact search terms in inputs for just search terms, and use of single " chars within search terms since these would get auto-promoted to exacts. For #4535 --- app/Search/SearchOptions.php | 35 ++++++++++++++++++++---------- tests/Entity/SearchOptionsTest.php | 35 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/app/Search/SearchOptions.php b/app/Search/SearchOptions.php index 0bf9c3116..af146d5fd 100644 --- a/app/Search/SearchOptions.php +++ b/app/Search/SearchOptions.php @@ -44,8 +44,8 @@ class SearchOptions $inputs = $request->only(['search', 'types', 'filters', 'exact', 'tags']); $parsedStandardTerms = static::parseStandardTermString($inputs['search'] ?? ''); - $instance->searches = $parsedStandardTerms['terms']; - $instance->exacts = $parsedStandardTerms['exacts']; + $instance->searches = array_filter($parsedStandardTerms['terms']); + $instance->exacts = array_filter($parsedStandardTerms['exacts']); array_push($instance->exacts, ...array_filter($inputs['exact'] ?? [])); @@ -78,7 +78,7 @@ class SearchOptions ]; $patterns = [ - 'exacts' => '/"(.*?)"/', + 'exacts' => '/"(.*?)(? '/\[(.*?)\]/', 'filters' => '/\{(.*?)\}/', ]; @@ -93,6 +93,11 @@ class SearchOptions } } + // Unescape exacts + foreach ($terms['exacts'] as $index => $exact) { + $terms['exacts'][$index] = str_replace('\"', '"', $exact); + } + // Parse standard terms $parsedStandardTerms = static::parseStandardTermString($searchString); array_push($terms['searches'], ...$parsedStandardTerms['terms']); @@ -106,12 +111,19 @@ class SearchOptions } $terms['filters'] = $splitFilters; + // Filter down terms where required + $terms['exacts'] = array_filter($terms['exacts']); + $terms['searches'] = array_filter($terms['searches']); + return $terms; } /** * Parse a standard search term string into individual search terms and - * extract any exact terms searches to be made. + * convert any required terms to exact matches. This is done since some + * characters will never be in the standard index, since we use them as + * delimiters, and therefore we convert a term to be exact if it + * contains one of those delimiter characters. * * @return array{terms: array, exacts: array} */ @@ -129,8 +141,8 @@ class SearchOptions continue; } - $parsedList = (strpbrk($searchTerm, $indexDelimiters) === false) ? 'terms' : 'exacts'; - $parsed[$parsedList][] = $searchTerm; + $becomeExact = (strpbrk($searchTerm, $indexDelimiters) !== false); + $parsed[$becomeExact ? 'exacts' : 'terms'][] = $searchTerm; } return $parsed; @@ -141,20 +153,21 @@ class SearchOptions */ public function toString(): string { - $string = implode(' ', $this->searches ?? []); + $parts = $this->searches; foreach ($this->exacts as $term) { - $string .= ' "' . $term . '"'; + $escaped = str_replace('"', '\"', $term); + $parts[] = '"' . $escaped . '"'; } foreach ($this->tags as $term) { - $string .= " [{$term}]"; + $parts[] = "[{$term}]"; } foreach ($this->filters as $filterName => $filterVal) { - $string .= ' {' . $filterName . ($filterVal ? ':' . $filterVal : '') . '}'; + $parts[] = '{' . $filterName . ($filterVal ? ':' . $filterVal : '') . '}'; } - return $string; + return implode(' ', $parts); } } diff --git a/tests/Entity/SearchOptionsTest.php b/tests/Entity/SearchOptionsTest.php index cac9c67f1..8bc9d02e4 100644 --- a/tests/Entity/SearchOptionsTest.php +++ b/tests/Entity/SearchOptionsTest.php @@ -3,6 +3,7 @@ namespace Tests\Entity; use BookStack\Search\SearchOptions; +use Illuminate\Http\Request; use Tests\TestCase; class SearchOptionsTest extends TestCase @@ -17,6 +18,13 @@ class SearchOptionsTest extends TestCase $this->assertEquals(['is_tree' => ''], $options->filters); } + public function test_from_string_properly_parses_escaped_quotes() + { + $options = SearchOptions::fromString('"\"cat\"" surprise "\"\"" "\"donkey" "\""'); + + $this->assertEquals(['"cat"', '""', '"donkey', '"'], $options->exacts); + } + public function test_to_string_includes_all_items_in_the_correct_format() { $expected = 'cat "dog" [tag=good] {is_tree}'; @@ -32,6 +40,15 @@ class SearchOptionsTest extends TestCase } } + public function test_to_string_escapes_quotes_as_expected() + { + $options = new SearchOptions(); + $options->exacts = ['"cat"', '""', '"donkey', '"']; + + $output = $options->toString(); + $this->assertEquals('"\"cat\"" "\"\"" "\"donkey" "\""', $output); + } + public function test_correct_filter_values_are_set_from_string() { $opts = SearchOptions::fromString('{is_tree} {name:dan} {cat:happy}'); @@ -42,4 +59,22 @@ class SearchOptionsTest extends TestCase 'cat' => 'happy', ], $opts->filters); } + public function test_it_cannot_parse_out_empty_exacts() + { + $options = SearchOptions::fromString('"" test ""'); + + $this->assertEmpty($options->exacts); + $this->assertCount(1, $options->searches); + } + + public function test_from_request_properly_parses_exacts_from_search_terms() + { + $request = new Request([ + 'search' => 'biscuits "cheese" "" "baked beans"' + ]); + + $options = SearchOptions::fromRequest($request); + $this->assertEquals(["biscuits"], $options->searches); + $this->assertEquals(['"cheese"', '""', '"baked', 'beans"'], $options->exacts); + } } From cb9c3fc9f5a0f0bdca03cdeb8b6446155cdc2cd4 Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Fri, 22 Sep 2023 10:49:37 +0200 Subject: [PATCH 38/97] Fix Dark theme --- app/App/HomeController.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/App/HomeController.php b/app/App/HomeController.php index d971247df..a2bb151de 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -148,8 +148,12 @@ class HomeController extends Controller public function manifest() { $manifest = config('manifest'); - - $manifest["background_color"] = setting('app-color'); + + if (setting()->getForCurrentUser('dark-mode-enabled')){ + $manifest["background_color"] = setting('app-color-dark'); + }else{ + $manifest["background_color"] = setting('app-color'); + } return response()->json($manifest); } From f910424fa3190d7d8fb15fa79f776ec77b2910a6 Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Fri, 22 Sep 2023 11:00:41 +0200 Subject: [PATCH 39/97] Implementation of required changes --- app/App/PwaManifestBuilder.php | 83 ++++++++++++++++++++++++++++++++++ app/Config/manifest.php | 55 ---------------------- routes/web.php | 3 +- 3 files changed, 85 insertions(+), 56 deletions(-) create mode 100644 app/App/PwaManifestBuilder.php delete mode 100644 app/Config/manifest.php diff --git a/app/App/PwaManifestBuilder.php b/app/App/PwaManifestBuilder.php new file mode 100644 index 000000000..4c517072c --- /dev/null +++ b/app/App/PwaManifestBuilder.php @@ -0,0 +1,83 @@ + config('app.name'), + "short_name" => config('app.name'), + "start_url" => "./", + "scope" => ".", + "display" => "standalone", + "background_color" => (setting()->getForCurrentUser('dark-mode-enabled') ? setting('app-color-dark') : setting('app-color')), + "description" => config('app.name'), + "theme_color" => setting('app-color'), + "launch_handler" => [ + "client_mode" => "focus-existing" + ], + "orientation" => "portrait", + "icons" => [ + [ + "src" => setting('app-icon-64') ?: url('/icon-64.png'), + "sizes" => "64x64", + "type" => "image/png" + ], + [ + "src" => setting('app-icon-32') ?: url('/icon-32.png'), + "sizes" => "32x32", + "type" => "image/png" + ], + [ + "src" => setting('app-icon-128') ?: url('/icon-128.png'), + "sizes" => "128x128", + "type" => "image/png" + ], + [ + "src" => setting('app-icon-180') ?: url('/icon-180.png'), + "sizes" => "180x180", + "type" => "image/png" + ], + [ + "src" => setting('app-icon') ?: url('/icon.png'), + "sizes" => "256x256", + "type" => "image/png" + ], + [ + "src" => "icon.ico", + "sizes" => "48x48", + "type" => "image/vnd.microsoft.icon" + ], + [ + "src" => "favicon.ico", + "sizes" => "48x48", + "type" => "image/vnd.microsoft.icon" + ], + ], + ]; + } + + /** + * Serve the application manifest. + * Ensures a 'manifest.json' + */ + public function manifest() + { + return response()->json($this->GenerateManifest()); + } +} diff --git a/app/Config/manifest.php b/app/Config/manifest.php deleted file mode 100644 index 640ba70e6..000000000 --- a/app/Config/manifest.php +++ /dev/null @@ -1,55 +0,0 @@ - (env('APP_NAME' | 'BookStack') ??'BookStack' ), - "short_name" => "bookstack", - "start_url" => "./", - "scope" => ".", - "display" => "standalone", - "background_color" => "#fff", - "description" =>( env('APP_NAME' | 'BookStack') ??'BookStack'), - "categories" => [ - "productivity", - "lifestyle" - ], - "launch_handler" => [ - "client_mode" => "focus-existing" - ], - "orientation" => "portrait", - "icons" => [ - [ - "src" => "/icon-64.png", - "sizes" => "64x64", - "type" => "image/png" - ], - [ - "src" => "/icon-32.png", - "sizes" => "32x32", - "type" => "image/png" - ], - [ - "src" => "/icon-128.png", - "sizes" => "128x128", - "type" => "image/png" - ], - [ - "src" => "icon-180.png", - "sizes" => "180x180", - "type" => "image/png" - ], - [ - "src" => "icon.png", - "sizes" => "256x256", - "type" => "image/png" - ], - [ - "src" => "icon.ico", - "sizes" => "48x48", - "type" => "image/vnd.microsoft.icon" - ], - [ - "src" => "favicon.ico", - "sizes" => "48x48", - "type" => "image/vnd.microsoft.icon" - ], - ], -]; \ No newline at end of file diff --git a/routes/web.php b/routes/web.php index 8116cdaf8..6bc563480 100644 --- a/routes/web.php +++ b/routes/web.php @@ -5,6 +5,7 @@ use BookStack\Activity\Controllers as ActivityControllers; use BookStack\Api\ApiDocsController; use BookStack\Api\UserApiTokenController; use BookStack\App\HomeController; +use BookStack\App\PwaManifestBuilder; use BookStack\Entities\Controllers as EntityControllers; use BookStack\Http\Middleware\VerifyCsrfToken; use BookStack\Permissions\PermissionsController; @@ -20,7 +21,7 @@ use Illuminate\View\Middleware\ShareErrorsFromSession; Route::get('/status', [SettingControllers\StatusController::class, 'show']); Route::get('/robots.txt', [HomeController::class, 'robots']); Route::get('/favicon.ico', [HomeController::class, 'favicon']); -Route::get('/manifest.json', [HomeController::class, 'manifest']); +Route::get('/manifest.json', [PwaManifestBuilder::class, 'manifest']); // Authenticated routes... Route::middleware('auth')->group(function () { From 9b99664bff7e25da85b3798db408a8294a35b80b Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Fri, 22 Sep 2023 11:15:13 +0200 Subject: [PATCH 40/97] Additional Tweaks and FIxes --- app/App/PwaManifestBuilder.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/App/PwaManifestBuilder.php b/app/App/PwaManifestBuilder.php index 4c517072c..533c48413 100644 --- a/app/App/PwaManifestBuilder.php +++ b/app/App/PwaManifestBuilder.php @@ -20,13 +20,13 @@ class PwaManifestBuilder extends Controller private function GenerateManifest() { return [ - "name" => config('app.name'), - "short_name" => config('app.name'), + "name" => setting('app-name'), + "short_name" => setting('app-name'), "start_url" => "./", - "scope" => ".", + "scope" => "/", "display" => "standalone", "background_color" => (setting()->getForCurrentUser('dark-mode-enabled') ? setting('app-color-dark') : setting('app-color')), - "description" => config('app.name'), + "description" => setting('app-name'), "theme_color" => setting('app-color'), "launch_handler" => [ "client_mode" => "focus-existing" From 2a2f893fcc045cd1bf7c70d0f40edfed389b1dce Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Fri, 22 Sep 2023 11:18:10 +0200 Subject: [PATCH 41/97] Formating Fixes --- app/App/HomeController.php | 105 ++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 61 deletions(-) diff --git a/app/App/HomeController.php b/app/App/HomeController.php index d0b326c8a..ab62e6d57 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -18,41 +18,41 @@ use Illuminate\Http\Request; class HomeController extends Controller { /** - * Display the homepage. - */ + * Display the homepage. + */ public function index(Request $request, ActivityQueries $activities) { $activity = $activities->latest(10); $draftPages = []; - + if ($this->isSignedIn()) { $draftPages = Page::visible() - ->where('draft', '=', true) - ->where('created_by', '=', user()->id) - ->orderBy('updated_at', 'desc') - ->with('book') - ->take(6) - ->get(); + ->where('draft', '=', true) + ->where('created_by', '=', user()->id) + ->orderBy('updated_at', 'desc') + ->with('book') + ->take(6) + ->get(); } - + $recentFactor = count($draftPages) > 0 ? 0.5 : 1; $recents = $this->isSignedIn() ? - (new RecentlyViewed())->run(12 * $recentFactor, 1) - : Book::visible()->orderBy('created_at', 'desc')->take(12 * $recentFactor)->get(); + (new RecentlyViewed())->run(12 * $recentFactor, 1) + : Book::visible()->orderBy('created_at', 'desc')->take(12 * $recentFactor)->get(); $favourites = (new TopFavourites())->run(6); $recentlyUpdatedPages = Page::visible()->with('book') - ->where('draft', false) - ->orderBy('updated_at', 'desc') - ->take($favourites->count() > 0 ? 5 : 10) - ->select(Page::$listAttributes) - ->get(); - + ->where('draft', false) + ->orderBy('updated_at', 'desc') + ->take($favourites->count() > 0 ? 5 : 10) + ->select(Page::$listAttributes) + ->get(); + $homepageOptions = ['default', 'books', 'bookshelves', 'page']; $homepageOption = setting('app-homepage-type', 'default'); if (!in_array($homepageOption, $homepageOptions)) { $homepageOption = 'default'; } - + $commonData = [ 'activity' => $activity, 'recents' => $recents, @@ -60,7 +60,7 @@ class HomeController extends Controller 'draftPages' => $draftPages, 'favourites' => $favourites, ]; - + // Add required list ordering & sorting for books & shelves views. if ($homepageOption === 'bookshelves' || $homepageOption === 'books') { $key = $homepageOption; @@ -70,27 +70,27 @@ class HomeController extends Controller 'created_at' => trans('common.sort_created_at'), 'updated_at' => trans('common.sort_updated_at'), ]); - + $commonData = array_merge($commonData, [ 'view' => $view, 'listOptions' => $listOptions, ]); } - + if ($homepageOption === 'bookshelves') { $shelves = app()->make(BookshelfRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); $data = array_merge($commonData, ['shelves' => $shelves]); - + return view('home.shelves', $data); } - + if ($homepageOption === 'books') { $books = app()->make(BookRepo::class)->getAllPaginated(18, $commonData['listOptions']->getSort(), $commonData['listOptions']->getOrder()); $data = array_merge($commonData, ['books' => $books]); - + return view('home.books', $data); } - + if ($homepageOption === 'page') { $homepageSetting = setting('app-homepage', '0:'); $id = intval(explode(':', $homepageSetting)[0]); @@ -98,63 +98,46 @@ class HomeController extends Controller $customHomepage = Page::query()->where('draft', '=', false)->findOrFail($id); $pageContent = new PageContent($customHomepage); $customHomepage->html = $pageContent->render(false); - + return view('home.specific-page', array_merge($commonData, ['customHomepage' => $customHomepage])); } - + return view('home.default', $commonData); } - + /** - * Show the view for /robots.txt. - */ + * Show the view for /robots.txt. + */ public function robots() { $sitePublic = setting('app-public', false); $allowRobots = config('app.allow_robots'); - + if ($allowRobots === null) { $allowRobots = $sitePublic; } - + return response() - ->view('misc.robots', ['allowRobots' => $allowRobots]) - ->header('Content-Type', 'text/plain'); + ->view('misc.robots', ['allowRobots' => $allowRobots]) + ->header('Content-Type', 'text/plain'); } - + /** - * Show the route for 404 responses. - */ + * Show the route for 404 responses. + */ public function notFound() { return response()->view('errors.404', [], 404); } - + /** - * Serve the application favicon. - * Ensures a 'favicon.ico' file exists at the web root location (if writable) to be served - * directly by the webserver in the future. - */ + * Serve the application favicon. + * Ensures a 'favicon.ico' file exists at the web root location (if writable) to be served + * directly by the webserver in the future. + */ public function favicon(FaviconHandler $favicons) { $exists = $favicons->restoreOriginalIfNotExists(); return response()->file($exists ? $favicons->getPath() : $favicons->getOriginalPath()); } - - /** - * Serve the application manifest. - * Ensures a 'manifest.json' - */ - public function manifest() - { - $manifest = config('manifest'); - - if (setting()->getForCurrentUser('dark-mode-enabled')){ - $manifest["background_color"] = setting('app-color-dark'); - }else{ - $manifest["background_color"] = setting('app-color'); - } - - return response()->json($manifest); - } -} +} \ No newline at end of file From 7e09c9a14725954cca5cc987f35b2bfed42d2057 Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Fri, 22 Sep 2023 11:19:17 +0200 Subject: [PATCH 42/97] Update HomeController.php --- app/App/HomeController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/App/HomeController.php b/app/App/HomeController.php index ab62e6d57..24b7c3ed8 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -140,4 +140,4 @@ class HomeController extends Controller $exists = $favicons->restoreOriginalIfNotExists(); return response()->file($exists ? $favicons->getPath() : $favicons->getOriginalPath()); } -} \ No newline at end of file +} From 10e8e1a88dcec88b5393537f509c3d266e224fe7 Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Fri, 22 Sep 2023 11:19:34 +0200 Subject: [PATCH 43/97] New line fix --- app/App/HomeController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/App/HomeController.php b/app/App/HomeController.php index ab62e6d57..24b7c3ed8 100644 --- a/app/App/HomeController.php +++ b/app/App/HomeController.php @@ -140,4 +140,4 @@ class HomeController extends Controller $exists = $favicons->restoreOriginalIfNotExists(); return response()->file($exists ? $favicons->getPath() : $favicons->getOriginalPath()); } -} \ No newline at end of file +} From 57791c14663ffa19f22e77b66a93b66d7f103dbf Mon Sep 17 00:00:00 2001 From: JonatanRek Date: Fri, 22 Sep 2023 11:31:24 +0200 Subject: [PATCH 44/97] Fix Reloading changes on dark mode switch --- app/App/PwaManifestBuilder.php | 21 +++++++-------------- resources/views/layouts/base.blade.php | 4 ++-- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/app/App/PwaManifestBuilder.php b/app/App/PwaManifestBuilder.php index 533c48413..f18e01248 100644 --- a/app/App/PwaManifestBuilder.php +++ b/app/App/PwaManifestBuilder.php @@ -2,23 +2,16 @@ namespace BookStack\App; -use BookStack\Activity\ActivityQueries; -use BookStack\Entities\Models\Book; -use BookStack\Entities\Models\Page; -use BookStack\Entities\Queries\RecentlyViewed; -use BookStack\Entities\Queries\TopFavourites; -use BookStack\Entities\Repos\BookRepo; -use BookStack\Entities\Repos\BookshelfRepo; -use BookStack\Entities\Tools\PageContent; use BookStack\Http\Controller; -use BookStack\Uploads\FaviconHandler; -use BookStack\Util\SimpleListOptions; -use Illuminate\Http\Request; class PwaManifestBuilder extends Controller { private function GenerateManifest() { + dump(setting()->getForCurrentUser('dark-mode-enabled')); + dump(setting('app-color-dark')); + dump(setting('app-color')); + return [ "name" => setting('app-name'), "short_name" => setting('app-name'), @@ -27,7 +20,7 @@ class PwaManifestBuilder extends Controller "display" => "standalone", "background_color" => (setting()->getForCurrentUser('dark-mode-enabled') ? setting('app-color-dark') : setting('app-color')), "description" => setting('app-name'), - "theme_color" => setting('app-color'), + "theme_color" => (setting()->getForCurrentUser('dark-mode-enabled') ? setting('app-color-dark') : setting('app-color')), "launch_handler" => [ "client_mode" => "focus-existing" ], @@ -59,12 +52,12 @@ class PwaManifestBuilder extends Controller "type" => "image/png" ], [ - "src" => "icon.ico", + "src" => public_path('icon.ico'), "sizes" => "48x48", "type" => "image/vnd.microsoft.icon" ], [ - "src" => "favicon.ico", + "src" => public_path('favicon.ico'), "sizes" => "48x48", "type" => "image/vnd.microsoft.icon" ], diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index 69a7e148e..13ad6a4fd 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -10,7 +10,7 @@ - + @@ -31,7 +31,7 @@ - + @yield('head') From fb417828a49e4e7480dd55739a094c12c86c71a8 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 23 Sep 2023 12:47:24 +0100 Subject: [PATCH 45/97] Readme: Updated badges, sponsors and top links --- readme.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/readme.md b/readme.md index aa335f9da..9ce5aabcd 100644 --- a/readme.md +++ b/readme.md @@ -10,7 +10,8 @@ [![Repo Stats](https://img.shields.io/static/v1?label=GitHub+project&message=stats&color=f27e3f)](https://gh-stats.bookstackapp.com/) [![Discord](https://img.shields.io/static/v1?label=Discord&message=chat&color=738adb&logo=discord)](https://discord.gg/ztkBqR2) [![Mastodon](https://img.shields.io/static/v1?label=Mastodon&message=@bookstack&color=595aff&logo=mastodon)](https://fosstodon.org/@bookstack) -[![Twitter](https://img.shields.io/static/v1?label=Twitter&message=@bookstack_app&color=1d9bf0&logo=twitter)](https://twitter.com/bookstack_app) +[![X - Formerly Twitter](https://img.shields.io/static/v1?label=Follow&message=@bookstack_app&color=1d9bf0&logo=x)](https://x.com/bookstack_app) + [![PeerTube](https://img.shields.io/static/v1?label=PeerTube&message=bookstack@foss.video&color=f2690d&logo=peertube)](https://foss.video/c/bookstack) [![YouTube](https://img.shields.io/static/v1?label=YouTube&message=bookstackapp&color=ff0000&logo=youtube)](https://www.youtube.com/bookstackapp) @@ -24,6 +25,7 @@ A platform for storing and organising information and documentation. Details for * [BookStack Blog](https://www.bookstackapp.com/blog) * [Issue List](https://github.com/BookStackApp/BookStack/issues) * [Discord Chat](https://discord.gg/ztkBqR2) +* [Support Options](https://www.bookstackapp.com/support/) ## 📚 Project Definition @@ -57,6 +59,10 @@ Note: Listed services are not tested, vetted nor supported by the official BookS Practicali + + + Stellar Hosted + Torutec From f77bb01b514a0ae6a469ec33a40d0d053d804d40 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 23 Sep 2023 13:41:10 +0100 Subject: [PATCH 46/97] Search: Added further backslash handling Added due to now not being able to perform an exact search where contains a trailing backslash. Now all backslashes in exact terms are consided escape chars and require escaping themselves. Potential breaking change due to search syntax handling change. Related to #4535. --- app/Search/SearchOptions.php | 31 ++++++++++++++++++++++---- tests/Commands/ResetMfaCommandTest.php | 2 +- tests/Entity/EntitySearchTest.php | 4 ++-- tests/Entity/SearchOptionsTest.php | 10 ++++----- 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/app/Search/SearchOptions.php b/app/Search/SearchOptions.php index af146d5fd..d38fc8d57 100644 --- a/app/Search/SearchOptions.php +++ b/app/Search/SearchOptions.php @@ -78,7 +78,7 @@ class SearchOptions ]; $patterns = [ - 'exacts' => '/"(.*?)(? '/"((?:\\\\.|[^"\\\\])*)"/', 'tags' => '/\[(.*?)\]/', 'filters' => '/\{(.*?)\}/', ]; @@ -93,9 +93,9 @@ class SearchOptions } } - // Unescape exacts + // Unescape exacts and backslash escapes foreach ($terms['exacts'] as $index => $exact) { - $terms['exacts'][$index] = str_replace('\"', '"', $exact); + $terms['exacts'][$index] = static::decodeEscapes($exact); } // Parse standard terms @@ -118,6 +118,28 @@ class SearchOptions return $terms; } + /** + * Decode backslash escaping within the input string. + */ + protected static function decodeEscapes(string $input): string + { + $decoded = ""; + $escaping = false; + + foreach (str_split($input) as $char) { + if ($escaping) { + $decoded .= $char; + $escaping = false; + } else if ($char === '\\') { + $escaping = true; + } else { + $decoded .= $char; + } + } + + return $decoded; + } + /** * Parse a standard search term string into individual search terms and * convert any required terms to exact matches. This is done since some @@ -156,7 +178,8 @@ class SearchOptions $parts = $this->searches; foreach ($this->exacts as $term) { - $escaped = str_replace('"', '\"', $term); + $escaped = str_replace('\\', '\\\\', $term); + $escaped = str_replace('"', '\"', $escaped); $parts[] = '"' . $escaped . '"'; } diff --git a/tests/Commands/ResetMfaCommandTest.php b/tests/Commands/ResetMfaCommandTest.php index 85f8f6430..39c8c689b 100644 --- a/tests/Commands/ResetMfaCommandTest.php +++ b/tests/Commands/ResetMfaCommandTest.php @@ -11,7 +11,7 @@ class ResetMfaCommandTest extends TestCase public function test_command_requires_email_or_id_option() { $this->artisan('bookstack:reset-mfa') - ->expectsOutput('Either a --id= or --email= option must be provided.') + ->expectsOutputToContain('Either a --id= or --email= option must be provided.') ->assertExitCode(1); } diff --git a/tests/Entity/EntitySearchTest.php b/tests/Entity/EntitySearchTest.php index a070ce3fa..fbb47226e 100644 --- a/tests/Entity/EntitySearchTest.php +++ b/tests/Entity/EntitySearchTest.php @@ -466,10 +466,10 @@ class EntitySearchTest extends TestCase $search = $this->asEditor()->get('/search?term=' . urlencode('\\\\cat\\dog')); $search->assertSee($page->getUrl(), false); - $search = $this->asEditor()->get('/search?term=' . urlencode('"\\dog\\"')); + $search = $this->asEditor()->get('/search?term=' . urlencode('"\\dog\\\\"')); $search->assertSee($page->getUrl(), false); - $search = $this->asEditor()->get('/search?term=' . urlencode('"\\badger\\"')); + $search = $this->asEditor()->get('/search?term=' . urlencode('"\\badger\\\\"')); $search->assertDontSee($page->getUrl(), false); $search = $this->asEditor()->get('/search?term=' . urlencode('[\\Categorylike%\\fluffy]')); diff --git a/tests/Entity/SearchOptionsTest.php b/tests/Entity/SearchOptionsTest.php index 8bc9d02e4..ea4d727a4 100644 --- a/tests/Entity/SearchOptionsTest.php +++ b/tests/Entity/SearchOptionsTest.php @@ -20,9 +20,9 @@ class SearchOptionsTest extends TestCase public function test_from_string_properly_parses_escaped_quotes() { - $options = SearchOptions::fromString('"\"cat\"" surprise "\"\"" "\"donkey" "\""'); + $options = SearchOptions::fromString('"\"cat\"" surprise "\"\"" "\"donkey" "\"" "\\\\"'); - $this->assertEquals(['"cat"', '""', '"donkey', '"'], $options->exacts); + $this->assertEquals(['"cat"', '""', '"donkey', '"', '\\'], $options->exacts); } public function test_to_string_includes_all_items_in_the_correct_format() @@ -40,13 +40,13 @@ class SearchOptionsTest extends TestCase } } - public function test_to_string_escapes_quotes_as_expected() + public function test_to_string_escapes_as_expected() { $options = new SearchOptions(); - $options->exacts = ['"cat"', '""', '"donkey', '"']; + $options->exacts = ['"cat"', '""', '"donkey', '"', '\\', '\\"']; $output = $options->toString(); - $this->assertEquals('"\"cat\"" "\"\"" "\"donkey" "\""', $output); + $this->assertEquals('"\"cat\"" "\"\"" "\"donkey" "\"" "\\\\" "\\\\\""', $output); } public function test_correct_filter_values_are_set_from_string() From c3b4128a38e2c07e87492c33ead2186bdf531904 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 24 Sep 2023 09:31:44 +0100 Subject: [PATCH 47/97] Homepage: Added tags button to non-default home views For #4558 --- resources/views/home/books.blade.php | 4 ++++ resources/views/home/shelves.blade.php | 4 ++++ tests/HomepageTest.php | 17 ++++++++++++----- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/resources/views/home/books.blade.php b/resources/views/home/books.blade.php index 95c0c9df2..a2f2bf796 100644 --- a/resources/views/home/books.blade.php +++ b/resources/views/home/books.blade.php @@ -19,6 +19,10 @@ @endif @include('entities.view-toggle', ['view' => $view, 'type' => 'books']) + + @icon('tag') + {{ trans('entities.tags_view_tags') }} + @include('home.parts.expand-toggle', ['classes' => 'text-link', 'target' => '.entity-list.compact .entity-item-snippet', 'key' => 'home-details']) @include('common.dark-mode-toggle', ['classes' => 'icon-list-item text-link'])
    diff --git a/resources/views/home/shelves.blade.php b/resources/views/home/shelves.blade.php index 9699d6b96..1265db29e 100644 --- a/resources/views/home/shelves.blade.php +++ b/resources/views/home/shelves.blade.php @@ -19,6 +19,10 @@ @endif @include('entities.view-toggle', ['view' => $view, 'type' => 'bookshelves']) + + @icon('tag') + {{ trans('entities.tags_view_tags') }} + @include('home.parts.expand-toggle', ['classes' => 'text-link', 'target' => '.entity-list.compact .entity-item-snippet', 'key' => 'home-details']) @include('common.dark-mode-toggle', ['classes' => 'icon-list-item text-link']) diff --git a/tests/HomepageTest.php b/tests/HomepageTest.php index eb552b2e2..977ae5256 100644 --- a/tests/HomepageTest.php +++ b/tests/HomepageTest.php @@ -126,9 +126,6 @@ class HomepageTest extends TestCase $homeVisit->assertSee('grid-card-content'); $homeVisit->assertSee('grid-card-footer'); $homeVisit->assertSee('featured-image-container'); - - $this->setSettings(['app-homepage-type' => false]); - $this->test_default_homepage_visible(); } public function test_set_bookshelves_homepage() @@ -145,9 +142,19 @@ class HomepageTest extends TestCase $homeVisit->assertSee('grid-card-content'); $homeVisit->assertSee('featured-image-container'); $this->withHtml($homeVisit)->assertElementContains('.grid-card', $shelf->name); + } - $this->setSettings(['app-homepage-type' => false]); - $this->test_default_homepage_visible(); + public function test_books_and_bookshelves_homepage_has_expected_actions() + { + $this->asEditor(); + + foreach (['bookshelves', 'books'] as $homepageType) { + $this->setSettings(['app-homepage-type' => $homepageType]); + + $html = $this->withHtml($this->get('/')); + $html->assertElementContains('.actions button', 'Dark Mode'); + $html->assertElementContains('.actions a[href$="/tags"]', 'View Tags'); + } } public function test_shelves_list_homepage_adheres_to_book_visibility_permissions() From d5a3bdb7aa7876f667256fb3e0d36ef46940adf9 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 24 Sep 2023 10:29:51 +0100 Subject: [PATCH 48/97] Header: Simplified, split and re-orgranised view file(s) - Moved "common" template partials, that are only used in layouts, to layouts/parts folder. - Simplified HTML structure of header template. - Extracted logo and links from header template to simplify. - Added header-links-start template for easier extension/customization without needing to override full list of links. - Added test to cover usage of this. For #4564 --- resources/sass/_header.scss | 4 +- resources/views/common/header.blade.php | 74 ------------------- resources/views/layouts/base.blade.php | 12 +-- .../parts}/custom-head.blade.php | 0 .../parts}/custom-styles.blade.php | 0 .../parts}/footer.blade.php | 0 .../parts/header-links-start.blade.php | 2 + .../layouts/parts/header-links.blade.php | 25 +++++++ .../views/layouts/parts/header-logo.blade.php | 8 ++ .../layouts/parts/header-search.blade.php | 20 +++++ .../parts}/header-user-menu.blade.php | 0 .../views/layouts/parts/header.blade.php | 25 +++++++ .../parts}/notifications.blade.php | 0 .../parts}/skip-to-content.blade.php | 0 resources/views/layouts/plain.blade.php | 4 +- tests/ThemeTest.php | 14 ++++ 16 files changed, 104 insertions(+), 84 deletions(-) delete mode 100644 resources/views/common/header.blade.php rename resources/views/{common => layouts/parts}/custom-head.blade.php (100%) rename resources/views/{common => layouts/parts}/custom-styles.blade.php (100%) rename resources/views/{common => layouts/parts}/footer.blade.php (100%) create mode 100644 resources/views/layouts/parts/header-links-start.blade.php create mode 100644 resources/views/layouts/parts/header-links.blade.php create mode 100644 resources/views/layouts/parts/header-logo.blade.php create mode 100644 resources/views/layouts/parts/header-search.blade.php rename resources/views/{common => layouts/parts}/header-user-menu.blade.php (100%) create mode 100644 resources/views/layouts/parts/header.blade.php rename resources/views/{common => layouts/parts}/notifications.blade.php (100%) rename resources/views/{common => layouts/parts}/skip-to-content.blade.php (100%) diff --git a/resources/sass/_header.scss b/resources/sass/_header.scss index c1b6af4c6..4a4c70401 100644 --- a/resources/sass/_header.scss +++ b/resources/sass/_header.scss @@ -2,12 +2,12 @@ * Includes the main navigation header and the faded toolbar. */ -header .grid { +header.grid { grid-template-columns: minmax(max-content, 2fr) 1fr minmax(max-content, 2fr); } @include smaller-than($l) { - header .grid { + header.grid { grid-template-columns: 1fr; grid-row-gap: 0; } diff --git a/resources/views/common/header.blade.php b/resources/views/common/header.blade.php deleted file mode 100644 index 86ad3563d..000000000 --- a/resources/views/common/header.blade.php +++ /dev/null @@ -1,74 +0,0 @@ - diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index f303aff26..f9dbc68b4 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -32,8 +32,8 @@ @yield('head') - @include('common.custom-styles') - @include('common.custom-head') + @include('layouts.parts.custom-styles') + @include('layouts.parts.custom-head') @stack('head') @@ -48,15 +48,15 @@ class="@stack('body-class')"> @include('layouts.parts.base-body-start') - @include('common.skip-to-content') - @include('common.notifications') - @include('common.header') + @include('layouts.parts.skip-to-content') + @include('layouts.parts.notifications') + @include('layouts.parts.header')
    @yield('content')
    - @include('common.footer') + @include('layouts.parts.footer') diff --git a/routes/web.php b/routes/web.php index c7fc92fc7..9f5e84c62 100644 --- a/routes/web.php +++ b/routes/web.php @@ -142,6 +142,7 @@ Route::middleware('auth')->group(function () { Route::post('/images/drawio', [UploadControllers\DrawioImageController::class, 'create']); Route::get('/images/edit/{id}', [UploadControllers\ImageController::class, 'edit']); Route::put('/images/{id}/file', [UploadControllers\ImageController::class, 'updateFile']); + Route::put('/images/{id}/rebuild-thumbnails', [UploadControllers\ImageController::class, 'rebuildThumbnails']); Route::put('/images/{id}', [UploadControllers\ImageController::class, 'update']); Route::delete('/images/{id}', [UploadControllers\ImageController::class, 'destroy']); From 5c318a45b8afd11f6454f4a558a9616786c1a467 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 30 Sep 2023 12:09:29 +0100 Subject: [PATCH 55/97] Images: Reverted some thumbnails to be on-demand generated Added since we can't always be sure of future image usage, and in many cases we don't generate ahead-of-time. Also: - Simplified image handling on certain models. - Updated various string handling operations to use newer functions. --- app/Entities/Models/Book.php | 15 +++-------- app/Entities/Models/Bookshelf.php | 20 +++++--------- app/Uploads/Image.php | 5 ++-- app/Uploads/ImageService.php | 45 ++++++++++++++++--------------- app/Users/Models/User.php | 2 +- 5 files changed, 38 insertions(+), 49 deletions(-) diff --git a/app/Entities/Models/Book.php b/app/Entities/Models/Book.php index fc4556857..f54a0bf2d 100644 --- a/app/Entities/Models/Book.php +++ b/app/Entities/Models/Book.php @@ -40,26 +40,19 @@ class Book extends Entity implements HasCoverImage /** * Returns book cover image, if book cover not exists return default cover image. - * - * @param int $width - Width of the image - * @param int $height - Height of the image - * - * @return string */ - public function getBookCover($width = 440, $height = 250) + public function getBookCover(int $width = 440, int $height = 250): string { $default = 'data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=='; - if (!$this->image_id) { + if (!$this->image_id || !$this->cover) { return $default; } try { - $cover = $this->cover ? url($this->cover->getThumb($width, $height, false)) : $default; + return $this->cover->getThumb($width, $height, false) ?? $default; } catch (Exception $err) { - $cover = $default; + return $default; } - - return $cover; } /** diff --git a/app/Entities/Models/Bookshelf.php b/app/Entities/Models/Bookshelf.php index ad52d9d37..4b44025a4 100644 --- a/app/Entities/Models/Bookshelf.php +++ b/app/Entities/Models/Bookshelf.php @@ -3,6 +3,7 @@ namespace BookStack\Entities\Models; use BookStack\Uploads\Image; +use Exception; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\BelongsToMany; @@ -49,28 +50,21 @@ class Bookshelf extends Entity implements HasCoverImage } /** - * Returns BookShelf cover image, if cover does not exists return default cover image. - * - * @param int $width - Width of the image - * @param int $height - Height of the image - * - * @return string + * Returns shelf cover image, if cover not exists return default cover image. */ - public function getBookCover($width = 440, $height = 250) + public function getBookCover(int $width = 440, int $height = 250): string { // TODO - Make generic, focused on books right now, Perhaps set-up a better image $default = 'data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=='; - if (!$this->image_id) { + if (!$this->image_id || !$this->cover) { return $default; } try { - $cover = $this->cover ? url($this->cover->getThumb($width, $height, false)) : $default; - } catch (\Exception $err) { - $cover = $default; + return $this->cover->getThumb($width, $height, false) ?? $default; + } catch (Exception $err) { + return $default; } - - return $cover; } /** diff --git a/app/Uploads/Image.php b/app/Uploads/Image.php index 5291dac05..5e197e750 100644 --- a/app/Uploads/Image.php +++ b/app/Uploads/Image.php @@ -45,13 +45,14 @@ class Image extends Model } /** - * Get an (already existing) thumbnail for this image. + * Get a thumbnail URL for this image. + * Attempts to generate the thumbnail if not already existing. * * @throws \Exception */ public function getThumb(?int $width, ?int $height, bool $keepRatio = false): ?string { - return app()->make(ImageService::class)->getThumbnail($this, $width, $height, $keepRatio); + return app()->make(ImageService::class)->getThumbnail($this, $width, $height, $keepRatio, false, true); } /** diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index 5db8defd3..c7e4aefad 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -21,23 +21,18 @@ use Intervention\Image\Exception\NotSupportedException; use Intervention\Image\Image as InterventionImage; use Intervention\Image\ImageManager; use League\Flysystem\WhitespacePathNormalizer; -use Psr\SimpleCache\InvalidArgumentException; use Symfony\Component\HttpFoundation\File\UploadedFile; use Symfony\Component\HttpFoundation\StreamedResponse; class ImageService { - protected ImageManager $imageTool; - protected Cache $cache; - protected FilesystemManager $fileSystem; - protected static array $supportedExtensions = ['jpg', 'jpeg', 'png', 'gif', 'webp']; - public function __construct(ImageManager $imageTool, FilesystemManager $fileSystem, Cache $cache) - { - $this->imageTool = $imageTool; - $this->fileSystem = $fileSystem; - $this->cache = $cache; + public function __construct( + protected ImageManager $imageTool, + protected FilesystemManager $fileSystem, + protected Cache $cache + ) { } /** @@ -206,7 +201,7 @@ class ImageService * Save image data for the given path in the public space, if possible, * for the provided storage mechanism. */ - protected function saveImageDataInPublicSpace(Storage $storage, string $path, string $data) + protected function saveImageDataInPublicSpace(Storage $storage, string $path, string $data): void { $storage->put($path, $data); @@ -269,8 +264,14 @@ class ImageService * * @throws Exception */ - public function getThumbnail(Image $image, ?int $width, ?int $height, bool $keepRatio = false, bool $shouldCreate = false): ?string - { + public function getThumbnail( + Image $image, + ?int $width, + ?int $height, + bool $keepRatio = false, + bool $shouldCreate = false, + bool $canCreate = false, + ): ?string { // Do not resize GIF images where we're not cropping if ($keepRatio && $this->isGif($image)) { return $this->getPublicUrl($image->path); @@ -305,7 +306,7 @@ class ImageService return $this->getPublicUrl($image->path); } - if (!$shouldCreate) { + if (!$shouldCreate && !$canCreate) { return null; } @@ -559,7 +560,7 @@ class ImageService // Check the image file exists && $disk->exists($imagePath) // Check the file is likely an image file - && strpos($disk->mimeType($imagePath), 'image/') === 0; + && str_starts_with($disk->mimeType($imagePath), 'image/'); } /** @@ -568,14 +569,14 @@ class ImageService */ protected function checkUserHasAccessToRelationOfImageAtPath(string $path): bool { - if (strpos($path, '/uploads/images/') === 0) { + if (str_starts_with($path, '/uploads/images/')) { $path = substr($path, 15); } // Strip thumbnail element from path if existing $originalPathSplit = array_filter(explode('/', $path), function (string $part) { - $resizedDir = (strpos($part, 'thumbs-') === 0 || strpos($part, 'scaled-') === 0); - $missingExtension = strpos($part, '.') === false; + $resizedDir = (str_starts_with($part, 'thumbs-') || str_starts_with($part, 'scaled-')); + $missingExtension = !str_contains($part, '.'); return !($resizedDir && $missingExtension); }); @@ -641,9 +642,9 @@ class ImageService $url = ltrim(trim($url), '/'); // Handle potential relative paths - $isRelative = strpos($url, 'http') !== 0; + $isRelative = !str_starts_with($url, 'http'); if ($isRelative) { - if (strpos(strtolower($url), 'uploads/images') === 0) { + if (str_starts_with(strtolower($url), 'uploads/images')) { return trim($url, '/'); } @@ -658,7 +659,7 @@ class ImageService foreach ($potentialHostPaths as $potentialBasePath) { $potentialBasePath = strtolower($potentialBasePath); - if (strpos(strtolower($url), $potentialBasePath) === 0) { + if (str_starts_with(strtolower($url), $potentialBasePath)) { return 'uploads/images/' . trim(substr($url, strlen($potentialBasePath)), '/'); } } @@ -679,7 +680,7 @@ class ImageService // region-based url will be used to prevent http issues. if (!$storageUrl && config('filesystems.images') === 's3') { $storageDetails = config('filesystems.disks.s3'); - if (strpos($storageDetails['bucket'], '.') === false) { + if (!str_contains($storageDetails['bucket'], '.')) { $storageUrl = 'https://' . $storageDetails['bucket'] . '.s3.amazonaws.com'; } else { $storageUrl = 'https://s3-' . $storageDetails['region'] . '.amazonaws.com/' . $storageDetails['bucket']; diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 39236c7e4..5bd308ae8 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -244,7 +244,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon } try { - $avatar = $this->avatar ? url($this->avatar->getThumb($size, $size, false)) : $default; + $avatar = $this->avatar?->getThumb($size, $size, false) ?? $default; } catch (Exception $err) { $avatar = $default; } From 97274a81401bc17ae3e79e77c7475f25a3193412 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 30 Sep 2023 12:29:49 +0100 Subject: [PATCH 56/97] Images: Added test to cover thubmnail regen endpoint --- tests/Helpers/FileProvider.php | 11 ++++++++++- tests/Uploads/ImageTest.php | 23 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/tests/Helpers/FileProvider.php b/tests/Helpers/FileProvider.php index 9e44697c7..99ee11efb 100644 --- a/tests/Helpers/FileProvider.php +++ b/tests/Helpers/FileProvider.php @@ -133,12 +133,21 @@ class FileProvider */ public function deleteAtRelativePath(string $path): void { - $fullPath = public_path($path); + $fullPath = $this->relativeToFullPath($path); if (file_exists($fullPath)) { unlink($fullPath); } } + /** + * Convert a relative path used by default in this provider to a full + * absolute local filesystem path. + */ + public function relativeToFullPath(string $path): string + { + return public_path($path); + } + /** * Delete all uploaded files. * To assist with cleanup. diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index a9684eef7..9943302d3 100644 --- a/tests/Uploads/ImageTest.php +++ b/tests/Uploads/ImageTest.php @@ -552,6 +552,29 @@ class ImageTest extends TestCase $this->files->deleteAtRelativePath($relPath); } + public function test_image_manager_regen_thumbnails() + { + $this->asEditor(); + $imageName = 'first-image.png'; + $relPath = $this->files->expectedImagePath('gallery', $imageName); + + $this->files->uploadGalleryImage($this, $imageName, $this->entities->page()->id); + $image = Image::first(); + + $resp = $this->get("/images/edit/{$image->id}"); + $this->withHtml($resp)->assertElementExists('button#image-manager-rebuild-thumbs'); + + $expectedThumbPath = dirname($relPath) . '/scaled-1680-/' . basename($relPath); + $this->files->deleteAtRelativePath($expectedThumbPath); + $this->assertFileDoesNotExist($this->files->relativeToFullPath($expectedThumbPath)); + + $resp = $this->put("/images/{$image->id}/rebuild-thumbnails"); + $resp->assertOk(); + + $this->assertFileExists($this->files->relativeToFullPath($expectedThumbPath)); + $this->files->deleteAtRelativePath($relPath); + } + protected function getTestProfileImage() { $imageName = 'profile.png'; From 40721433f721bf9233a57e5072493dba16c3552a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 30 Sep 2023 12:43:51 +0100 Subject: [PATCH 57/97] Image manager: Tweaked grid sizing to prevent massive items --- resources/sass/_components.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/sass/_components.scss b/resources/sass/_components.scss index c66a432bf..c1989c1f6 100644 --- a/resources/sass/_components.scss +++ b/resources/sass/_components.scss @@ -382,7 +382,7 @@ body.flexbox-support #entity-selector-wrap .popup-body .form-group { .image-manager-list { padding: 3px; display: grid; - grid-template-columns: repeat( auto-fit, minmax(140px, 1fr) ); + grid-template-columns: repeat( auto-fill, minmax(max(140px, 17%), 1fr) ); gap: 3px; z-index: 3; > div { From 7247e31936ebf630b28be5870a5760be920b0d90 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 30 Sep 2023 18:28:42 +0100 Subject: [PATCH 58/97] Images: Started refactor of image service To break it up. Also added better memory handling to other parts of the app. --- app/Entities/Tools/ExportFormatter.php | 2 +- .../Controllers/DrawioImageController.php | 8 +- .../Controllers/GalleryImageController.php | 5 + app/Uploads/Controllers/ImageController.php | 8 +- app/Uploads/ImageResizer.php | 95 ++++++ app/Uploads/ImageService.php | 299 +++--------------- app/Uploads/ImageStorage.php | 183 +++++++++++ lang/en/errors.php | 1 + 8 files changed, 340 insertions(+), 261 deletions(-) create mode 100644 app/Uploads/ImageResizer.php create mode 100644 app/Uploads/ImageStorage.php diff --git a/app/Entities/Tools/ExportFormatter.php b/app/Entities/Tools/ExportFormatter.php index 80b039b80..9a8c687b0 100644 --- a/app/Entities/Tools/ExportFormatter.php +++ b/app/Entities/Tools/ExportFormatter.php @@ -222,7 +222,7 @@ class ExportFormatter foreach ($imageTagsOutput[0] as $index => $imgMatch) { $oldImgTagString = $imgMatch; $srcString = $imageTagsOutput[2][$index]; - $imageEncoded = $this->imageService->imageUriToBase64($srcString); + $imageEncoded = $this->imageService->imageUrlToBase64($srcString); if ($imageEncoded === null) { $imageEncoded = $srcString; } diff --git a/app/Uploads/Controllers/DrawioImageController.php b/app/Uploads/Controllers/DrawioImageController.php index 35deada88..49f0c1655 100644 --- a/app/Uploads/Controllers/DrawioImageController.php +++ b/app/Uploads/Controllers/DrawioImageController.php @@ -10,11 +10,9 @@ use Illuminate\Http\Request; class DrawioImageController extends Controller { - protected $imageRepo; - - public function __construct(ImageRepo $imageRepo) - { - $this->imageRepo = $imageRepo; + public function __construct( + protected ImageRepo $imageRepo + ) { } /** diff --git a/app/Uploads/Controllers/GalleryImageController.php b/app/Uploads/Controllers/GalleryImageController.php index 02e58faf5..0696ca62b 100644 --- a/app/Uploads/Controllers/GalleryImageController.php +++ b/app/Uploads/Controllers/GalleryImageController.php @@ -5,6 +5,7 @@ namespace BookStack\Uploads\Controllers; use BookStack\Exceptions\ImageUploadException; use BookStack\Http\Controller; use BookStack\Uploads\ImageRepo; +use BookStack\Util\OutOfMemoryHandler; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Log; @@ -53,6 +54,10 @@ class GalleryImageController extends Controller return $this->jsonError(implode("\n", $exception->errors()['file'])); } + new OutOfMemoryHandler(function () { + return $this->jsonError(trans('errors.image_upload_memory_limit')); + }); + try { $imageUpload = $request->file('file'); $uploadedTo = $request->get('uploaded_to', 0); diff --git a/app/Uploads/Controllers/ImageController.php b/app/Uploads/Controllers/ImageController.php index edf1533fa..f92338bc8 100644 --- a/app/Uploads/Controllers/ImageController.php +++ b/app/Uploads/Controllers/ImageController.php @@ -11,7 +11,6 @@ use BookStack\Uploads\ImageService; use BookStack\Util\OutOfMemoryHandler; use Exception; use Illuminate\Http\Request; -use Illuminate\Validation\ValidationException; class ImageController extends Controller { @@ -39,9 +38,6 @@ class ImageController extends Controller /** * Update image details. - * - * @throws ImageUploadException - * @throws ValidationException */ public function update(Request $request, string $id) { @@ -75,6 +71,10 @@ class ImageController extends Controller $this->checkOwnablePermission('image-update', $image); $file = $request->file('file'); + new OutOfMemoryHandler(function () { + return $this->jsonError(trans('errors.image_upload_memory_limit')); + }); + try { $this->imageRepo->updateImageFile($image, $file); } catch (ImageUploadException $exception) { diff --git a/app/Uploads/ImageResizer.php b/app/Uploads/ImageResizer.php new file mode 100644 index 000000000..7a89b9d35 --- /dev/null +++ b/app/Uploads/ImageResizer.php @@ -0,0 +1,95 @@ +intervention->make($imageData); + } catch (NotSupportedException $e) { + throw new ImageUploadException(trans('errors.cannot_create_thumbs')); + } + + $this->orientImageToOriginalExif($thumb, $imageData); + + if ($keepRatio) { + $thumb->resize($width, $height, function ($constraint) { + $constraint->aspectRatio(); + $constraint->upsize(); + }); + } else { + $thumb->fit($width, $height); + } + + $thumbData = (string) $thumb->encode(); + + // Use original image data if we're keeping the ratio + // and the resizing does not save any space. + if ($keepRatio && strlen($thumbData) > strlen($imageData)) { + return $imageData; + } + + return $thumbData; + } + + /** + * Orientate the given intervention image based upon the given original image data. + * Intervention does have an `orientate` method but the exif data it needs is lost before it + * can be used (At least when created using binary string data) so we need to do some + * implementation on our side to use the original image data. + * Bulk of logic taken from: https://github.com/Intervention/image/blob/b734a4988b2148e7d10364b0609978a88d277536/src/Intervention/Image/Commands/OrientateCommand.php + * Copyright (c) Oliver Vogel, MIT License. + */ + protected function orientImageToOriginalExif(InterventionImage $image, string $originalData): void + { + if (!extension_loaded('exif')) { + return; + } + + $stream = Utils::streamFor($originalData)->detach(); + $exif = @exif_read_data($stream); + $orientation = $exif ? ($exif['Orientation'] ?? null) : null; + + switch ($orientation) { + case 2: + $image->flip(); + break; + case 3: + $image->rotate(180); + break; + case 4: + $image->rotate(180)->flip(); + break; + case 5: + $image->rotate(270)->flip(); + break; + case 6: + $image->rotate(270); + break; + case 7: + $image->rotate(90)->flip(); + break; + case 8: + $image->rotate(90); + break; + } + } +} diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index c7e4aefad..81d6add92 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -8,19 +8,16 @@ use BookStack\Entities\Models\Page; use BookStack\Exceptions\ImageUploadException; use ErrorException; use Exception; -use GuzzleHttp\Psr7\Utils; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Filesystem\FileNotFoundException; -use Illuminate\Contracts\Filesystem\Filesystem as Storage; +use Illuminate\Contracts\Filesystem\Filesystem as StorageDisk; use Illuminate\Filesystem\FilesystemAdapter; use Illuminate\Filesystem\FilesystemManager; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; use Intervention\Image\Exception\NotSupportedException; -use Intervention\Image\Image as InterventionImage; use Intervention\Image\ImageManager; -use League\Flysystem\WhitespacePathNormalizer; use Symfony\Component\HttpFoundation\File\UploadedFile; use Symfony\Component\HttpFoundation\StreamedResponse; @@ -31,79 +28,15 @@ class ImageService public function __construct( protected ImageManager $imageTool, protected FilesystemManager $fileSystem, - protected Cache $cache + protected Cache $cache, + protected ImageStorage $storage, ) { } - /** - * Get the storage that will be used for storing images. - */ - protected function getStorageDisk(string $imageType = ''): Storage - { - return $this->fileSystem->disk($this->getStorageDiskName($imageType)); - } - - /** - * Check if local secure image storage (Fetched behind authentication) - * is currently active in the instance. - */ - protected function usingSecureImages(string $imageType = 'gallery'): bool - { - return $this->getStorageDiskName($imageType) === 'local_secure_images'; - } - - /** - * Check if "local secure restricted" (Fetched behind auth, with permissions enforced) - * is currently active in the instance. - */ - protected function usingSecureRestrictedImages() - { - return config('filesystems.images') === 'local_secure_restricted'; - } - - /** - * Change the originally provided path to fit any disk-specific requirements. - * This also ensures the path is kept to the expected root folders. - */ - protected function adjustPathForStorageDisk(string $path, string $imageType = ''): string - { - $path = (new WhitespacePathNormalizer())->normalizePath(str_replace('uploads/images/', '', $path)); - - if ($this->usingSecureImages($imageType)) { - return $path; - } - - return 'uploads/images/' . $path; - } - - /** - * Get the name of the storage disk to use. - */ - protected function getStorageDiskName(string $imageType): string - { - $storageType = config('filesystems.images'); - $localSecureInUse = ($storageType === 'local_secure' || $storageType === 'local_secure_restricted'); - - // Ensure system images (App logo) are uploaded to a public space - if ($imageType === 'system' && $localSecureInUse) { - return 'local'; - } - - // Rename local_secure options to get our image specific storage driver which - // is scoped to the relevant image directories. - if ($localSecureInUse) { - return 'local_secure_images'; - } - - return $storageType; - } - /** * Saves a new image from an upload. * * @throws ImageUploadException - * - * @return mixed */ public function saveNewFromUpload( UploadedFile $uploadedFile, @@ -112,7 +45,7 @@ class ImageService int $resizeWidth = null, int $resizeHeight = null, bool $keepRatio = true - ) { + ): Image { $imageName = $uploadedFile->getClientOriginalName(); $imageData = file_get_contents($uploadedFile->getRealPath()); @@ -146,13 +79,13 @@ class ImageService */ public function saveNew(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image { - $storage = $this->getStorageDisk($type); + $disk = $this->storage->getDisk($type); $secureUploads = setting('app-secure-images'); - $fileName = $this->cleanImageFileName($imageName); + $fileName = $this->storage->cleanImageFileName($imageName); $imagePath = '/uploads/images/' . $type . '/' . date('Y-m') . '/'; - while ($storage->exists($this->adjustPathForStorageDisk($imagePath . $fileName, $type))) { + while ($disk->exists($this->storage->adjustPathForDisk($imagePath . $fileName, $type))) { $fileName = Str::random(3) . $fileName; } @@ -162,7 +95,7 @@ class ImageService } try { - $this->saveImageDataInPublicSpace($storage, $this->adjustPathForStorageDisk($fullPath, $type), $imageData); + $this->storage->storeInPublicSpace($disk, $this->storage->adjustPathForDisk($fullPath, $type), $imageData); } catch (Exception $e) { Log::error('Error when attempting image upload:' . $e->getMessage()); @@ -172,7 +105,7 @@ class ImageService $imageDetails = [ 'name' => $imageName, 'path' => $fullPath, - 'url' => $this->getPublicUrl($fullPath), + 'url' => $this->storage->getPublicUrl($fullPath), 'type' => $type, 'uploaded_to' => $uploadedTo, ]; @@ -189,50 +122,17 @@ class ImageService return $image; } + /** + * Replace an existing image file in the system using the given file. + */ public function replaceExistingFromUpload(string $path, string $type, UploadedFile $file): void { $imageData = file_get_contents($file->getRealPath()); - $storage = $this->getStorageDisk($type); - $adjustedPath = $this->adjustPathForStorageDisk($path, $type); - $storage->put($adjustedPath, $imageData); + $disk = $this->storage->getDisk($type); + $adjustedPath = $this->storage->adjustPathForDisk($path, $type); + $disk->put($adjustedPath, $imageData); } - /** - * Save image data for the given path in the public space, if possible, - * for the provided storage mechanism. - */ - protected function saveImageDataInPublicSpace(Storage $storage, string $path, string $data): void - { - $storage->put($path, $data); - - // Set visibility when a non-AWS-s3, s3-like storage option is in use. - // Done since this call can break s3-like services but desired for other image stores. - // Attempting to set ACL during above put request requires different permissions - // hence would technically be a breaking change for actual s3 usage. - $usingS3 = strtolower(config('filesystems.images')) === 's3'; - $usingS3Like = $usingS3 && !is_null(config('filesystems.disks.s3.endpoint')); - if (!$usingS3Like) { - $storage->setVisibility($path, 'public'); - } - } - - /** - * Clean up an image file name to be both URL and storage safe. - */ - protected function cleanImageFileName(string $name): string - { - $name = str_replace(' ', '-', $name); - $nameParts = explode('.', $name); - $extension = array_pop($nameParts); - $name = implode('-', $nameParts); - $name = Str::slug($name); - - if (strlen($name) === 0) { - $name = Str::random(10); - } - - return $name . '.' . $extension; - } /** * Checks if the image is a gif. Returns true if it is, else false. @@ -274,7 +174,7 @@ class ImageService ): ?string { // Do not resize GIF images where we're not cropping if ($keepRatio && $this->isGif($image)) { - return $this->getPublicUrl($image->path); + return $this->storage->getPublicUrl($image->path); } $thumbDirName = '/' . ($keepRatio ? 'scaled-' : 'thumbs-') . $width . '-' . $height . '/'; @@ -286,24 +186,24 @@ class ImageService // Return path if in cache $cachedThumbPath = $this->cache->get($thumbCacheKey); if ($cachedThumbPath && !$shouldCreate) { - return $this->getPublicUrl($cachedThumbPath); + return $this->storage->getPublicUrl($cachedThumbPath); } // If thumbnail has already been generated, serve that and cache path - $storage = $this->getStorageDisk($image->type); - if (!$shouldCreate && $storage->exists($this->adjustPathForStorageDisk($thumbFilePath, $image->type))) { + $disk = $this->storage->getDisk($image->type); + if (!$shouldCreate && $disk->exists($this->storage->adjustPathForDisk($thumbFilePath, $image->type))) { $this->cache->put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); - return $this->getPublicUrl($thumbFilePath); + return $this->storage->getPublicUrl($thumbFilePath); } - $imageData = $storage->get($this->adjustPathForStorageDisk($imagePath, $image->type)); + $imageData = $disk->get($this->storage->adjustPathForDisk($imagePath, $image->type)); // Do not resize apng images where we're not cropping if ($keepRatio && $this->isApngData($image, $imageData)) { $this->cache->put($thumbCacheKey, $image->path, 60 * 60 * 72); - return $this->getPublicUrl($image->path); + return $this->storage->getPublicUrl($image->path); } if (!$shouldCreate && !$canCreate) { @@ -312,10 +212,10 @@ class ImageService // If not in cache and thumbnail does not exist, generate thumb and cache path $thumbData = $this->resizeImage($imageData, $width, $height, $keepRatio); - $this->saveImageDataInPublicSpace($storage, $this->adjustPathForStorageDisk($thumbFilePath, $image->type), $thumbData); + $this->storage->storeInPublicSpace($disk, $this->storage->adjustPathForDisk($thumbFilePath, $image->type), $thumbData); $this->cache->put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); - return $this->getPublicUrl($thumbFilePath); + return $this->storage->getPublicUrl($thumbFilePath); } /** @@ -353,59 +253,17 @@ class ImageService return $thumbData; } - /** - * Orientate the given intervention image based upon the given original image data. - * Intervention does have an `orientate` method but the exif data it needs is lost before it - * can be used (At least when created using binary string data) so we need to do some - * implementation on our side to use the original image data. - * Bulk of logic taken from: https://github.com/Intervention/image/blob/b734a4988b2148e7d10364b0609978a88d277536/src/Intervention/Image/Commands/OrientateCommand.php - * Copyright (c) Oliver Vogel, MIT License. - */ - protected function orientImageToOriginalExif(InterventionImage $image, string $originalData): void - { - if (!extension_loaded('exif')) { - return; - } - - $stream = Utils::streamFor($originalData)->detach(); - $exif = @exif_read_data($stream); - $orientation = $exif ? ($exif['Orientation'] ?? null) : null; - - switch ($orientation) { - case 2: - $image->flip(); - break; - case 3: - $image->rotate(180); - break; - case 4: - $image->rotate(180)->flip(); - break; - case 5: - $image->rotate(270)->flip(); - break; - case 6: - $image->rotate(270); - break; - case 7: - $image->rotate(90)->flip(); - break; - case 8: - $image->rotate(90); - break; - } - } /** * Get the raw data content from an image. * - * @throws FileNotFoundException + * @throws Exception */ public function getImageData(Image $image): string { - $storage = $this->getStorageDisk(); + $disk = $this->storage->getDisk(); - return $storage->get($this->adjustPathForStorageDisk($image->path, $image->type)); + return $disk->get($this->storage->adjustPathForDisk($image->path, $image->type)); } /** @@ -425,24 +283,24 @@ class ImageService */ protected function destroyImagesFromPath(string $path, string $imageType): bool { - $path = $this->adjustPathForStorageDisk($path, $imageType); - $storage = $this->getStorageDisk($imageType); + $path = $this->storage->adjustPathForDisk($path, $imageType); + $disk = $this->storage->getDisk($imageType); $imageFolder = dirname($path); $imageFileName = basename($path); - $allImages = collect($storage->allFiles($imageFolder)); + $allImages = collect($disk->allFiles($imageFolder)); // Delete image files $imagesToDelete = $allImages->filter(function ($imagePath) use ($imageFileName) { return basename($imagePath) === $imageFileName; }); - $storage->delete($imagesToDelete->all()); + $disk->delete($imagesToDelete->all()); // Cleanup of empty folders - $foldersInvolved = array_merge([$imageFolder], $storage->directories($imageFolder)); + $foldersInvolved = array_merge([$imageFolder], $disk->directories($imageFolder)); foreach ($foldersInvolved as $directory) { - if ($this->isFolderEmpty($storage, $directory)) { - $storage->deleteDirectory($directory); + if ($this->isFolderEmpty($disk, $directory)) { + $disk->deleteDirectory($directory); } } @@ -452,7 +310,7 @@ class ImageService /** * Check whether a folder is empty. */ - protected function isFolderEmpty(Storage $storage, string $path): bool + protected function isFolderEmpty(StorageDisk $storage, string $path): bool { $files = $storage->files($path); $folders = $storage->directories($path); @@ -506,33 +364,33 @@ class ImageService * * @throws FileNotFoundException */ - public function imageUriToBase64(string $uri): ?string + public function imageUrlToBase64(string $url): ?string { - $storagePath = $this->imageUrlToStoragePath($uri); - if (empty($uri) || is_null($storagePath)) { + $storagePath = $this->storage->urlToPath($url); + if (empty($url) || is_null($storagePath)) { return null; } - $storagePath = $this->adjustPathForStorageDisk($storagePath); + $storagePath = $this->storage->adjustPathForDisk($storagePath); // Apply access control when local_secure_restricted images are active - if ($this->usingSecureRestrictedImages()) { + if ($this->storage->usingSecureRestrictedImages()) { if (!$this->checkUserHasAccessToRelationOfImageAtPath($storagePath)) { return null; } } - $storage = $this->getStorageDisk(); + $disk = $this->storage->getDisk(); $imageData = null; - if ($storage->exists($storagePath)) { - $imageData = $storage->get($storagePath); + if ($disk->exists($storagePath)) { + $imageData = $disk->get($storagePath); } if (is_null($imageData)) { return null; } - $extension = pathinfo($uri, PATHINFO_EXTENSION); + $extension = pathinfo($url, PATHINFO_EXTENSION); if ($extension === 'svg') { $extension = 'svg+xml'; } @@ -547,15 +405,14 @@ class ImageService */ public function pathAccessibleInLocalSecure(string $imagePath): bool { - /** @var FilesystemAdapter $disk */ - $disk = $this->getStorageDisk('gallery'); + $disk = $this->storage->getDisk('gallery'); - if ($this->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImageAtPath($imagePath)) { + if ($this->storage->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImageAtPath($imagePath)) { return false; } // Check local_secure is active - return $this->usingSecureImages() + return $this->storage->usingSecureImages() && $disk instanceof FilesystemAdapter // Check the image file exists && $disk->exists($imagePath) @@ -617,7 +474,7 @@ class ImageService */ public function streamImageFromStorageResponse(string $imageType, string $path): StreamedResponse { - $disk = $this->getStorageDisk($imageType); + $disk = $this->storage->getDisk($imageType); return $disk->response($path); } @@ -631,64 +488,4 @@ class ImageService { return in_array($extension, static::$supportedExtensions); } - - /** - * Get a storage path for the given image URL. - * Ensures the path will start with "uploads/images". - * Returns null if the url cannot be resolved to a local URL. - */ - private function imageUrlToStoragePath(string $url): ?string - { - $url = ltrim(trim($url), '/'); - - // Handle potential relative paths - $isRelative = !str_starts_with($url, 'http'); - if ($isRelative) { - if (str_starts_with(strtolower($url), 'uploads/images')) { - return trim($url, '/'); - } - - return null; - } - - // Handle local images based on paths on the same domain - $potentialHostPaths = [ - url('uploads/images/'), - $this->getPublicUrl('/uploads/images/'), - ]; - - foreach ($potentialHostPaths as $potentialBasePath) { - $potentialBasePath = strtolower($potentialBasePath); - if (str_starts_with(strtolower($url), $potentialBasePath)) { - return 'uploads/images/' . trim(substr($url, strlen($potentialBasePath)), '/'); - } - } - - return null; - } - - /** - * Gets a public facing url for an image by checking relevant environment variables. - * If s3-style store is in use it will default to guessing a public bucket URL. - */ - private function getPublicUrl(string $filePath): string - { - $storageUrl = config('filesystems.url'); - - // Get the standard public s3 url if s3 is set as storage type - // Uses the nice, short URL if bucket name has no periods in otherwise the longer - // region-based url will be used to prevent http issues. - if (!$storageUrl && config('filesystems.images') === 's3') { - $storageDetails = config('filesystems.disks.s3'); - if (!str_contains($storageDetails['bucket'], '.')) { - $storageUrl = 'https://' . $storageDetails['bucket'] . '.s3.amazonaws.com'; - } else { - $storageUrl = 'https://s3-' . $storageDetails['region'] . '.amazonaws.com/' . $storageDetails['bucket']; - } - } - - $basePath = $storageUrl ?: url('/'); - - return rtrim($basePath, '/') . $filePath; - } } diff --git a/app/Uploads/ImageStorage.php b/app/Uploads/ImageStorage.php new file mode 100644 index 000000000..c51450052 --- /dev/null +++ b/app/Uploads/ImageStorage.php @@ -0,0 +1,183 @@ +fileSystem->disk($this->getDiskName($imageType)); + } + + /** + * Check if local secure image storage (Fetched behind authentication) + * is currently active in the instance. + */ + public function usingSecureImages(string $imageType = 'gallery'): bool + { + return $this->getDiskName($imageType) === 'local_secure_images'; + } + + /** + * Check if "local secure restricted" (Fetched behind auth, with permissions enforced) + * is currently active in the instance. + */ + public function usingSecureRestrictedImages() + { + return config('filesystems.images') === 'local_secure_restricted'; + } + + /** + * Change the originally provided path to fit any disk-specific requirements. + * This also ensures the path is kept to the expected root folders. + */ + public function adjustPathForDisk(string $path, string $imageType = ''): string + { + $path = (new WhitespacePathNormalizer())->normalizePath(str_replace('uploads/images/', '', $path)); + + if ($this->usingSecureImages($imageType)) { + return $path; + } + + return 'uploads/images/' . $path; + } + + /** + * Clean up an image file name to be both URL and storage safe. + */ + public function cleanImageFileName(string $name): string + { + $name = str_replace(' ', '-', $name); + $nameParts = explode('.', $name); + $extension = array_pop($nameParts); + $name = implode('-', $nameParts); + $name = Str::slug($name); + + if (strlen($name) === 0) { + $name = Str::random(10); + } + + return $name . '.' . $extension; + } + + /** + * Get the name of the storage disk to use. + */ + protected function getDiskName(string $imageType): string + { + $storageType = config('filesystems.images'); + $localSecureInUse = ($storageType === 'local_secure' || $storageType === 'local_secure_restricted'); + + // Ensure system images (App logo) are uploaded to a public space + if ($imageType === 'system' && $localSecureInUse) { + return 'local'; + } + + // Rename local_secure options to get our image specific storage driver which + // is scoped to the relevant image directories. + if ($localSecureInUse) { + return 'local_secure_images'; + } + + return $storageType; + } + + /** + * Get a storage path for the given image URL. + * Ensures the path will start with "uploads/images". + * Returns null if the url cannot be resolved to a local URL. + */ + public function urlToPath(string $url): ?string + { + $url = ltrim(trim($url), '/'); + + // Handle potential relative paths + $isRelative = !str_starts_with($url, 'http'); + if ($isRelative) { + if (str_starts_with(strtolower($url), 'uploads/images')) { + return trim($url, '/'); + } + + return null; + } + + // Handle local images based on paths on the same domain + $potentialHostPaths = [ + url('uploads/images/'), + $this->getPublicUrl('/uploads/images/'), + ]; + + foreach ($potentialHostPaths as $potentialBasePath) { + $potentialBasePath = strtolower($potentialBasePath); + if (str_starts_with(strtolower($url), $potentialBasePath)) { + return 'uploads/images/' . trim(substr($url, strlen($potentialBasePath)), '/'); + } + } + + return null; + } + + /** + * Gets a public facing url for an image by checking relevant environment variables. + * If s3-style store is in use it will default to guessing a public bucket URL. + */ + public function getPublicUrl(string $filePath): string + { + $storageUrl = config('filesystems.url'); + + // Get the standard public s3 url if s3 is set as storage type + // Uses the nice, short URL if bucket name has no periods in otherwise the longer + // region-based url will be used to prevent http issues. + if (!$storageUrl && config('filesystems.images') === 's3') { + $storageDetails = config('filesystems.disks.s3'); + if (!str_contains($storageDetails['bucket'], '.')) { + $storageUrl = 'https://' . $storageDetails['bucket'] . '.s3.amazonaws.com'; + } else { + $storageUrl = 'https://s3-' . $storageDetails['region'] . '.amazonaws.com/' . $storageDetails['bucket']; + } + } + + $basePath = $storageUrl ?: url('/'); + + return rtrim($basePath, '/') . $filePath; + } + + /** + * Save image data for the given path in the public space, if possible, + * for the provided storage mechanism. + */ + public function storeInPublicSpace(StorageDisk $storage, string $path, string $data): void + { + $storage->put($path, $data); + + // Set visibility when a non-AWS-s3, s3-like storage option is in use. + // Done since this call can break s3-like services but desired for other image stores. + // Attempting to set ACL during above put request requires different permissions + // hence would technically be a breaking change for actual s3 usage. + if (!$this->isS3Like()) { + $storage->setVisibility($path, 'public'); + } + } + + /** + * Check if the image storage in use is an S3-like (but not likely S3) external system. + */ + protected function isS3Like(): bool + { + $usingS3 = strtolower(config('filesystems.images')) === 's3'; + return $usingS3 && !is_null(config('filesystems.disks.s3.endpoint')); + } +} diff --git a/lang/en/errors.php b/lang/en/errors.php index 4164d558b..285817e47 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -51,6 +51,7 @@ return [ 'image_upload_error' => 'An error occurred uploading the image', 'image_upload_type_error' => 'The image type being uploaded is invalid', 'image_upload_replace_type' => 'Image file replacements must be of the same type', + 'image_upload_memory_limit' => 'Failed to handle image upload and/or create thumbnails due to system resource limits', 'image_thumbnail_memory_limit' => 'Failed to create image size variations due to system resource limits', 'drawing_data_not_found' => 'Drawing data could not be loaded. The drawing file might no longer exist or you may not have permission to access it.', From e703009d7fa6d1f3e448c23611ac907277412c42 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 30 Sep 2023 19:12:22 +0100 Subject: [PATCH 59/97] Images: Added thin wrapper around image filesystem instances Extracts duplicated required handling (Like path adjustment) out to simpler storage disk instance which can be passed around. --- app/Uploads/ImageService.php | 75 +++-------------- app/Uploads/ImageStorage.php | 63 ++------------ app/Uploads/ImageStorageDisk.php | 139 +++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 118 deletions(-) create mode 100644 app/Uploads/ImageStorageDisk.php diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index 81d6add92..f8567c3e5 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -9,9 +9,6 @@ use BookStack\Exceptions\ImageUploadException; use ErrorException; use Exception; use Illuminate\Contracts\Cache\Repository as Cache; -use Illuminate\Contracts\Filesystem\FileNotFoundException; -use Illuminate\Contracts\Filesystem\Filesystem as StorageDisk; -use Illuminate\Filesystem\FilesystemAdapter; use Illuminate\Filesystem\FilesystemManager; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; @@ -85,7 +82,7 @@ class ImageService $imagePath = '/uploads/images/' . $type . '/' . date('Y-m') . '/'; - while ($disk->exists($this->storage->adjustPathForDisk($imagePath . $fileName, $type))) { + while ($disk->exists($imagePath . $fileName)) { $fileName = Str::random(3) . $fileName; } @@ -95,7 +92,7 @@ class ImageService } try { - $this->storage->storeInPublicSpace($disk, $this->storage->adjustPathForDisk($fullPath, $type), $imageData); + $disk->put($fullPath, $imageData, true); } catch (Exception $e) { Log::error('Error when attempting image upload:' . $e->getMessage()); @@ -129,11 +126,9 @@ class ImageService { $imageData = file_get_contents($file->getRealPath()); $disk = $this->storage->getDisk($type); - $adjustedPath = $this->storage->adjustPathForDisk($path, $type); - $disk->put($adjustedPath, $imageData); + $disk->put($path, $imageData); } - /** * Checks if the image is a gif. Returns true if it is, else false. */ @@ -191,13 +186,13 @@ class ImageService // If thumbnail has already been generated, serve that and cache path $disk = $this->storage->getDisk($image->type); - if (!$shouldCreate && $disk->exists($this->storage->adjustPathForDisk($thumbFilePath, $image->type))) { + if (!$shouldCreate && $disk->exists($thumbFilePath)) { $this->cache->put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); return $this->storage->getPublicUrl($thumbFilePath); } - $imageData = $disk->get($this->storage->adjustPathForDisk($imagePath, $image->type)); + $imageData = $disk->get($imagePath); // Do not resize apng images where we're not cropping if ($keepRatio && $this->isApngData($image, $imageData)) { @@ -212,7 +207,7 @@ class ImageService // If not in cache and thumbnail does not exist, generate thumb and cache path $thumbData = $this->resizeImage($imageData, $width, $height, $keepRatio); - $this->storage->storeInPublicSpace($disk, $this->storage->adjustPathForDisk($thumbFilePath, $image->type), $thumbData); + $disk->put($thumbFilePath, $thumbData, true); $this->cache->put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); return $this->storage->getPublicUrl($thumbFilePath); @@ -253,7 +248,6 @@ class ImageService return $thumbData; } - /** * Get the raw data content from an image. * @@ -263,7 +257,7 @@ class ImageService { $disk = $this->storage->getDisk(); - return $disk->get($this->storage->adjustPathForDisk($image->path, $image->type)); + return $disk->get($image->path); } /** @@ -271,53 +265,13 @@ class ImageService * * @throws Exception */ - public function destroy(Image $image) + public function destroy(Image $image): void { - $this->destroyImagesFromPath($image->path, $image->type); + $disk = $this->storage->getDisk($image->type); + $disk->destroyAllMatchingNameFromPath($image->path); $image->delete(); } - /** - * Destroys an image at the given path. - * Searches for image thumbnails in addition to main provided path. - */ - protected function destroyImagesFromPath(string $path, string $imageType): bool - { - $path = $this->storage->adjustPathForDisk($path, $imageType); - $disk = $this->storage->getDisk($imageType); - - $imageFolder = dirname($path); - $imageFileName = basename($path); - $allImages = collect($disk->allFiles($imageFolder)); - - // Delete image files - $imagesToDelete = $allImages->filter(function ($imagePath) use ($imageFileName) { - return basename($imagePath) === $imageFileName; - }); - $disk->delete($imagesToDelete->all()); - - // Cleanup of empty folders - $foldersInvolved = array_merge([$imageFolder], $disk->directories($imageFolder)); - foreach ($foldersInvolved as $directory) { - if ($this->isFolderEmpty($disk, $directory)) { - $disk->deleteDirectory($directory); - } - } - - return true; - } - - /** - * Check whether a folder is empty. - */ - protected function isFolderEmpty(StorageDisk $storage, string $path): bool - { - $files = $storage->files($path); - $folders = $storage->directories($path); - - return count($files) === 0 && count($folders) === 0; - } - /** * Delete gallery and drawings that are not within HTML content of pages or page revisions. * Checks based off of only the image name. @@ -325,7 +279,7 @@ class ImageService * * Returns the path of the images that would be/have been deleted. */ - public function deleteUnusedImages(bool $checkRevisions = true, bool $dryRun = true) + public function deleteUnusedImages(bool $checkRevisions = true, bool $dryRun = true): array { $types = ['gallery', 'drawio']; $deletedPaths = []; @@ -361,8 +315,6 @@ class ImageService * Attempts to convert the URL to a system storage url then * fetch the data from the disk or storage location. * Returns null if the image data cannot be fetched from storage. - * - * @throws FileNotFoundException */ public function imageUrlToBase64(string $url): ?string { @@ -371,8 +323,6 @@ class ImageService return null; } - $storagePath = $this->storage->adjustPathForDisk($storagePath); - // Apply access control when local_secure_restricted images are active if ($this->storage->usingSecureRestrictedImages()) { if (!$this->checkUserHasAccessToRelationOfImageAtPath($storagePath)) { @@ -412,8 +362,7 @@ class ImageService } // Check local_secure is active - return $this->storage->usingSecureImages() - && $disk instanceof FilesystemAdapter + return $disk->usingSecureImages() // Check the image file exists && $disk->exists($imagePath) // Check the file is likely an image file diff --git a/app/Uploads/ImageStorage.php b/app/Uploads/ImageStorage.php index c51450052..dc4abc0f2 100644 --- a/app/Uploads/ImageStorage.php +++ b/app/Uploads/ImageStorage.php @@ -2,10 +2,8 @@ namespace BookStack\Uploads; -use Illuminate\Contracts\Filesystem\Filesystem as StorageDisk; use Illuminate\Filesystem\FilesystemManager; use Illuminate\Support\Str; -use League\Flysystem\WhitespacePathNormalizer; class ImageStorage { @@ -17,44 +15,25 @@ class ImageStorage /** * Get the storage disk for the given image type. */ - public function getDisk(string $imageType = ''): StorageDisk + public function getDisk(string $imageType = ''): ImageStorageDisk { - return $this->fileSystem->disk($this->getDiskName($imageType)); - } + $diskName = $this->getDiskName($imageType); - /** - * Check if local secure image storage (Fetched behind authentication) - * is currently active in the instance. - */ - public function usingSecureImages(string $imageType = 'gallery'): bool - { - return $this->getDiskName($imageType) === 'local_secure_images'; + return new ImageStorageDisk( + $diskName, + $this->fileSystem->disk($diskName), + ); } /** * Check if "local secure restricted" (Fetched behind auth, with permissions enforced) * is currently active in the instance. */ - public function usingSecureRestrictedImages() + public function usingSecureRestrictedImages(): bool { return config('filesystems.images') === 'local_secure_restricted'; } - /** - * Change the originally provided path to fit any disk-specific requirements. - * This also ensures the path is kept to the expected root folders. - */ - public function adjustPathForDisk(string $path, string $imageType = ''): string - { - $path = (new WhitespacePathNormalizer())->normalizePath(str_replace('uploads/images/', '', $path)); - - if ($this->usingSecureImages($imageType)) { - return $path; - } - - return 'uploads/images/' . $path; - } - /** * Clean up an image file name to be both URL and storage safe. */ @@ -78,7 +57,7 @@ class ImageStorage */ protected function getDiskName(string $imageType): string { - $storageType = config('filesystems.images'); + $storageType = strtolower(config('filesystems.images')); $localSecureInUse = ($storageType === 'local_secure' || $storageType === 'local_secure_restricted'); // Ensure system images (App logo) are uploaded to a public space @@ -154,30 +133,4 @@ class ImageStorage return rtrim($basePath, '/') . $filePath; } - - /** - * Save image data for the given path in the public space, if possible, - * for the provided storage mechanism. - */ - public function storeInPublicSpace(StorageDisk $storage, string $path, string $data): void - { - $storage->put($path, $data); - - // Set visibility when a non-AWS-s3, s3-like storage option is in use. - // Done since this call can break s3-like services but desired for other image stores. - // Attempting to set ACL during above put request requires different permissions - // hence would technically be a breaking change for actual s3 usage. - if (!$this->isS3Like()) { - $storage->setVisibility($path, 'public'); - } - } - - /** - * Check if the image storage in use is an S3-like (but not likely S3) external system. - */ - protected function isS3Like(): bool - { - $usingS3 = strtolower(config('filesystems.images')) === 's3'; - return $usingS3 && !is_null(config('filesystems.disks.s3.endpoint')); - } } diff --git a/app/Uploads/ImageStorageDisk.php b/app/Uploads/ImageStorageDisk.php new file mode 100644 index 000000000..3a95661ca --- /dev/null +++ b/app/Uploads/ImageStorageDisk.php @@ -0,0 +1,139 @@ +diskName === 'local_secure_images'; + } + + /** + * Change the originally provided path to fit any disk-specific requirements. + * This also ensures the path is kept to the expected root folders. + */ + protected function adjustPathForDisk(string $path): string + { + $path = (new WhitespacePathNormalizer())->normalizePath(str_replace('uploads/images/', '', $path)); + + if ($this->usingSecureImages()) { + return $path; + } + + return 'uploads/images/' . $path; + } + + /** + * Check if a file at the given path exists. + */ + public function exists(string $path): bool + { + return $this->filesystem->exists($this->adjustPathForDisk($path)); + } + + /** + * Get the file at the given path. + */ + public function get(string $path): bool + { + return $this->filesystem->get($this->adjustPathForDisk($path)); + } + + /** + * Save the given image data at the given path. Can choose to set + * the image as public which will update its visibility after saving. + */ + public function put(string $path, string $data, bool $makePublic = false): void + { + $path = $this->adjustPathForDisk($path); + $this->filesystem->put($path, $data); + + // Set visibility when a non-AWS-s3, s3-like storage option is in use. + // Done since this call can break s3-like services but desired for other image stores. + // Attempting to set ACL during above put request requires different permissions + // hence would technically be a breaking change for actual s3 usage. + if ($makePublic && !$this->isS3Like()) { + $this->filesystem->setVisibility($path, 'public'); + } + } + + /** + * Destroys an image at the given path. + * Searches for image thumbnails in addition to main provided path. + */ + public function destroyAllMatchingNameFromPath(string $path): void + { + $path = $this->adjustPathForDisk($path); + + $imageFolder = dirname($path); + $imageFileName = basename($path); + $allImages = collect($this->filesystem->allFiles($imageFolder)); + + // Delete image files + $imagesToDelete = $allImages->filter(function ($imagePath) use ($imageFileName) { + return basename($imagePath) === $imageFileName; + }); + $this->filesystem->delete($imagesToDelete->all()); + + // Cleanup of empty folders + $foldersInvolved = array_merge([$imageFolder], $this->filesystem->directories($imageFolder)); + foreach ($foldersInvolved as $directory) { + if ($this->isFolderEmpty($directory)) { + $this->filesystem->deleteDirectory($directory); + } + } + } + + /** + * Get the mime type of the file at the given path. + * Only works for local filesystem adapters. + */ + public function mimeType(string $path): string + { + return $this->filesystem instanceof FilesystemAdapter ? $this->filesystem->mimeType($path) : ''; + } + + /** + * Get a stream response for the image at the given path. + */ + public function response(string $path): StreamedResponse + { + return $this->filesystem->response($path); + } + + /** + * Check if the image storage in use is an S3-like (but not likely S3) external system. + */ + protected function isS3Like(): bool + { + $usingS3 = $this->diskName === 's3'; + return $usingS3 && !is_null(config('filesystems.disks.s3.endpoint')); + } + + /** + * Check whether a folder is empty. + */ + protected function isFolderEmpty(string $path): bool + { + $files = $this->filesystem->files($path); + $folders = $this->filesystem->directories($path); + + return count($files) === 0 && count($folders) === 0; + } +} From 20bcbd76efdffa7537c7373197db4491ea846ab0 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 30 Sep 2023 20:00:48 +0100 Subject: [PATCH 60/97] Images: Extracted out image resizing to its own class --- app/Uploads/Image.php | 2 +- app/Uploads/ImageRepo.php | 9 +-- app/Uploads/ImageResizer.php | 94 +++++++++++++++++++++- app/Uploads/ImageService.php | 132 +------------------------------ app/Uploads/ImageStorageDisk.php | 5 +- tests/Uploads/ImageTest.php | 1 + 6 files changed, 102 insertions(+), 141 deletions(-) diff --git a/app/Uploads/Image.php b/app/Uploads/Image.php index 5e197e750..1e42f414b 100644 --- a/app/Uploads/Image.php +++ b/app/Uploads/Image.php @@ -52,7 +52,7 @@ class Image extends Model */ public function getThumb(?int $width, ?int $height, bool $keepRatio = false): ?string { - return app()->make(ImageService::class)->getThumbnail($this, $width, $height, $keepRatio, false, true); + return app()->make(ImageResizer::class)->resizeToThumbnailUrl($this, $width, $height, $keepRatio, false, true); } /** diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index 8a770da78..4aa36bab9 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -13,7 +13,8 @@ class ImageRepo { public function __construct( protected ImageService $imageService, - protected PermissionApplicator $permissions + protected PermissionApplicator $permissions, + protected ImageResizer $imageResizer, ) { } @@ -225,14 +226,12 @@ class ImageRepo } /** - * Get the thumbnail for an image. - * If $keepRatio is true only the width will be used. - * Checks the cache then storage to avoid creating / accessing the filesystem on every check. + * Get a thumbnail URL for the given image. */ protected function getThumbnail(Image $image, ?int $width, ?int $height, bool $keepRatio, bool $shouldCreate): ?string { try { - return $this->imageService->getThumbnail($image, $width, $height, $keepRatio, $shouldCreate); + return $this->imageResizer->resizeToThumbnailUrl($image, $width, $height, $keepRatio, $shouldCreate); } catch (Exception $exception) { return null; } diff --git a/app/Uploads/ImageResizer.php b/app/Uploads/ImageResizer.php index 7a89b9d35..5fe8a8954 100644 --- a/app/Uploads/ImageResizer.php +++ b/app/Uploads/ImageResizer.php @@ -3,28 +3,91 @@ namespace BookStack\Uploads; use BookStack\Exceptions\ImageUploadException; +use Exception; use GuzzleHttp\Psr7\Utils; -use Intervention\Image\Exception\NotSupportedException; +use Illuminate\Support\Facades\Cache; use Intervention\Image\Image as InterventionImage; use Intervention\Image\ImageManager; class ImageResizer { public function __construct( - protected ImageManager $intervention + protected ImageManager $intervention, + protected ImageStorage $storage, ) { } + /** + * Get the thumbnail for an image. + * If $keepRatio is true only the width will be used. + * Checks the cache then storage to avoid creating / accessing the filesystem on every check. + * + * @throws Exception + */ + public function resizeToThumbnailUrl( + Image $image, + ?int $width, + ?int $height, + bool $keepRatio = false, + bool $shouldCreate = false, + bool $canCreate = false, + ): ?string { + // Do not resize GIF images where we're not cropping + if ($keepRatio && $this->isGif($image)) { + return $this->storage->getPublicUrl($image->path); + } + + $thumbDirName = '/' . ($keepRatio ? 'scaled-' : 'thumbs-') . $width . '-' . $height . '/'; + $imagePath = $image->path; + $thumbFilePath = dirname($imagePath) . $thumbDirName . basename($imagePath); + + $thumbCacheKey = 'images::' . $image->id . '::' . $thumbFilePath; + + // Return path if in cache + $cachedThumbPath = Cache::get($thumbCacheKey); + if ($cachedThumbPath && !$shouldCreate) { + return $this->storage->getPublicUrl($cachedThumbPath); + } + + // If thumbnail has already been generated, serve that and cache path + $disk = $this->storage->getDisk($image->type); + if (!$shouldCreate && $disk->exists($thumbFilePath)) { + Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); + + return $this->storage->getPublicUrl($thumbFilePath); + } + + $imageData = $disk->get($imagePath); + + // Do not resize apng images where we're not cropping + if ($keepRatio && $this->isApngData($image, $imageData)) { + Cache::put($thumbCacheKey, $image->path, 60 * 60 * 72); + + return $this->storage->getPublicUrl($image->path); + } + + if (!$shouldCreate && !$canCreate) { + return null; + } + + // If not in cache and thumbnail does not exist, generate thumb and cache path + $thumbData = $this->resizeImageData($imageData, $width, $height, $keepRatio); + $disk->put($thumbFilePath, $thumbData, true); + Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); + + return $this->storage->getPublicUrl($thumbFilePath); + } + /** * Resize the image of given data to the specified size, and return the new image data. * * @throws ImageUploadException */ - protected function resizeImageData(string $imageData, ?int $width, ?int $height, bool $keepRatio): string + public function resizeImageData(string $imageData, ?int $width, ?int $height, bool $keepRatio): string { try { $thumb = $this->intervention->make($imageData); - } catch (NotSupportedException $e) { + } catch (Exception $e) { throw new ImageUploadException(trans('errors.cannot_create_thumbs')); } @@ -92,4 +155,27 @@ class ImageResizer break; } } + + /** + * Checks if the image is a gif. Returns true if it is, else false. + */ + protected function isGif(Image $image): bool + { + return strtolower(pathinfo($image->path, PATHINFO_EXTENSION)) === 'gif'; + } + + /** + * Check if the given image and image data is apng. + */ + protected function isApngData(Image $image, string &$imageData): bool + { + $isPng = strtolower(pathinfo($image->path, PATHINFO_EXTENSION)) === 'png'; + if (!$isPng) { + return false; + } + + $initialHeader = substr($imageData, 0, strpos($imageData, 'IDAT')); + + return str_contains($initialHeader, 'acTL'); + } } diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index f8567c3e5..1655a4cc3 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -6,15 +6,10 @@ use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; use BookStack\Entities\Models\Page; use BookStack\Exceptions\ImageUploadException; -use ErrorException; use Exception; -use Illuminate\Contracts\Cache\Repository as Cache; -use Illuminate\Filesystem\FilesystemManager; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; -use Intervention\Image\Exception\NotSupportedException; -use Intervention\Image\ImageManager; use Symfony\Component\HttpFoundation\File\UploadedFile; use Symfony\Component\HttpFoundation\StreamedResponse; @@ -23,10 +18,8 @@ class ImageService protected static array $supportedExtensions = ['jpg', 'jpeg', 'png', 'gif', 'webp']; public function __construct( - protected ImageManager $imageTool, - protected FilesystemManager $fileSystem, - protected Cache $cache, protected ImageStorage $storage, + protected ImageResizer $resizer, ) { } @@ -47,7 +40,7 @@ class ImageService $imageData = file_get_contents($uploadedFile->getRealPath()); if ($resizeWidth !== null || $resizeHeight !== null) { - $imageData = $this->resizeImage($imageData, $resizeWidth, $resizeHeight, $keepRatio); + $imageData = $this->resizer->resizeImageData($imageData, $resizeWidth, $resizeHeight, $keepRatio); } return $this->saveNew($imageName, $imageData, $type, $uploadedTo); @@ -129,125 +122,6 @@ class ImageService $disk->put($path, $imageData); } - /** - * Checks if the image is a gif. Returns true if it is, else false. - */ - protected function isGif(Image $image): bool - { - return strtolower(pathinfo($image->path, PATHINFO_EXTENSION)) === 'gif'; - } - - /** - * Check if the given image and image data is apng. - */ - protected function isApngData(Image $image, string &$imageData): bool - { - $isPng = strtolower(pathinfo($image->path, PATHINFO_EXTENSION)) === 'png'; - if (!$isPng) { - return false; - } - - $initialHeader = substr($imageData, 0, strpos($imageData, 'IDAT')); - - return str_contains($initialHeader, 'acTL'); - } - - /** - * Get the thumbnail for an image. - * If $keepRatio is true only the width will be used. - * Checks the cache then storage to avoid creating / accessing the filesystem on every check. - * - * @throws Exception - */ - public function getThumbnail( - Image $image, - ?int $width, - ?int $height, - bool $keepRatio = false, - bool $shouldCreate = false, - bool $canCreate = false, - ): ?string { - // Do not resize GIF images where we're not cropping - if ($keepRatio && $this->isGif($image)) { - return $this->storage->getPublicUrl($image->path); - } - - $thumbDirName = '/' . ($keepRatio ? 'scaled-' : 'thumbs-') . $width . '-' . $height . '/'; - $imagePath = $image->path; - $thumbFilePath = dirname($imagePath) . $thumbDirName . basename($imagePath); - - $thumbCacheKey = 'images::' . $image->id . '::' . $thumbFilePath; - - // Return path if in cache - $cachedThumbPath = $this->cache->get($thumbCacheKey); - if ($cachedThumbPath && !$shouldCreate) { - return $this->storage->getPublicUrl($cachedThumbPath); - } - - // If thumbnail has already been generated, serve that and cache path - $disk = $this->storage->getDisk($image->type); - if (!$shouldCreate && $disk->exists($thumbFilePath)) { - $this->cache->put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); - - return $this->storage->getPublicUrl($thumbFilePath); - } - - $imageData = $disk->get($imagePath); - - // Do not resize apng images where we're not cropping - if ($keepRatio && $this->isApngData($image, $imageData)) { - $this->cache->put($thumbCacheKey, $image->path, 60 * 60 * 72); - - return $this->storage->getPublicUrl($image->path); - } - - if (!$shouldCreate && !$canCreate) { - return null; - } - - // If not in cache and thumbnail does not exist, generate thumb and cache path - $thumbData = $this->resizeImage($imageData, $width, $height, $keepRatio); - $disk->put($thumbFilePath, $thumbData, true); - $this->cache->put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); - - return $this->storage->getPublicUrl($thumbFilePath); - } - - /** - * Resize the image of given data to the specified size, and return the new image data. - * - * @throws ImageUploadException - */ - protected function resizeImage(string $imageData, ?int $width, ?int $height, bool $keepRatio): string - { - try { - $thumb = $this->imageTool->make($imageData); - } catch (ErrorException | NotSupportedException $e) { - throw new ImageUploadException(trans('errors.cannot_create_thumbs')); - } - - $this->orientImageToOriginalExif($thumb, $imageData); - - if ($keepRatio) { - $thumb->resize($width, $height, function ($constraint) { - $constraint->aspectRatio(); - $constraint->upsize(); - }); - } else { - $thumb->fit($width, $height); - } - - $thumbData = (string) $thumb->encode(); - - // Use original image data if we're keeping the ratio - // and the resizing does not save any space. - if ($keepRatio && strlen($thumbData) > strlen($imageData)) { - return $imageData; - } - - return $thumbData; - } - /** * Get the raw data content from an image. * @@ -375,7 +249,7 @@ class ImageService */ protected function checkUserHasAccessToRelationOfImageAtPath(string $path): bool { - if (str_starts_with($path, '/uploads/images/')) { + if (str_starts_with($path, 'uploads/images/')) { $path = substr($path, 15); } diff --git a/app/Uploads/ImageStorageDisk.php b/app/Uploads/ImageStorageDisk.php index 3a95661ca..798b72abd 100644 --- a/app/Uploads/ImageStorageDisk.php +++ b/app/Uploads/ImageStorageDisk.php @@ -50,7 +50,7 @@ class ImageStorageDisk /** * Get the file at the given path. */ - public function get(string $path): bool + public function get(string $path): ?string { return $this->filesystem->get($this->adjustPathForDisk($path)); } @@ -106,6 +106,7 @@ class ImageStorageDisk */ public function mimeType(string $path): string { + $path = $this->adjustPathForDisk($path); return $this->filesystem instanceof FilesystemAdapter ? $this->filesystem->mimeType($path) : ''; } @@ -114,7 +115,7 @@ class ImageStorageDisk */ public function response(string $path): StreamedResponse { - return $this->filesystem->response($path); + return $this->filesystem->response($this->adjustPathForDisk($path)); } /** diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index 9943302d3..4da964d48 100644 --- a/tests/Uploads/ImageTest.php +++ b/tests/Uploads/ImageTest.php @@ -557,6 +557,7 @@ class ImageTest extends TestCase $this->asEditor(); $imageName = 'first-image.png'; $relPath = $this->files->expectedImagePath('gallery', $imageName); + $this->files->deleteAtRelativePath($relPath); $this->files->uploadGalleryImage($this, $imageName, $this->entities->page()->id); $image = Image::first(); From b2d48d9a7f52ae0d37567eec57469ea2d9c901d3 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 1 Oct 2023 13:05:18 +0100 Subject: [PATCH 61/97] Images: Rolled out image memory handling to image actions - Moved thumnbail loading out of repo into ImageResizer. - Updated gallery and editor image handling to show errors where possible to indicate memory issues for resizing/thumbs. - Updated gallery to load image data in a per-image basis via edit form for more resiliant thumb/data fetching. Data was previously provided via gallery listing, which could be affected by failing generation of other images. - Updated image manager double click handling to be more pleasant and not flash away the edit form. - Updated editor handlers to use main URL when thumbs fail to load. --- .../Controllers/DrawioImageController.php | 19 ++++++-- .../Controllers/GalleryImageController.php | 18 ++++++-- app/Uploads/Controllers/ImageController.php | 26 ++++++++--- .../Controllers/ImageGalleryApiController.php | 7 ++- app/Uploads/Image.php | 2 +- app/Uploads/ImageRepo.php | 43 +++---------------- app/Uploads/ImageResizer.php | 43 +++++++++++++++---- lang/en/errors.php | 5 ++- resources/js/components/image-manager.js | 20 +++++---- resources/js/markdown/actions.js | 4 +- resources/js/wysiwyg/drop-paste-handling.js | 2 +- resources/js/wysiwyg/plugins-imagemanager.js | 2 +- resources/sass/_components.scss | 12 ++++++ .../pages/parts/image-manager-form.blade.php | 9 ++++ .../pages/parts/image-manager-list.blade.php | 8 +++- 15 files changed, 142 insertions(+), 78 deletions(-) diff --git a/app/Uploads/Controllers/DrawioImageController.php b/app/Uploads/Controllers/DrawioImageController.php index 49f0c1655..6293da4f7 100644 --- a/app/Uploads/Controllers/DrawioImageController.php +++ b/app/Uploads/Controllers/DrawioImageController.php @@ -5,6 +5,8 @@ namespace BookStack\Uploads\Controllers; use BookStack\Exceptions\ImageUploadException; use BookStack\Http\Controller; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; +use BookStack\Util\OutOfMemoryHandler; use Exception; use Illuminate\Http\Request; @@ -19,7 +21,7 @@ class DrawioImageController extends Controller * Get a list of gallery images, in a list. * Can be paged and filtered by entity. */ - public function list(Request $request) + public function list(Request $request, ImageResizer $resizer) { $page = $request->get('page', 1); $searchTerm = $request->get('search', null); @@ -27,11 +29,20 @@ class DrawioImageController extends Controller $parentTypeFilter = $request->get('filter_type', null); $imgData = $this->imageRepo->getEntityFiltered('drawio', $parentTypeFilter, $page, 24, $uploadedToFilter, $searchTerm); - - return view('pages.parts.image-manager-list', [ + $viewData = [ + 'warning' => '', 'images' => $imgData['images'], 'hasMore' => $imgData['has_more'], - ]); + ]; + + new OutOfMemoryHandler(function () use ($viewData) { + $viewData['warning'] = trans('errors.image_gallery_thumbnail_memory_limit'); + return response()->view('pages.parts.image-manager-list', $viewData, 200); + }); + + $resizer->loadGalleryThumbnailsForMany($imgData['images']); + + return view('pages.parts.image-manager-list', $viewData); } /** diff --git a/app/Uploads/Controllers/GalleryImageController.php b/app/Uploads/Controllers/GalleryImageController.php index 0696ca62b..258f2bef6 100644 --- a/app/Uploads/Controllers/GalleryImageController.php +++ b/app/Uploads/Controllers/GalleryImageController.php @@ -5,6 +5,7 @@ namespace BookStack\Uploads\Controllers; use BookStack\Exceptions\ImageUploadException; use BookStack\Http\Controller; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; use BookStack\Util\OutOfMemoryHandler; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; @@ -22,7 +23,7 @@ class GalleryImageController extends Controller * Get a list of gallery images, in a list. * Can be paged and filtered by entity. */ - public function list(Request $request) + public function list(Request $request, ImageResizer $resizer) { $page = $request->get('page', 1); $searchTerm = $request->get('search', null); @@ -30,11 +31,20 @@ class GalleryImageController extends Controller $parentTypeFilter = $request->get('filter_type', null); $imgData = $this->imageRepo->getEntityFiltered('gallery', $parentTypeFilter, $page, 30, $uploadedToFilter, $searchTerm); - - return view('pages.parts.image-manager-list', [ + $viewData = [ + 'warning' => '', 'images' => $imgData['images'], 'hasMore' => $imgData['has_more'], - ]); + ]; + + new OutOfMemoryHandler(function () use ($viewData) { + $viewData['warning'] = trans('errors.image_gallery_thumbnail_memory_limit'); + return response()->view('pages.parts.image-manager-list', $viewData, 200); + }); + + $resizer->loadGalleryThumbnailsForMany($imgData['images']); + + return view('pages.parts.image-manager-list', $viewData); } /** diff --git a/app/Uploads/Controllers/ImageController.php b/app/Uploads/Controllers/ImageController.php index f92338bc8..c68ffdf6b 100644 --- a/app/Uploads/Controllers/ImageController.php +++ b/app/Uploads/Controllers/ImageController.php @@ -4,9 +4,11 @@ namespace BookStack\Uploads\Controllers; use BookStack\Exceptions\ImageUploadException; use BookStack\Exceptions\NotFoundException; +use BookStack\Exceptions\NotifyException; use BookStack\Http\Controller; use BookStack\Uploads\Image; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; use BookStack\Uploads\ImageService; use BookStack\Util\OutOfMemoryHandler; use Exception; @@ -16,7 +18,8 @@ class ImageController extends Controller { public function __construct( protected ImageRepo $imageRepo, - protected ImageService $imageService + protected ImageService $imageService, + protected ImageResizer $imageResizer, ) { } @@ -98,12 +101,20 @@ class ImageController extends Controller $dependantPages = $this->imageRepo->getPagesUsingImage($image); } - $this->imageRepo->loadThumbs($image, false); - - return view('pages.parts.image-manager-form', [ + $viewData = [ 'image' => $image, 'dependantPages' => $dependantPages ?? null, - ]); + 'warning' => '', + ]; + + new OutOfMemoryHandler(function () use ($viewData) { + $viewData['warning'] = trans('errors.image_thumbnail_memory_limit'); + return response()->view('pages.parts.image-manager-form', $viewData); + }); + + $this->imageResizer->loadGalleryThumbnailsForImage($image, false); + + return view('pages.parts.image-manager-form', $viewData); } /** @@ -135,15 +146,16 @@ class ImageController extends Controller return $this->jsonError(trans('errors.image_thumbnail_memory_limit')); }); - $this->imageRepo->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); return response(trans('components.image_rebuild_thumbs_success')); } /** * Check related page permission and ensure type is drawio or gallery. + * @throws NotifyException */ - protected function checkImagePermission(Image $image) + protected function checkImagePermission(Image $image): void { if ($image->type !== 'drawio' && $image->type !== 'gallery') { $this->showPermissionError(); diff --git a/app/Uploads/Controllers/ImageGalleryApiController.php b/app/Uploads/Controllers/ImageGalleryApiController.php index efdff5be4..ec96e4593 100644 --- a/app/Uploads/Controllers/ImageGalleryApiController.php +++ b/app/Uploads/Controllers/ImageGalleryApiController.php @@ -6,6 +6,7 @@ use BookStack\Entities\Models\Page; use BookStack\Http\ApiController; use BookStack\Uploads\Image; use BookStack\Uploads\ImageRepo; +use BookStack\Uploads\ImageResizer; use Illuminate\Http\Request; class ImageGalleryApiController extends ApiController @@ -15,7 +16,8 @@ class ImageGalleryApiController extends ApiController ]; public function __construct( - protected ImageRepo $imageRepo + protected ImageRepo $imageRepo, + protected ImageResizer $imageResizer, ) { } @@ -130,7 +132,7 @@ class ImageGalleryApiController extends ApiController */ protected function formatForSingleResponse(Image $image): array { - $this->imageRepo->loadThumbs($image, false); + $this->imageResizer->loadGalleryThumbnailsForImage($image, false); $data = $image->toArray(); $data['created_by'] = $image->createdBy; $data['updated_by'] = $image->updatedBy; @@ -138,6 +140,7 @@ class ImageGalleryApiController extends ApiController $escapedUrl = htmlentities($image->url); $escapedName = htmlentities($image->name); + if ($image->type === 'drawio') { $data['content']['html'] = "
    id}\">
    "; $data['content']['markdown'] = $data['content']['html']; diff --git a/app/Uploads/Image.php b/app/Uploads/Image.php index 1e42f414b..0a267a644 100644 --- a/app/Uploads/Image.php +++ b/app/Uploads/Image.php @@ -52,7 +52,7 @@ class Image extends Model */ public function getThumb(?int $width, ?int $height, bool $keepRatio = false): ?string { - return app()->make(ImageResizer::class)->resizeToThumbnailUrl($this, $width, $height, $keepRatio, false, true); + return app()->make(ImageResizer::class)->resizeToThumbnailUrl($this, $width, $height, $keepRatio, false); } /** diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index 4aa36bab9..0e312d883 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -30,19 +30,13 @@ class ImageRepo * Execute a paginated query, returning in a standard format. * Also runs the query through the restriction system. */ - private function returnPaginated(Builder $query, int $page = 1, int $pageSize = 24): array + protected function returnPaginated(Builder $query, int $page = 1, int $pageSize = 24): array { $images = $query->orderBy('created_at', 'desc')->skip($pageSize * ($page - 1))->take($pageSize + 1)->get(); - $hasMore = count($images) > $pageSize; - - $returnImages = $images->take($pageSize); - $returnImages->each(function (Image $image) { - $this->loadThumbs($image, false); - }); return [ - 'images' => $returnImages, - 'has_more' => $hasMore, + 'images' => $images->take($pageSize), + 'has_more' => count($images) > $pageSize, ]; } @@ -120,7 +114,7 @@ class ImageRepo $image = $this->imageService->saveNewFromUpload($uploadFile, $type, $uploadedTo, $resizeWidth, $resizeHeight, $keepRatio); if ($type !== 'system') { - $this->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); } return $image; @@ -134,7 +128,7 @@ class ImageRepo public function saveNewFromData(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image { $image = $this->imageService->saveNew($imageName, $imageData, $type, $uploadedTo); - $this->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); return $image; } @@ -161,7 +155,7 @@ class ImageRepo $image->fill($updateDetails); $image->updated_by = user()->id; $image->save(); - $this->loadThumbs($image, false); + $this->imageResizer->loadGalleryThumbnailsForImage($image, false); return $image; } @@ -182,7 +176,7 @@ class ImageRepo $image->save(); $this->imageService->replaceExistingFromUpload($image->path, $image->type, $file); - $this->loadThumbs($image, true); + $this->imageResizer->loadGalleryThumbnailsForImage($image, true); } /** @@ -214,29 +208,6 @@ class ImageRepo } } - /** - * Load thumbnails onto an image object. - */ - public function loadThumbs(Image $image, bool $shouldCreate): void - { - $image->setAttribute('thumbs', [ - 'gallery' => $this->getThumbnail($image, 150, 150, false, $shouldCreate), - 'display' => $this->getThumbnail($image, 1680, null, true, $shouldCreate), - ]); - } - - /** - * Get a thumbnail URL for the given image. - */ - protected function getThumbnail(Image $image, ?int $width, ?int $height, bool $keepRatio, bool $shouldCreate): ?string - { - try { - return $this->imageResizer->resizeToThumbnailUrl($image, $width, $height, $keepRatio, $shouldCreate); - } catch (Exception $exception) { - return null; - } - } - /** * Get the raw image data from an Image. */ diff --git a/app/Uploads/ImageResizer.php b/app/Uploads/ImageResizer.php index 5fe8a8954..0d090a94b 100644 --- a/app/Uploads/ImageResizer.php +++ b/app/Uploads/ImageResizer.php @@ -11,12 +11,42 @@ use Intervention\Image\ImageManager; class ImageResizer { + protected const THUMBNAIL_CACHE_TIME = 604_800; // 1 week + public function __construct( protected ImageManager $intervention, protected ImageStorage $storage, ) { } + /** + * Load gallery thumbnails for a set of images. + * @param iterable $images + */ + public function loadGalleryThumbnailsForMany(iterable $images, bool $shouldCreate = false): void + { + foreach ($images as $image) { + $this->loadGalleryThumbnailsForImage($image, $shouldCreate); + } + } + + /** + * Load gallery thumbnails into the given image instance. + */ + public function loadGalleryThumbnailsForImage(Image $image, bool $shouldCreate): void + { + $thumbs = ['gallery' => null, 'display' => null]; + + try { + $thumbs['gallery'] = $this->resizeToThumbnailUrl($image, 150, 150, false, $shouldCreate); + $thumbs['display'] = $this->resizeToThumbnailUrl($image, 1680, null, true, $shouldCreate); + } catch (Exception $exception) { + // Prevent thumbnail errors from stopping execution + } + + $image->setAttribute('thumbs', $thumbs); + } + /** * Get the thumbnail for an image. * If $keepRatio is true only the width will be used. @@ -29,8 +59,7 @@ class ImageResizer ?int $width, ?int $height, bool $keepRatio = false, - bool $shouldCreate = false, - bool $canCreate = false, + bool $shouldCreate = false ): ?string { // Do not resize GIF images where we're not cropping if ($keepRatio && $this->isGif($image)) { @@ -52,7 +81,7 @@ class ImageResizer // If thumbnail has already been generated, serve that and cache path $disk = $this->storage->getDisk($image->type); if (!$shouldCreate && $disk->exists($thumbFilePath)) { - Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); + Cache::put($thumbCacheKey, $thumbFilePath, static::THUMBNAIL_CACHE_TIME); return $this->storage->getPublicUrl($thumbFilePath); } @@ -61,19 +90,15 @@ class ImageResizer // Do not resize apng images where we're not cropping if ($keepRatio && $this->isApngData($image, $imageData)) { - Cache::put($thumbCacheKey, $image->path, 60 * 60 * 72); + Cache::put($thumbCacheKey, $image->path, static::THUMBNAIL_CACHE_TIME); return $this->storage->getPublicUrl($image->path); } - if (!$shouldCreate && !$canCreate) { - return null; - } - // If not in cache and thumbnail does not exist, generate thumb and cache path $thumbData = $this->resizeImageData($imageData, $width, $height, $keepRatio); $disk->put($thumbFilePath, $thumbData, true); - Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72); + Cache::put($thumbCacheKey, $thumbFilePath, static::THUMBNAIL_CACHE_TIME); return $this->storage->getPublicUrl($thumbFilePath); } diff --git a/lang/en/errors.php b/lang/en/errors.php index 285817e47..8813cf90a 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -51,8 +51,9 @@ return [ 'image_upload_error' => 'An error occurred uploading the image', 'image_upload_type_error' => 'The image type being uploaded is invalid', 'image_upload_replace_type' => 'Image file replacements must be of the same type', - 'image_upload_memory_limit' => 'Failed to handle image upload and/or create thumbnails due to system resource limits', - 'image_thumbnail_memory_limit' => 'Failed to create image size variations due to system resource limits', + 'image_upload_memory_limit' => 'Failed to handle image upload and/or create thumbnails due to system resource limits.', + 'image_thumbnail_memory_limit' => 'Failed to create image size variations due to system resource limits.', + 'image_gallery_thumbnail_memory_limit' => 'Failed to create gallery thumbnails due to system resource limits.', 'drawing_data_not_found' => 'Drawing data could not be loaded. The drawing file might no longer exist or you may not have permission to access it.', // Attachments diff --git a/resources/js/components/image-manager.js b/resources/js/components/image-manager.js index bc0493a88..b6397b004 100644 --- a/resources/js/components/image-manager.js +++ b/resources/js/components/image-manager.js @@ -1,6 +1,4 @@ -import { - onChildEvent, onSelect, removeLoading, showLoading, -} from '../services/dom'; +import {onChildEvent, onSelect, removeLoading, showLoading,} from '../services/dom'; import {Component} from './component'; export class ImageManager extends Component { @@ -229,8 +227,8 @@ export class ImageManager extends Component { this.loadGallery(); } - onImageSelectEvent(event) { - const image = JSON.parse(event.detail.data); + async onImageSelectEvent(event) { + let image = JSON.parse(event.detail.data); const isDblClick = ((image && image.id === this.lastSelected.id) && Date.now() - this.lastSelectedTime < 400); const alreadySelected = event.target.classList.contains('selected'); @@ -238,12 +236,15 @@ export class ImageManager extends Component { el.classList.remove('selected'); }); - if (!alreadySelected) { + if (!alreadySelected && !isDblClick) { event.target.classList.add('selected'); - this.loadImageEditForm(image.id); - } else { + image = await this.loadImageEditForm(image.id); + } else if (!isDblClick) { this.resetEditForm(); + } else if (isDblClick) { + image = this.lastSelected; } + this.selectButton.classList.toggle('hidden', alreadySelected); if (isDblClick && this.callback) { @@ -265,6 +266,9 @@ export class ImageManager extends Component { this.formContainer.innerHTML = formHtml; this.formContainerPlaceholder.setAttribute('hidden', ''); window.$components.init(this.formContainer); + + const imageDataEl = this.formContainer.querySelector('#image-manager-form-image-data'); + return JSON.parse(imageDataEl.text); } runLoadMore() { diff --git a/resources/js/markdown/actions.js b/resources/js/markdown/actions.js index a7fde9322..4909a59d0 100644 --- a/resources/js/markdown/actions.js +++ b/resources/js/markdown/actions.js @@ -34,7 +34,7 @@ export class Actions { const imageManager = window.$components.first('image-manager'); imageManager.show(image => { - const imageUrl = image.thumbs.display || image.url; + const imageUrl = image.thumbs?.display || image.url; const selectedText = this.#getSelectionText(); const newText = `[![${selectedText || image.name}](${imageUrl})](${image.url})`; this.#replaceSelection(newText, newText.length); @@ -417,7 +417,7 @@ export class Actions { const newContent = `[![](${data.thumbs.display})](${data.url})`; this.#findAndReplaceContent(placeHolderText, newContent); } catch (err) { - window.$events.emit('error', this.editor.config.text.imageUploadError); + window.$events.error(err?.data?.message || this.editor.config.text.imageUploadError); this.#findAndReplaceContent(placeHolderText, ''); console.error(err); } diff --git a/resources/js/wysiwyg/drop-paste-handling.js b/resources/js/wysiwyg/drop-paste-handling.js index 33078cd1d..9668692c8 100644 --- a/resources/js/wysiwyg/drop-paste-handling.js +++ b/resources/js/wysiwyg/drop-paste-handling.js @@ -61,7 +61,7 @@ function paste(editor, options, event) { editor.dom.replace(newEl, id); }).catch(err => { editor.dom.remove(id); - window.$events.emit('error', options.translations.imageUploadErrorText); + window.$events.error(err?.data?.message || options.translations.imageUploadErrorText); console.error(err); }); }, 10); diff --git a/resources/js/wysiwyg/plugins-imagemanager.js b/resources/js/wysiwyg/plugins-imagemanager.js index 37b5bfafd..f1ea12050 100644 --- a/resources/js/wysiwyg/plugins-imagemanager.js +++ b/resources/js/wysiwyg/plugins-imagemanager.js @@ -11,7 +11,7 @@ function register(editor) { /** @type {ImageManager} * */ const imageManager = window.$components.first('image-manager'); imageManager.show(image => { - const imageUrl = image.thumbs.display || image.url; + const imageUrl = image.thumbs?.display || image.url; let html = ``; html += `${image.name}`; html += ''; diff --git a/resources/sass/_components.scss b/resources/sass/_components.scss index c1989c1f6..150f78e12 100644 --- a/resources/sass/_components.scss +++ b/resources/sass/_components.scss @@ -457,6 +457,18 @@ body.flexbox-support #entity-selector-wrap .popup-body .form-group { text-align: center; } +.image-manager-list .image-manager-list-warning { + grid-column: 1 / -1; + aspect-ratio: auto; +} + +.image-manager-warning { + @include lightDark(background, #FFF, #333); + color: var(--color-warning); + font-weight: bold; + border-inline: 3px solid var(--color-warning); +} + .image-manager-sidebar { width: 300px; margin: 0 auto; diff --git a/resources/views/pages/parts/image-manager-form.blade.php b/resources/views/pages/parts/image-manager-form.blade.php index 3a73bee7c..bd84e247d 100644 --- a/resources/views/pages/parts/image-manager-form.blade.php +++ b/resources/views/pages/parts/image-manager-form.blade.php @@ -8,8 +8,17 @@ option:dropzone:file-accept="image/*" class="image-manager-details"> + @if($warning ?? '') +
    +
    @icon('warning')
    +
    {{ $warning }}
    +
    + @endif +
    + +
    +
    @icon('warning')
    +
    {{ $warning }}
    + +@endif @foreach($images as $index => $image)
    - - @if(userCan('attachment-create-all')) - - @endif - - @if($comments->enabled()) - - @endif +
    +
    + + + @if(userCan('attachment-create-all')) + + @endif + + @if($comments->enabled()) + + @endif +
    diff --git a/resources/views/pages/parts/form.blade.php b/resources/views/pages/parts/form.blade.php index 6d59afe33..56f249baa 100644 --- a/resources/views/pages/parts/form.blade.php +++ b/resources/views/pages/parts/form.blade.php @@ -1,4 +1,4 @@ -
    - {{--Editors--}} -
    +
    + {{--Editors--}} +
    - {{--WYSIWYG Editor--}} - @if($editor === 'wysiwyg') - @include('pages.parts.wysiwyg-editor', ['model' => $model]) - @endif + {{--WYSIWYG Editor--}} + @if($editor === 'wysiwyg') + @include('pages.parts.wysiwyg-editor', ['model' => $model]) + @endif - {{--Markdown Editor--}} - @if($editor === 'markdown') - @include('pages.parts.markdown-editor', ['model' => $model]) - @endif + {{--Markdown Editor--}} + @if($editor === 'markdown') + @include('pages.parts.markdown-editor', ['model' => $model]) + @endif +
    + + @include('pages.parts.editor-toolbox')
    {{--Mobile Save Button--}} From 45c74090929338323d72787a90a66391e554805e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 13 Oct 2023 17:33:11 +0100 Subject: [PATCH 73/97] Editor: Started toying with more singificant design update --- resources/sass/_blocks.scss | 1 + resources/sass/_header.scss | 39 ---- resources/sass/_pages.scss | 45 +++-- .../pages/parts/editor-toolbar.blade.php | 191 ++++++++++-------- resources/views/pages/parts/form.blade.php | 41 ++-- 5 files changed, 161 insertions(+), 156 deletions(-) diff --git a/resources/sass/_blocks.scss b/resources/sass/_blocks.scss index ca6efbbe1..d63ed3802 100644 --- a/resources/sass/_blocks.scss +++ b/resources/sass/_blocks.scss @@ -155,6 +155,7 @@ margin-bottom: $-l; overflow: initial; min-height: 60vh; + border-radius: 8px; &.auto-height { min-height: 0; } diff --git a/resources/sass/_header.scss b/resources/sass/_header.scss index 4a4c70401..e2daae437 100644 --- a/resources/sass/_header.scss +++ b/resources/sass/_header.scss @@ -364,43 +364,4 @@ header .search-box.search-active:focus-within { .faded span.faded-text { display: inline-block; padding: $-s; -} - -.action-buttons .text-button { - display: inline-block; - padding: $-xs $-s; - &:last-child { - padding-inline-end: 0; - } - &:first-child { - padding-inline-start: 0; - } -} - - -.action-buttons .dropdown-container:last-child a { - padding-inline-end: 0; - padding-inline-start: $-s; -} -.action-buttons { - text-align: end; - &.text-left { - text-align: start; - .text-button { - padding-inline-end: $-m; - padding-inline-start: 0; - } - } - &.text-center { - text-align: center; - } -} - -@include smaller-than($m) { - .action-buttons .text-button { - padding: $-xs $-xs; - } - .action-buttons .dropdown-container:last-child a { - padding-inline-start: $-xs; - } } \ No newline at end of file diff --git a/resources/sass/_pages.scss b/resources/sass/_pages.scss index c3a4b3b44..dd88a6b7b 100755 --- a/resources/sass/_pages.scss +++ b/resources/sass/_pages.scss @@ -19,8 +19,21 @@ } } -.page-edit-toolbar .text-button { - font-size: $fs-m; +.page-editor-page-area { + width: 100%; + max-width: 1140px; + border-radius: 8px; + box-shadow: $bs-card; + overflow: hidden; +} + +.page-edit-toolbar { + width: 100%; + max-width: 1140px; + margin: 0 auto; + display: grid; + grid-template-columns: minmax(max-content, 2fr) 1.5fr minmax(max-content, 2fr); + align-items: center; } body.tox-fullscreen .page-editor .edit-area, @@ -155,18 +168,25 @@ body.tox-fullscreen, body.markdown-fullscreen { // Attribute form .floating-toolbox { - //border-left: 1px solid #CCC; @include lightDark(background-color, #FFF, #222); - @include lightDark(border-color, #DDD, #000); - right: $-xl*2; - width: 40px; overflow: hidden; align-items: stretch; flex-direction: row; display: flex; - min-height: 0; + max-height: 100%; + border-radius: 8px; + box-shadow: $bs-card; + position: fixed; + right: $-s; + margin-bottom: auto; &.open { - width: 480px; + right: 0; + position: relative; + max-width: 480px; + margin-bottom: 0; + } + &:not(.open) .toolbox-tab-content { + display: none !important; } .toolbox-toggle svg { transition: transform ease-in-out 180ms; @@ -185,7 +205,7 @@ body.tox-fullscreen, body.markdown-fullscreen { position: relative; } .tabs { - border: 1px solid #CCC; + border-right: 1px solid #DDD; width: 40px; flex: 0 1 auto; margin-right: -1px; @@ -210,9 +230,9 @@ body.tox-fullscreen, body.markdown-fullscreen { font-size: 16px; line-height: 1.6; } - &.open .tabs > button.active { - @include lightDark(color, #444, #EEE); - background-color: rgba(0, 0, 0, 0.1); + &.open .tabs-inner > button.active { + background-color: var(--color-primary-light); + color: var(--color-primary) } h4 { font-size: 24px; @@ -249,7 +269,6 @@ body.tox-fullscreen, body.markdown-fullscreen { display: none; overflow-y: auto; padding-bottom: 45px; - border-block-start: 1px solid #CCC; } .suggestion-box { diff --git a/resources/views/pages/parts/editor-toolbar.blade.php b/resources/views/pages/parts/editor-toolbar.blade.php index ccc92794e..d25f6a0a4 100644 --- a/resources/views/pages/parts/editor-toolbar.blade.php +++ b/resources/views/pages/parts/editor-toolbar.blade.php @@ -1,97 +1,116 @@ -
    -
    +
    -
    +
    +
    @icon('back'){{ trans('common.back') }} + class="icon-list-item text-link">@icon('back'){{ trans('common.back') }}
    +
    -
    -