feat: better supports for compilation when scanning

app/Console/Commands/SyncCommand.php

@@ -13,7 +13,7 @@ class SyncCommand extends Command
 {
     protected $signature = 'koel:sync
         {record? : A single watch record. Consult Wiki for more info.}
-        {--tags= : The comma-separated tags to sync into the database}
+        {--excludes= : The comma-separated tags to excludes from syncing}
         {--force : Force re-syncing even unchanged files}';
 
     protected $description = 'Sync songs found in configured directory against the database.';
@@ -42,7 +42,7 @@ class SyncCommand extends Command
             return;
         }
 
-        $this->syngle($record);
+        $this->syncSingleRecord($record);
     }
 
     /**
@@ -52,12 +52,12 @@ class SyncCommand extends Command
     {
         $this->info('Syncing media from ' . Setting::get('media_path') . PHP_EOL);
 
-        // Get the tags to sync.
+        // The excluded tags.
         // Notice that this is only meaningful for existing records.
-        // New records will have every applicable field sync'ed in.
+        // New records will have every applicable field synced in.
-        $tags = $this->option('tags') ? explode(',', $this->option('tags')) : [];
+        $excludes = $this->option('excludes') ? explode(',', $this->option('excludes')) : [];
 
-        $this->mediaSyncService->sync(null, $tags, $this->option('force'), $this);
+        $this->mediaSyncService->sync(null, $excludes, $this->option('force'), $this);
 
         $this->output->writeln(
             PHP_EOL . PHP_EOL
@@ -68,8 +68,6 @@ class SyncCommand extends Command
     }
 
     /**
-     * SYNc a sinGLE file or directory. See my awesome pun?
-     *
      * @param string $record The watch record.
      *                       As of current we only support inotifywait.
      *                       Some examples:
@@ -79,7 +77,7 @@ class SyncCommand extends Command
      *
      * @see http://man7.org/linux/man-pages/man1/inotifywait.1.html
      */
-    public function syngle(string $record): void
+    public function syncSingleRecord(string $record): void
     {
         $this->mediaSyncService->syncByWatchRecord(new InotifyWatchRecord($record));
     }

app/Repositories/AlbumRepository.php

@@ -44,7 +44,6 @@ class AlbumRepository extends AbstractRepository
     public function getByIds(array $ids, ?User $scopedUser = null): Collection
     {
         return Album::withMeta($scopedUser ?? $this->auth->user())
-            ->isStandard()
             ->whereIn('albums.id', $ids)
             ->get();
     }

app/Services/FileSynchronizer.php

@@ -6,10 +6,10 @@ use App\Models\Album;
 use App\Models\Artist;
 use App\Models\Song;
 use App\Repositories\SongRepository;
+use App\Values\SongScanInformation;
 use getID3;
-use getid3_lib;
 use Illuminate\Contracts\Cache\Repository as Cache;
-use InvalidArgumentException;
+use Illuminate\Support\Arr;
 use SplFileInfo;
 use Symfony\Component\Finder\Finder;
 use Throwable;
@@ -20,12 +20,6 @@ class FileSynchronizer
     public const SYNC_RESULT_BAD_FILE = 2;
     public const SYNC_RESULT_UNMODIFIED = 3;
 
-    private getID3 $getID3;
-    private MediaMetadataService $mediaMetadataService;
-    private Helper $helper;
-    private SongRepository $songRepository;
-    private Cache $cache;
-    private Finder $finder;
     private ?int $fileModifiedTime = null;
     private ?string $filePath = null;
 
@@ -43,35 +37,19 @@ class FileSynchronizer
     private ?string $syncError;
 
     public function __construct(
-        getID3 $getID3,
+        private getID3 $getID3,
-        MediaMetadataService $mediaMetadataService,
+        private MediaMetadataService $mediaMetadataService,
-        Helper $helper,
+        private Helper $helper,
-        SongRepository $songRepository,
+        private SongRepository $songRepository,
-        Cache $cache,
+        private Cache $cache,
-        Finder $finder
+        private Finder $finder
     ) {
-        $this->getID3 = $getID3;
-        $this->mediaMetadataService = $mediaMetadataService;
-        $this->helper = $helper;
-        $this->songRepository = $songRepository;
-        $this->cache = $cache;
-        $this->finder = $finder;
     }
 
