diff --git a/extensions/gdpr/src/Data/Assets.php b/extensions/gdpr/src/Data/Assets.php index 98256fdaff..017162c750 100644 --- a/extensions/gdpr/src/Data/Assets.php +++ b/extensions/gdpr/src/Data/Assets.php @@ -9,6 +9,7 @@ namespace Flarum\Gdpr\Data; +use Flarum\User\AvatarUploader; use Illuminate\Support\Str; class Assets extends Type @@ -52,10 +53,9 @@ public function anonymize(): void public function delete(): void { if ($this->user->avatar_url) { - $filesystem = $this->getDisk('flarum-avatars'); - $fileName = $this->getAvatarFileName(); - - $filesystem->exists($fileName) && $filesystem->delete($fileName); + // Delegate to AvatarUploader so all HiDPI variants (@2x, @3x) are + // removed too — not just the base file. + resolve(AvatarUploader::class)->deleteAllVariants($this->getAvatarFileName()); } } diff --git a/extensions/gdpr/src/Data/User.php b/extensions/gdpr/src/Data/User.php index a7a2db3b43..6b13d2bc21 100644 --- a/extensions/gdpr/src/Data/User.php +++ b/extensions/gdpr/src/Data/User.php @@ -33,7 +33,11 @@ public function anonymize(): void { $columns = $this->getTableColumns($this->user); - $remove = ['id', 'username', 'password', 'email', 'is_email_confirmed', 'preferences', 'joined_at', 'anonymized', 'discussion_count', 'comment_count']; + // `$remove` here means "skip this column when wiping to null" — i.e. preserve + // the existing value or set it explicitly below. NOT NULL boolean columns + // (e.g. has_avatar_*) are listed here because nulling them would violate + // their constraint; they're set explicitly after the loop instead. + $remove = ['id', 'username', 'password', 'email', 'is_email_confirmed', 'preferences', 'joined_at', 'anonymized', 'discussion_count', 'comment_count', 'has_avatar_2x', 'has_avatar_3x']; foreach ($columns as $column) { if (in_array($column, $remove)) { @@ -51,6 +55,10 @@ public function anonymize(): void $this->user->setPreferencesAttribute([]); $this->user->joined_at = Carbon::now(); $this->user->anonymized = true; + // The avatar files themselves are removed by Data\Assets::anonymize(); reset + // the variant flags here to match. + $this->user->has_avatar_2x = false; + $this->user->has_avatar_3x = false; $this->user->groups()->sync([]); $this->user->save(); diff --git a/framework/core/migrations/2026_05_07_000000_add_avatar_variant_flags_to_users.php b/framework/core/migrations/2026_05_07_000000_add_avatar_variant_flags_to_users.php new file mode 100644 index 0000000000..60bffd0bad --- /dev/null +++ b/framework/core/migrations/2026_05_07_000000_add_avatar_variant_flags_to_users.php @@ -0,0 +1,26 @@ + function (Builder $schema) { + $schema->table('users', function (Blueprint $table) { + $table->boolean('has_avatar_2x')->default(false)->after('avatar_url'); + $table->boolean('has_avatar_3x')->default(false)->after('has_avatar_2x'); + }); + }, + + 'down' => function (Builder $schema) { + $schema->table('users', function (Blueprint $table) { + $table->dropColumn(['has_avatar_2x', 'has_avatar_3x']); + }); + } +]; diff --git a/framework/core/src/Console/ConsoleServiceProvider.php b/framework/core/src/Console/ConsoleServiceProvider.php index 5a0d43d96b..86057a0c8c 100644 --- a/framework/core/src/Console/ConsoleServiceProvider.php +++ b/framework/core/src/Console/ConsoleServiceProvider.php @@ -19,6 +19,7 @@ use Flarum\Foundation\Console\AssetsPublishCommand; use Flarum\Foundation\Console\CacheClearCommand; use Flarum\Foundation\Console\InfoCommand; +use Flarum\User\Console\BackfillAvatarVariantsCommand; use Flarum\User\Console\ConvertAvatarsToWebpCommand; use Illuminate\Console\Events\CommandFinished; use Illuminate\Console\Scheduling\CacheEventMutex; @@ -71,6 +72,7 @@ public function register(): void ToggleExtensionCommand::class, BisectCommand::class, ConvertAvatarsToWebpCommand::class, + BackfillAvatarVariantsCommand::class, // Used internally to create DB dumps before major releases. // \Flarum\Database\Console\GenerateDumpCommand::class ]; diff --git a/framework/core/src/User/AvatarUploader.php b/framework/core/src/User/AvatarUploader.php index 3d9889e911..73fcc8ae1f 100644 --- a/framework/core/src/User/AvatarUploader.php +++ b/framework/core/src/User/AvatarUploader.php @@ -61,6 +61,9 @@ public function uploadPresized(User $user, ImageInterface $image1x, ?ImageInterf $this->uploadDir->put($path, $encoded); } + + $user->has_avatar_2x = $image2x !== null; + $user->has_avatar_3x = $image3x !== null; } public function upload(User $user, ImageInterface $image): void @@ -77,6 +80,8 @@ public function upload(User $user, ImageInterface $image): void $this->removeFileAfterSave($user); $user->changeAvatarPath($basePath); + $generated = ['' => false, '@2x' => false, '@3x' => false]; + foreach (self::SIZES as $suffix => $size) { // Never upscale — skip this variant if the source is too small. if ($sourceWidth < $size || $sourceHeight < $size) { @@ -89,7 +94,11 @@ public function upload(User $user, ImageInterface $image): void $path = $avatarBase.$suffix.'.'.$extension; $this->uploadDir->put($path, $encoded); + $generated[$suffix] = true; } + + $user->has_avatar_2x = $generated['@2x']; + $user->has_avatar_3x = $generated['@3x']; } /** @@ -115,6 +124,8 @@ public function remove(User $user): void $this->removeFileAfterSave($user); $user->changeAvatarPath(null); + $user->has_avatar_2x = false; + $user->has_avatar_3x = false; } /** @@ -134,37 +145,31 @@ public function deleteAllVariants(string $basePath): void } /** - * Return the srcset string for a given base path, including only variants that exist on disk. - * Returns null if only the base file exists (no HiDPI variants). + * Return the srcset string for the user's locally-stored avatar, including only + * variants that the user record reports as present (`has_avatar_2x` / + * `has_avatar_3x`). Returns null if the avatar is external, missing, or has no + * HiDPI variants — this avoids any filesystem `exists()` calls in the read path. */ - public function srcsetFor(string $basePath): ?string + public function srcsetFor(User $user): ?string { - // External URLs (e.g. from OAuth provider) — no local variants. - if (str_contains($basePath, '://')) { - return null; - } - - // Collect which variant paths exist on disk first, without calling url(). - $existing = []; - - foreach (self::SIZES as $suffix => $size) { - $path = $this->variantPath($basePath, $suffix); + $basePath = $user->getRawOriginal('avatar_url'); - if ($this->uploadDir->exists($path)) { - $existing[$path] = $size / 100; - } + if (! $basePath || str_contains($basePath, '://')) { + return null; } - // Only meaningful if we have more than just the 1× base. - if (count($existing) <= 1) { + if (! $user->has_avatar_2x && ! $user->has_avatar_3x) { return null; } - $entries = []; + $entries = [$this->uploadDir->url($basePath).' 1x']; + + if ($user->has_avatar_2x) { + $entries[] = $this->uploadDir->url($this->variantPath($basePath, '@2x')).' 2x'; + } - foreach ($existing as $path => $multiplier) { - $url = $this->uploadDir->url($path); - $entries[] = "$url {$multiplier}x"; + if ($user->has_avatar_3x) { + $entries[] = $this->uploadDir->url($this->variantPath($basePath, '@3x')).' 3x'; } return implode(', ', $entries); diff --git a/framework/core/src/User/Console/BackfillAvatarVariantsCommand.php b/framework/core/src/User/Console/BackfillAvatarVariantsCommand.php new file mode 100644 index 0000000000..7500969220 --- /dev/null +++ b/framework/core/src/User/Console/BackfillAvatarVariantsCommand.php @@ -0,0 +1,136 @@ +uploadDir = $filesystemFactory->disk('flarum-avatars'); + parent::__construct(); + } + + protected function configure(): void + { + $this + ->setName('avatars:backfill-variants') + ->setDescription('Detect and record which HiDPI variants (@2x, @3x) exist for each locally-stored avatar.') + ->addOption( + 'chunk', + null, + InputOption::VALUE_REQUIRED, + 'Number of users to process per database chunk', + '100' + ) + ->addOption( + 'force', + null, + InputOption::VALUE_NONE, + 'Re-check users even if both flags are already set' + ); + } + + protected function fire(): int + { + $chunkSize = max(1, (int) $this->input->getOption('chunk')); + $force = (bool) $this->input->getOption('force'); + + $query = User::whereNotNull('avatar_url') + ->where('avatar_url', 'not like', '%://%'); + + if (! $force) { + $query->where(function ($q) { + $q->where('has_avatar_2x', false)->orWhere('has_avatar_3x', false); + }); + } + + $total = $query->count(); + + if ($total === 0) { + $this->info('No avatars to check.'); + + return Command::SUCCESS; + } + + $this->info("Checking variant files for $total avatar(s)..."); + + $progress = new ProgressBar($this->output, $total); + $progress->start(); + + $updated = 0; + $unchanged = 0; + $missing = 0; + + $query->chunkById($chunkSize, function ($users) use ($progress, &$updated, &$unchanged, &$missing) { + foreach ($users as $user) { + $basePath = $user->getRawOriginal('avatar_url'); + + // The base file must exist for variants to be meaningful — a user + // whose 1× is missing is broken regardless of variant flags. + if (! $this->uploadDir->exists($basePath)) { + $missing++; + $progress->advance(); + continue; + } + + $has2x = $this->uploadDir->exists($this->variantPath($basePath, '@2x')); + $has3x = $this->uploadDir->exists($this->variantPath($basePath, '@3x')); + + if ($user->has_avatar_2x === $has2x && $user->has_avatar_3x === $has3x) { + $unchanged++; + } else { + User::where('id', $user->id)->update([ + 'has_avatar_2x' => $has2x, + 'has_avatar_3x' => $has3x, + ]); + $updated++; + } + + $progress->advance(); + } + }); + + $progress->finish(); + $this->output->writeln(''); + $this->info("Done. Updated: $updated, Unchanged: $unchanged, Base file missing: $missing."); + + return Command::SUCCESS; + } + + private function variantPath(string $basePath, string $suffix): string + { + $dot = strrpos($basePath, '.'); + + return $dot !== false + ? substr($basePath, 0, $dot).$suffix.substr($basePath, $dot) + : $basePath.$suffix; + } +} diff --git a/framework/core/src/User/User.php b/framework/core/src/User/User.php index c6ecca70c7..a0b2f65b25 100644 --- a/framework/core/src/User/User.php +++ b/framework/core/src/User/User.php @@ -49,6 +49,8 @@ * @property bool $is_email_confirmed * @property string $password * @property string|null $avatar_url + * @property bool $has_avatar_2x + * @property bool $has_avatar_3x * @property array $preferences * @property \Carbon\Carbon|null $joined_at * @property \Carbon\Carbon|null $last_seen_at @@ -77,6 +79,8 @@ class User extends AbstractModel protected $casts = [ 'id' => 'integer', 'is_email_confirmed' => 'boolean', + 'has_avatar_2x' => 'boolean', + 'has_avatar_3x' => 'boolean', 'joined_at' => 'datetime', 'last_seen_at' => 'datetime', 'marked_all_as_read_at' => 'datetime', @@ -295,7 +299,7 @@ public function getAvatarSrcsetAttribute(): ?string $value = $this->getRawOriginal('avatar_url'); if ($value && ! str_contains($value, '://')) { - return resolve(AvatarUploader::class)->srcsetFor($value); + return resolve(AvatarUploader::class)->srcsetFor($this); } return static::$avatarDriver->avatarSrcset($this); diff --git a/framework/core/tests/integration/api/users/AvatarSrcsetTest.php b/framework/core/tests/integration/api/users/AvatarSrcsetTest.php index ff0e32a33d..deb87df390 100644 --- a/framework/core/tests/integration/api/users/AvatarSrcsetTest.php +++ b/framework/core/tests/integration/api/users/AvatarSrcsetTest.php @@ -41,6 +41,26 @@ protected function setUp(): void 'is_email_confirmed' => 1, 'avatar_url' => 'https://example.com/avatar.png', ], + [ + 'id' => 5, + 'username' => 'hidpi_avatar_user', + 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', + 'email' => 'hidpiavatar@machine.local', + 'is_email_confirmed' => 1, + 'avatar_url' => 'XYZxyz12345.webp', + 'has_avatar_2x' => 1, + 'has_avatar_3x' => 1, + ], + [ + 'id' => 6, + 'username' => 'two_x_only_user', + 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', + 'email' => 'twoxonly@machine.local', + 'is_email_confirmed' => 1, + 'avatar_url' => 'PQRpqr67890.webp', + 'has_avatar_2x' => 1, + 'has_avatar_3x' => 0, + ], ], ]); } @@ -97,6 +117,50 @@ public function user_with_local_avatar_but_no_hidpi_variants_has_null_avatar_src $this->assertNull($data['data']['attributes']['avatarSrcset']); } + #[Test] + public function user_with_hidpi_variant_flags_returns_full_srcset(): void + { + $response = $this->send( + $this->request('GET', '/api/users/5', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode((string) $response->getBody(), true); + $srcset = $data['data']['attributes']['avatarSrcset'] ?? null; + + $this->assertNotNull($srcset, 'Expected non-null srcset for user with both variant flags set.'); + $this->assertStringContainsString('1x', $srcset); + $this->assertStringContainsString('2x', $srcset); + $this->assertStringContainsString('3x', $srcset); + $this->assertStringContainsString('XYZxyz12345.webp', $srcset); + $this->assertStringContainsString('XYZxyz12345@2x.webp', $srcset); + $this->assertStringContainsString('XYZxyz12345@3x.webp', $srcset); + } + + #[Test] + public function user_with_only_2x_variant_flag_omits_3x_from_srcset(): void + { + $response = $this->send( + $this->request('GET', '/api/users/6', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode((string) $response->getBody(), true); + $srcset = $data['data']['attributes']['avatarSrcset'] ?? null; + + $this->assertNotNull($srcset); + $this->assertStringContainsString('1x', $srcset); + $this->assertStringContainsString('2x', $srcset); + $this->assertStringNotContainsString('3x', $srcset); + $this->assertStringNotContainsString('PQRpqr67890@3x.webp', $srcset); + } + #[Test] public function deleting_avatar_returns_null_avatar_srcset(): void { diff --git a/framework/core/tests/unit/User/AvatarUploaderTest.php b/framework/core/tests/unit/User/AvatarUploaderTest.php index 6fa5acfb29..96e2094d7d 100644 --- a/framework/core/tests/unit/User/AvatarUploaderTest.php +++ b/framework/core/tests/unit/User/AvatarUploaderTest.php @@ -50,6 +50,8 @@ public function test_removing_avatar_removes_file() $user = new User(); $user->changeAvatarPath('ABCDEFGHabcdefgh.png'); + $user->has_avatar_2x = true; + $user->has_avatar_3x = true; $user->syncOriginal(); $this->uploader->remove($user); @@ -60,6 +62,8 @@ public function test_removing_avatar_removes_file() $user->syncOriginal(); $this->assertEquals(null, $user->getRawOriginal('avatar_url')); + $this->assertFalse($user->has_avatar_2x); + $this->assertFalse($user->has_avatar_3x); } #[Test] @@ -146,6 +150,8 @@ public function test_upload_generates_all_three_variants_for_large_source() $this->assertStringNotContainsString('@', $putPaths[0]); $this->assertStringContainsString('@2x', $putPaths[1]); $this->assertStringContainsString('@3x', $putPaths[2]); + $this->assertTrue($user->has_avatar_2x); + $this->assertTrue($user->has_avatar_3x); } #[Test] @@ -164,6 +170,8 @@ public function test_upload_skips_variants_that_would_require_upscaling() $this->assertCount(1, $putPaths); $this->assertStringNotContainsString('@', $putPaths[0]); + $this->assertFalse($user->has_avatar_2x); + $this->assertFalse($user->has_avatar_3x); } #[Test] @@ -183,31 +191,63 @@ public function test_upload_generates_two_variants_for_mid_size_source() $this->assertCount(2, $putPaths); $this->assertStringNotContainsString('@', $putPaths[0]); $this->assertStringContainsString('@2x', $putPaths[1]); + $this->assertTrue($user->has_avatar_2x); + $this->assertFalse($user->has_avatar_3x); } #[Test] - public function test_srcset_for_returns_null_when_only_base_exists() + public function test_upload_presized_records_only_supplied_variants() { - $this->filesystem->shouldReceive('exists')->with('abc.webp')->andReturn(true); - $this->filesystem->shouldReceive('exists')->with('abc@2x.webp')->andReturn(false); - $this->filesystem->shouldReceive('exists')->with('abc@3x.webp')->andReturn(false); + $this->filesystem->shouldReceive('put')->atLeast()->once(); + $this->filesystem->shouldIgnoreMissing(); + + $user = new User(); - $result = $this->uploader->srcsetFor('abc.webp'); + // OAuth path with 1× and 2× only — no 3×. + $this->uploader->uploadPresized( + $user, + ImageManager::gd()->create(100, 100), + ImageManager::gd()->create(200, 200), + null, + ); - $this->assertNull($result); + $this->assertTrue($user->has_avatar_2x); + $this->assertFalse($user->has_avatar_3x); } #[Test] - public function test_srcset_for_returns_string_when_hidpi_variants_exist() + public function test_srcset_for_returns_null_when_no_variants_recorded() { - $this->filesystem->shouldReceive('exists')->with('abc.webp')->andReturn(true); - $this->filesystem->shouldReceive('exists')->with('abc@2x.webp')->andReturn(true); - $this->filesystem->shouldReceive('exists')->with('abc@3x.webp')->andReturn(true); + // No filesystem calls should happen on the read path. + $this->filesystem->shouldNotReceive('exists'); + $this->filesystem->shouldNotReceive('url'); + + $user = new User(); + $user->setRawAttributes([ + 'avatar_url' => 'abc.webp', + 'has_avatar_2x' => false, + 'has_avatar_3x' => false, + ], true); + + $this->assertNull($this->uploader->srcsetFor($user)); + } + + #[Test] + public function test_srcset_for_returns_string_when_hidpi_variants_recorded() + { + $this->filesystem->shouldNotReceive('exists'); $this->filesystem->shouldReceive('url')->with('abc.webp')->andReturn('https://cdn.example.com/abc.webp'); $this->filesystem->shouldReceive('url')->with('abc@2x.webp')->andReturn('https://cdn.example.com/abc@2x.webp'); $this->filesystem->shouldReceive('url')->with('abc@3x.webp')->andReturn('https://cdn.example.com/abc@3x.webp'); - $result = $this->uploader->srcsetFor('abc.webp'); + $user = new User(); + $user->setRawAttributes([ + 'avatar_url' => 'abc.webp', + 'has_avatar_2x' => true, + 'has_avatar_3x' => true, + ], true); + + $result = $this->uploader->srcsetFor($user); $this->assertNotNull($result); $this->assertStringContainsString('1x', $result); @@ -215,14 +255,51 @@ public function test_srcset_for_returns_string_when_hidpi_variants_exist() $this->assertStringContainsString('3x', $result); } + #[Test] + public function test_srcset_for_includes_only_recorded_variants() + { + $this->filesystem->shouldNotReceive('exists'); + $this->filesystem->shouldReceive('url')->with('abc.webp')->andReturn('https://cdn.example.com/abc.webp'); + $this->filesystem->shouldReceive('url')->with('abc@2x.webp')->andReturn('https://cdn.example.com/abc@2x.webp'); + $this->filesystem->shouldNotReceive('url')->with('abc@3x.webp'); + + $user = new User(); + $user->setRawAttributes([ + 'avatar_url' => 'abc.webp', + 'has_avatar_2x' => true, + 'has_avatar_3x' => false, + ], true); + + $result = $this->uploader->srcsetFor($user); + + $this->assertNotNull($result); + $this->assertStringContainsString('1x', $result); + $this->assertStringContainsString('2x', $result); + $this->assertStringNotContainsString('3x', $result); + } + #[Test] public function test_srcset_for_returns_null_for_external_url() { $this->filesystem->shouldNotReceive('exists'); + $this->filesystem->shouldNotReceive('url'); + + $user = new User(); + $user->setRawAttributes(['avatar_url' => 'https://example.com/avatar.png'], true); + + $this->assertNull($this->uploader->srcsetFor($user)); + } + + #[Test] + public function test_srcset_for_returns_null_when_no_avatar() + { + $this->filesystem->shouldNotReceive('exists'); + $this->filesystem->shouldNotReceive('url'); - $result = $this->uploader->srcsetFor('https://example.com/avatar.png'); + $user = new User(); + $user->setRawAttributes(['avatar_url' => null], true); - $this->assertNull($result); + $this->assertNull($this->uploader->srcsetFor($user)); } #[Test]