-    /** @param string|SplFileInfo $path */
+    public function setFile(string|SplFileInfo $path): self
-    public function setFile($path): self
     {
-        $splFileInfo = null;
         $splFileInfo = $path instanceof SplFileInfo ? $path : new SplFileInfo($path);
 
-        // Workaround for #344, where getMTime() fails for certain files with Unicode names on Windows.
-        try {
-            $this->fileModifiedTime = $splFileInfo->getMTime();
-        } catch (Throwable $e) {
-            // Not worth logging the error. Just use current stamp for mtime.
-            $this->fileModifiedTime = time();
-        }
-
         $this->filePath = $splFileInfo->getPathname();
         $this->fileHash = $this->helper->getFileHash($this->filePath);
         $this->song = $this->songRepository->getOneById($this->fileHash); // @phpstan-ignore-line
@@ -80,121 +58,55 @@ class FileSynchronizer
         return $this;
     }
 
-    /**
+    public function getFileInfo(): ?SongScanInformation
-     * Get all applicable info from the file.
-     *
-     * @return array<mixed>
-     */
-    public function getFileInfo(): array
     {
         $info = $this->getID3->analyze($this->filePath);
 
         if (isset($info['error']) || !isset($info['playtime_seconds'])) {
             $this->syncError = isset($info['error']) ? $info['error'][0] : 'No playtime found';
 
-            return [];
+            return null;
         }
 
-        // Copy the available tags over to comment.
+        return SongScanInformation::fromGetId3Info($info);
-        // This is a helper from getID3, though it doesn't really work well.
-        // We'll still prefer getting ID3v2 tags directly later.
-        getid3_lib::CopyTagsToComments($info);
-
-        $props = [
-            'artist' => '',
-            'album' => '',
-            'albumartist' => '',
-            'compilation' => false,
-            'title' => basename($this->filePath, '.' . pathinfo($this->filePath, PATHINFO_EXTENSION)),
-            'length' => $info['playtime_seconds'],
-            'track' => $this->getTrackNumberFromInfo($info),
-            'disc' => (int) array_get($info, 'comments.part_of_a_set.0', 1),
-            'lyrics' => '',
-            'cover' => array_get($info, 'comments.picture', [null])[0],
-            'path' => $this->filePath,
-            'mtime' => $this->fileModifiedTime,
-        ];
-
-        $comments = array_get($info, 'comments_html');
-
-        if (!$comments) {
-            return $props;
-        }
-
-        $this->gatherPropsFromTags($info, $comments, $props);
-        $props['compilation'] = (bool) $props['compilation'] || $this->isCompilation($props);
-
-        return $props;
     }
 
     /**
-     * Sync the song with all available media info against the database.
+     * Sync the song with all available media info into the database.
      *
-     * @param array<string> $tags The (selective) tags to sync (if the song exists)
+     * @param array<string> $excludes The tags to exclude (only taken into account if the song already exists)
      * @param bool $force Whether to force syncing, even if the file is unchanged
      */
-    public function sync(array $tags, bool $force = false): int
+    public function sync(array $excludes = [], bool $force = false): int
     {
         if (!$this->isFileNewOrChanged() && !$force) {
             return self::SYNC_RESULT_UNMODIFIED;
         }
 
-        $info = $this->getFileInfo();
+        $info = $this->getFileInfo()?->toArray();
 
         if (!$info) {
             return self::SYNC_RESULT_BAD_FILE;
         }
 
-        // Fixes #366. If the file is new, we use all tags by simply setting $force to false.
+        if (!$this->isFileNew()) {
-        if ($this->isFileNew()) {
+            foreach ($excludes as $tag) {
-            $force = false;
+                unset($info[$tag]);
+            }
         }
 
-        if ($this->isFileChanged() || $force) {
+        $artist = Arr::get($info, 'artist') ? Artist::getOrCreate($info['artist']) : $this->song->artist;
-            // This is a changed file, or the user is forcing updates.
+        $albumArtist = Arr::get($info, 'albumartist') ? Artist::getOrCreate($info['albumartist']) : $artist;
-            // In such a case, the user must have specified a list of tags to sync.
+        $album = Arr::get($info, 'album') ? Album::getOrCreate($albumArtist, $info['album']) : $this->song->album;
-            // A sample command could be: ./artisan koel:sync --force --tags=artist,album,lyrics
-            // We cater for these tags by removing those not specified.
-
-            // There's a special case with 'album' though.
-            // If 'compilation' tag is specified, 'album' must be counted in as well.
-            // But if 'album' isn't specified, we don't want to update normal albums.
-            // This variable is to keep track of this state.
-            $changeCompilationAlbumOnly = false;
-
-            if (in_array('compilation', $tags, true) && !in_array('album', $tags, true)) {
-                $tags[] = 'album';
-                $changeCompilationAlbumOnly = true;
-            }
-
-            $info = array_intersect_key($info, array_flip($tags));
-
-            // If the "artist" tag is specified, use it.
-            // Otherwise, re-use the existing model value.
-            $artist = isset($info['artist']) ? Artist::getOrCreate($info['artist']) : $this->song->album->artist;
-
-            // If the "album" tag is specified, use it.
-            // Otherwise, re-use the existing model value.
-            if (isset($info['album'])) {
-                $album = $changeCompilationAlbumOnly
-                    ? $this->song->album
-                    : Album::getOrCreate($artist, $info['album'], array_get($info, 'compilation'));
-            } else {
-                $album = $this->song->album;
-            }
-        } else {
-            // The file is newly added.
-            $artist = Artist::getOrCreate($info['artist']);
-            $album = Album::getOrCreate($artist, $info['album'], array_get($info, 'compilation'));
-        }
 
         if (!$album->has_cover) {
-            $this->generateAlbumCover($album, array_get($info, 'cover'));
+            $this->tryGenerateAlbumCover($album, Arr::get($info, 'cover', []));
         }
 
-        $data = array_except($info, ['artist', 'albumartist', 'album', 'cover', 'compilation']);
+        $data = Arr::except($info, ['album', 'artist', 'albumartist', 'cover']);
         $data['album_id'] = $album->id;
         $data['artist_id'] = $artist->id;
+
         $this->song = Song::updateOrCreate(['id' => $this->fileHash], $data);
 
         return self::SYNC_RESULT_SUCCESS;
@@ -205,8 +117,9 @@ class FileSynchronizer
      *
      * @param array<mixed>|null $coverData
      */
-    private function generateAlbumCover(Album $album, ?array $coverData): void
+    private function tryGenerateAlbumCover(Album $album, ?array $coverData): void
     {
+        try {
             // If the album has no cover, we try to get the cover image from existing tag data
             if ($coverData) {
                 $extension = explode('/', $coverData['image_mime']);
@@ -224,14 +137,14 @@ class FileSynchronizer
                 $extension = pathinfo($cover, PATHINFO_EXTENSION);
                 $this->mediaMetadataService->writeAlbumCover($album, file_get_contents($cover), $extension);
             }
+        } catch (Throwable) {
+        }
     }
 
     /**
      * Issue #380.
      * Some albums have its own cover image under the same directory as cover|folder.jpg/png.
      * We'll check if such a cover file is found, and use it if positive.
-     *
-     * @throws InvalidArgumentException
      */
     private function getCoverFileUnderSameDirectory(): ?string
     {
@@ -251,15 +164,15 @@ class FileSynchronizer
 
             $cover = $matches ? $matches[0] : null;
 
-            return $cover && $this->isImage($cover) ? $cover : null;
+            return $cover && static::isImage($cover) ? $cover : null;
         });
     }
 
-    private function isImage(string $path): bool
+    private static function isImage(string $path): bool
     {
         try {
             return (bool) exif_imagetype($path);
-        } catch (Throwable $e) {
+        } catch (Throwable) {
             return false;
         }
     }
@@ -290,60 +203,6 @@ class FileSynchronizer
         return $this->syncError;
     }
 
-    private function getTrackNumberFromInfo(array $info): int
-    {
-        $track = 0;
-
-        // Apparently track numbers can be stored with different indices as the following.
-        $trackIndices = [
-            'comments.track',
-            'comments.tracknumber',
-            'comments.track_number',
-        ];
-
-        for ($i = 0; $i < count($trackIndices) && $track === 0; ++$i) {
-            $track = (int) array_get($info, $trackIndices[$i], [0])[0];
-        }
-
-        return $track;
-    }
-
-    private function gatherPropsFromTags(array $info, array $comments, array &$props): void
-    {
-        $propertyMap = [
-            'artist' => 'artist',
-            'albumartist' => 'band',
-            'album' => 'album',
-            'title' => 'title',
-            'lyrics' => ['unsychronised_lyric', 'unsynchronised_lyric'],
-            'compilation' => 'part_of_a_compilation',
-        ];
-
-        foreach ($propertyMap as $name => $tags) {
-            foreach ((array) $tags as $tag) {
-                $value = array_get($info, "tags.id3v2.$tag", [null])[0] ?: array_get($comments, $tag, [''])[0];
-
-                if ($value) {
-                    $props[$name] = $value;
-                }
-            }
-
-            // Fixes #323, where tag names can be htmlentities()'ed
-            if (is_string($props[$name]) && $props[$name]) {
-                $props[$name] = trim(html_entity_decode($props[$name]));
-            }
-        }
-    }
-
-    private function isCompilation(array $props): bool
-    {
-        // A "compilation" property can be determined by:
-        // - "part_of_a_compilation" tag (used by iTunes), or
-        // - "albumartist" (used by non-retarded applications).
-        // Also, the latter is only valid if the value is NOT the same as "artist".
-        return $props['albumartist'] && $props['artist'] !== $props['albumartist'];
-    }
-
     public function getSong(): ?Song
     {
         return $this->song;

app/Services/MediaSyncService.php

@@ -16,23 +16,6 @@ use Symfony\Component\Finder\Finder;
 
 class MediaSyncService
 {
-    /**
-     * All applicable tags in a media file that we cater for.
-     * Note that each isn't necessarily a valid ID3 tag name.
-     */
-    public const APPLICABLE_TAGS = [
-        'artist',
-        'album',
-        'title',
-        'length',
-        'track',
-        'disc',
-        'lyrics',
-        'cover',
-        'mtime',
-        'compilation',
-    ];
-
     public function __construct(
         private SongRepository $songRepository,
         private FileSynchronizer $fileSynchronizer,
@@ -42,14 +25,7 @@ class MediaSyncService
     }
 
     /**
-     * Tags to be synced.
+     * @param array<string> $excludes The tags to exclude.
-     */
-    protected array $tags = [];
-
-    /**
-     * Sync the media. Oh sync the media.
-     *
-     * @param array<string> $tags The tags to sync.
      * Only taken into account for existing records.
      * New records will have all tags synced in regardless.
      * @param bool $force Whether to force syncing even unchanged files
@@ -57,23 +33,18 @@ class MediaSyncService
      */
     public function sync(
         ?string $mediaPath = null,
-        array $tags = [],
+        array $excludes = [],
         bool $force = false,
         ?SyncCommand $syncCommand = null
     ): void {
         $this->setSystemRequirements();
-        $this->setTags($tags);
 
         $syncResult = SyncResult::init();
-
         $songPaths = $this->gatherFiles($mediaPath ?: Setting::get('media_path'));
-
+        $syncCommand?->createProgressBar(count($songPaths));
-        if ($syncCommand) {
-            $syncCommand->createProgressBar(count($songPaths));
-        }
 
         foreach ($songPaths as $path) {
-            $result = $this->fileSynchronizer->setFile($path)->sync($this->tags, $force);
+            $result = $this->fileSynchronizer->setFile($path)->sync($excludes, $force);
 
             switch ($result) {
                 case FileSynchronizer::SYNC_RESULT_SUCCESS:
@@ -113,7 +84,7 @@ class MediaSyncService
         return iterator_to_array(
             $this->finder->create()
                 ->ignoreUnreadableDirs()
-                ->ignoreDotFiles((bool) config('koel.ignore_dot_files')) // https://github.com/phanan/koel/issues/450
+                ->ignoreDotFiles((bool) config('koel.ignore_dot_files')) // https://github.com/koel/koel/issues/450
                 ->files()
                 ->followLinks()
                 ->name('/\.(mp3|ogg|m4a|flac)$/i')
@@ -151,23 +122,6 @@ class MediaSyncService
         }
     }
 
-    /**
-     * Construct an array of tags to be synced into the database from an input array of tags.
-     * If the input array is empty or contains only invalid items, we use all tags.
-     * Otherwise, we only use the valid items in it.
-     *
-     * @param array<string> $tags
-     */
-    public function setTags(array $tags = []): void
-    {
-        $this->tags = array_intersect($tags, self::APPLICABLE_TAGS) ?: self::APPLICABLE_TAGS;
-
-        // We always keep track of mtime.
-        if (!in_array('mtime', $this->tags, true)) {
-            $this->tags[] = 'mtime';
-        }
-    }
-
     private function setSystemRequirements(): void
     {
         if (!app()->runningInConsole()) {
@@ -194,7 +148,7 @@ class MediaSyncService
 
     private function handleNewOrModifiedFileRecord(string $path): void
     {
-        $result = $this->fileSynchronizer->setFile($path)->sync($this->tags);
+        $result = $this->fileSynchronizer->setFile($path)->sync();
 
         if ($result === FileSynchronizer::SYNC_RESULT_SUCCESS) {
             $this->logger->info("Synchronized $path");
@@ -221,7 +175,7 @@ class MediaSyncService
     private function handleNewOrModifiedDirectoryRecord(string $path): void
     {
         foreach ($this->gatherFiles($path) as $file) {
-            $this->fileSynchronizer->setFile($file)->sync($this->tags);
+            $this->fileSynchronizer->setFile($file)->sync();
         }
 
         $this->logger->info("Synced all song(s) under $path");

app/Services/UploadService.php

@@ -24,8 +24,7 @@ class UploadService
         $file->move($this->getUploadDirectory(), $targetFileName);
 
         $targetPathName = $this->getUploadDirectory() . $targetFileName;
-        $this->fileSynchronizer->setFile($targetPathName);
+        $result = $this->fileSynchronizer->setFile($targetPathName)->sync();
-        $result = $this->fileSynchronizer->sync(MediaSyncService::APPLICABLE_TAGS);
 
         if ($result !== FileSynchronizer::SYNC_RESULT_SUCCESS) {
             @unlink($targetPathName);

app/Values/SongScanInformation.php

@@ -0,0 +1,114 @@
+<?php
+
+namespace App\Values;
+
+use App\Models\Album;
+use App\Models\Artist;
+use Illuminate\Contracts\Support\Arrayable;
+use Illuminate\Support\Arr;
+use SplFileInfo;
+use Throwable;
+
+final class SongScanInformation implements Arrayable
+{
+    private function __construct(
+        public ?string $title,
+        public ?string $albumName,
+        public ?string $artistName,
+        public ?string $albumArtistName,
+        public ?int $track,
+        public ?int $disc,
+        public ?string $lyrics,
+        public ?int $length,
+        public ?array $cover,
+        public ?string $path,
+        public ?int $mTime,
+    ) {
+    }
+
+    public static function fromGetId3Info(array $info): self
+    {
+        // We prefer ID3v2 tags over ID3v1 tags.
+        $tags = array_merge(Arr::get($info, 'tags.id3v1', []), Arr::get($info, 'tags.id3v2', []));
+        $comments = Arr::get($info, 'comments', []);
+
+        $title = self::getTag($tags, 'title', 'Untitled');
+        $albumName = self::getTag($tags, 'album', Album::UNKNOWN_NAME);
+        $artistName = self::getTag($tags, 'artist', Artist::UNKNOWN_NAME);
+        $albumArtistName = self::getTag($tags, ['albumartist', 'album_artist', 'band']);
+
+        // If the song is explicitly marked as a compilation but there's no album artist name, use the umbrella
+        // "Various Artists" artist.
+        if (self::getTag($tags, 'part_of_a_compilation') && !$albumArtistName) {
+            $albumArtistName = Artist::VARIOUS_NAME;
+        }
+
+        $track = (int) self::getTag($tags, ['track', 'tracknumber', 'track_number']);
+        $disc = (int) self::getTag($tags, 'part_of_a_set', 1);
+        $lyrics = self::getTag($tags, ['unsynchronised_lyric', 'unsychronised_lyric']);
+        $path = Arr::get($info, 'filenamepath');
+        $length = (float) Arr::get($info, 'playtime_seconds');
+        $cover = self::getTag($comments, 'picture', []);
+        $mTime = self::getMTime($path);
+
+        return new self(
+            title: $title,
+            albumName: $albumName,
+            artistName: $artistName,
+            albumArtistName: $albumArtistName,
+            track: $track,
+            disc: $disc,
+            lyrics: $lyrics,
+            length: $length,
+            cover: $cover,
+            path: $path,
+            mTime: $mTime,
+        );
+    }
+
+    private static function getTag(array $arr, string | array $keys, $default = ''): mixed
+    {
+        $keys = Arr::wrap($keys);
+
+        for ($i = 0; $i < count($keys); ++$i) {
+            $value = Arr::get($arr, $keys[$i] . '.0');
+
+            if ($value) {
+                break;
+            }
+        }
+
+        return $value ?? $default;
+    }
+
+    private static function getMTime(mixed $path): int
+    {
+        $splFileInfo = new SplFileInfo($path);
+
+        // Workaround for #344, where getMTime() fails for certain files with Unicode names on Windows.
+        try {
+            return $splFileInfo->getMTime();
+        } catch (Throwable) {
+            // Just use current stamp for mtime.
+            return time();
+        }
+    }
+
+    /** @return array<mixed> */
+    public function toArray(): array
+    {
+        return [
+            'title' => $this->title,
+            'album' => $this->albumName,
+            'artist' => $this->artistName,
+            'albumartist' => $this->albumArtistName,
+            'track' => $this->track,
+            'disc' => $this->disc,
+            'lyrics' => $this->lyrics,
+            'length' => $this->length,
+            'cover' => $this->cover,
+            'path' => $this->path,
+            'mtime' => $this->mTime,
+        ];
+    }
+}

app/Values/SyncResult.php

@@ -6,20 +6,8 @@ use Illuminate\Support\Collection;
 
 final class SyncResult
 {
-    /** @var Collection|array<string> */
+    private function __construct(public Collection $success, public Collection $bad, public Collection $unmodified)
-    public Collection $success;
-
-    /** @var Collection|array<string> */
-    public Collection $bad;
-
-    /** @var Collection|array<string> */
-    public Collection $unmodified;
-
-    private function __construct(Collection $success, Collection $bad, Collection $unmodified)
     {
-        $this->success = $success;
-        $this->bad = $bad;
-        $this->unmodified = $unmodified;
     }
 
     public static function init(): self

public/.gitignore

@@ -3,9 +3,9 @@ fonts
 
 # Ignore all (generated) images under img, but keep the folder structure
 img/*
-!img/albums
+!img/covers
-img/albums/*
+img/covers/*
-!img/albums/.gitkeep
+!img/covers/.gitkeep
 !img/artists
 img/artists/*
 !img/artists/.gitkeep

tests/Unit/Services/UploadServiceTest.php

@@ -7,7 +7,6 @@ use App\Exceptions\SongUploadFailedException;
 use App\Models\Setting;
 use App\Models\Song;
 use App\Services\FileSynchronizer;
-use App\Services\MediaSyncService;
 use App\Services\UploadService;
 use Illuminate\Http\UploadedFile;
 use Mockery;
@@ -56,7 +55,7 @@ class UploadServiceTest extends TestCase
         $this->fileSynchronizer
             ->shouldReceive('sync')
             ->once()
-            ->with(MediaSyncService::APPLICABLE_TAGS)
+            ->with()
             ->andReturn(FileSynchronizer::SYNC_RESULT_BAD_FILE);
 
         $this->fileSynchronizer
@@ -91,7 +90,7 @@ class UploadServiceTest extends TestCase
         $this->fileSynchronizer
             ->shouldReceive('sync')
             ->once()
-            ->with(MediaSyncService::APPLICABLE_TAGS)
+            ->with()
             ->andReturn(FileSynchronizer::SYNC_RESULT_SUCCESS);
 
         $song = new Song();