Massive refactorings

app/Console/Commands/SyncMedia.php

@@ -3,6 +3,7 @@
 namespace App\Console\Commands;
 
 use App\Libraries\WatchRecord\InotifyWatchRecord;
+use App\Models\File;
 use App\Models\Setting;
 use Illuminate\Console\Command;
 use Media;
@@ -102,20 +103,20 @@ class SyncMedia extends Command
      * Log a song's sync status to console.
      *
      * @param string $path
-     * @param mixed  $result
+     * @param int $result
      * @param string $reason
      */
     public function logToConsole($path, $result, $reason = '')
     {
         $name = basename($path);
 
-        if ($result === true) {
+        if ($result === File::SYNC_RESULT_UNMODIFIED) {
             if ($this->option('verbose')) {
                 $this->line("$name has no changes – ignoring");
             }
 
             ++$this->ignored;
-        } elseif ($result === false) {
+        } elseif ($result === File::SYNC_RESULT_BAD_FILE) {
             if ($this->option('verbose')) {
                 $this->error("$name is not a valid media file because: ".$reason);
             }

app/Listeners/LoveTrackOnLastfm.php

@@ -33,7 +33,7 @@ class LoveTrackOnLastfm
     {
         if (!$this->lastfm->enabled() ||
             !($sessionKey = $event->user->lastfm_session_key) ||
-            $event->interaction->song->album->artist->isUnknown()
+            $event->interaction->song->album->artist->is_unknown
         ) {
             return;
         }

app/Listeners/UpdateLastfmNowPlaying.php

@@ -34,7 +34,7 @@ class UpdateLastfmNowPlaying
     {
         if (!$this->lastfm->enabled() ||
             !($sessionKey = $event->user->lastfm_session_key) ||
-            $event->song->artist->isUnknown()
+            $event->song->artist->is_unknown
         ) {
             return;
         }

app/Models/Album.php

@@ -16,6 +16,7 @@ use Illuminate\Database\Eloquent\Model;
  * @property Artist artist          The album's artist
  * @property int    artist_id
  * @property Collection  songs
+ * @property bool   is_unknown
  */
 class Album extends Model
 {
@@ -40,7 +41,7 @@ class Album extends Model
         return $this->hasMany(Song::class);
     }
 
-    public function isUnknown()
+    public function getIsUnknownAttribute()
     {
         return $this->id === self::UNKNOWN_ID;
     }
@@ -58,7 +59,7 @@ class Album extends Model
     {
         // If this is a compilation album, its artist must be "Various Artists"
         if ($isCompilation) {
-            $artist = Artist::getVarious();
+            $artist = Artist::getVariousArtist();
         }
 
         return self::firstOrCreate([
@@ -74,18 +75,16 @@ class Album extends Model
      */
     public function getInfo()
     {
-        if ($this->isUnknown()) {
+        if ($this->is_unknown) {
             return false;
         }
 
         $info = Lastfm::getAlbumInfo($this->name, $this->artist->name);
+        $image = array_get($info, 'image');
 
         // If our current album has no cover, and Last.fm has one, why don't we steal it?
         // Great artists steal for their great albums!
-        if (!$this->has_cover &&
+        if (!$this->has_cover && is_string($image) && ini_get('allow_url_fopen')) {
-            is_string($image = array_get($info, 'image')) &&
-            ini_get('allow_url_fopen')
-        ) {
             $extension = explode('.', $image);
             $this->writeCoverFile(file_get_contents($image), last($extension));
             $info['cover'] = $this->cover;
@@ -127,24 +126,24 @@ class Album extends Model
     public function writeCoverFile($binaryData, $extension)
     {
         $extension = trim(strtolower($extension), '. ');
-        $destPath = $this->generateRandomCoverPath($extension);
+        $destination = $this->generateRandomCoverPath($extension);
-        file_put_contents($destPath, $binaryData);
+        file_put_contents($destination, $binaryData);
 
-        $this->update(['cover' => basename($destPath)]);
+        $this->update(['cover' => basename($destination)]);
     }
 
     /**
      * Copy a cover file from an existing image on the system.
      *
-     * @param string $srcPath The original image's full path.
+     * @param string $source The original image's full path.
      */
-    public function copyCoverFile($srcPath)
+    public function copyCoverFile($source)
     {
-        $extension = pathinfo($srcPath, PATHINFO_EXTENSION);
+        $extension = pathinfo($source, PATHINFO_EXTENSION);
-        $destPath = $this->generateRandomCoverPath($extension);
+        $destination = $this->generateRandomCoverPath($extension);
-        copy($srcPath, $destPath);
+        copy($source, $destination);
 
-        $this->update(['cover' => basename($destPath)]);
+        $this->update(['cover' => basename($destination)]);
     }
 
     /**

app/Models/Artist.php

@@ -13,6 +13,8 @@ use Log;
  * @property int    id      The model ID
  * @property string name    The artist name
  * @property string image
+ * @property bool   is_unknown
+ * @property bool   is_various
  */
 class Artist extends Model
 {
@@ -37,12 +39,12 @@ class Artist extends Model
         return $this->hasManyThrough(Song::class, Album::class);
     }
 
-    public function isUnknown()
+    public function getIsUnknownAttribute()
     {
         return $this->id === self::UNKNOWN_ID;
     }
 
-    public function isVarious()
+    public function getVariousAttribute()
     {
         return $this->id === self::VARIOUS_ID;
     }
@@ -52,7 +54,7 @@ class Artist extends Model
      *
      * @return Artist
      */
-    public static function getVarious()
+    public static function getVariousArtist()
     {
         return self::find(self::VARIOUS_ID);
     }
@@ -97,17 +99,15 @@ class Artist extends Model
      */
     public function getInfo()
     {
-        if ($this->isUnknown()) {
+        if ($this->is_unknown) {
             return false;
         }
 
         $info = Lastfm::getArtistInfo($this->name);
+        $image = array_get($info, 'image');
 
         // If our current artist has no image, and Last.fm has one, copy the image for our local use.
-        if (!$this->image &&
+        if (!$this->image && is_string($image) && ini_get('allow_url_fopen')) {
-            is_string($image = array_get($info, 'image')) &&
-            ini_get('allow_url_fopen')
-        ) {
             try {
                 $extension = explode('.', $image);
                 $fileName = uniqid().'.'.trim(strtolower(last($extension)), '. ');

app/Models/File.php

@@ -62,6 +62,10 @@ class File
      */
     protected $syncError;
 
+    const SYNC_RESULT_SUCCESS = 1;
+    const SYNC_RESULT_BAD_FILE = 2;
+    const SYNC_RESULT_UNMODIFIED = 3;
+
     /**
      * Construct our File object.
      * Upon construction, we'll set the path, hash, and associated Song object (if any).
@@ -102,7 +106,7 @@ class File
         if (isset($info['error']) || !isset($info['playtime_seconds'])) {
             $this->syncError = isset($info['error']) ? $info['error'][0] : 'No playtime found';
 
-            return;
+            return [];
         }
 
         // Copy the available tags over to comment.
@@ -193,12 +197,12 @@ class File
     {
         // If the file is not new or changed and we're not forcing update, don't do anything.
         if (!$this->isNewOrChanged() && !$force) {
-            return true;
+            return self::SYNC_RESULT_UNMODIFIED;
         }
 
         // If the file is invalid, don't do anything.
         if (!$info = $this->getInfo()) {
-            return false;
+            return self::SYNC_RESULT_BAD_FILE;
         }
 
         // Fixes #366. If the file is new, we use all tags by simply setting $force to false.
@@ -246,28 +250,41 @@ class File
             $album = Album::get($artist, $info['album'], $isCompilation);
         }
 
-        if (!$album->has_cover) {
+        $album->has_cover || $this->generateAlbumCover($album, array_get($info, 'cover'));
-            // If the album has no cover, we try to get the cover image from existing tag data
-            if (!empty($info['cover'])) {
-                try {
-                    $album->generateCover($info['cover']);
-                } catch (Exception $e) {
-                    Log::error($e);
-                }
-            }
-            // or, if there's a cover image under the same directory, use it.
-            elseif ($cover = $this->getCoverFileUnderSameDirectory()) {
-                $album->copyCoverFile($cover);
-            }
-        }
 
         $info['album_id'] = $album->id;
         $info['artist_id'] = $artist->id;
 
         // Remove these values from the info array, so that we can just use the array as model's input data.
         array_forget($info, ['artist', 'albumartist', 'album', 'cover', 'compilation']);
+        $this->song = Song::updateOrCreate(['id' => $this->hash], $info);
 
-        return Song::updateOrCreate(['id' => $this->hash], $info);
+        return self::SYNC_RESULT_SUCCESS;
+    }
+
+    /**
+     * Try to generate a cover for an album based on extracted data, or use the cover file under the directory.
+     *
+     * @param Album $album
+     * @param $coverData
+     */
+    private function generateAlbumCover(Album $album, $coverData)
+    {
+        // If the album has no cover, we try to get the cover image from existing tag data
+        if ($coverData) {
+            try {
+                $album->generateCover($coverData);
+
+                return;
+            } catch (Exception $e) {
+                Log::error($e);
+            }
+        }
+
+        // Or, if there's a cover image under the same directory, use it.
+        if ($cover = $this->getCoverFileUnderSameDirectory()) {
+            $album->copyCoverFile($cover);
+        }
     }
 
     /**

app/Models/Interaction.php

@@ -45,19 +45,17 @@ class Interaction extends Model
      */
     public static function increasePlayCount($songId, User $user)
     {
-        $interaction = self::firstOrCreate([
+        return tap(self::firstOrCreate([
             'song_id' => $songId,
             'user_id' => $user->id,
-        ]);
+        ]), function (Interaction $interaction) {
-
             if (!$interaction->exists) {
                 $interaction->liked = false;
             }
 
             ++$interaction->play_count;
             $interaction->save();
-
+        });
-        return $interaction;
     }
 
     /**
@@ -70,21 +68,15 @@ class Interaction extends Model
      */
     public static function toggleLike($songId, User $user)
     {
-        $interaction = self::firstOrCreate([
+        return tap(self::firstOrCreate([
             'song_id' => $songId,
             'user_id' => $user->id,
-        ]);
+        ]), function (Interaction $interaction) {
-
-        if (!$interaction->exists) {
-            $interaction->play_count = 0;
-        }
-
             $interaction->liked = !$interaction->liked;
             $interaction->save();
 
             event(new SongLikeToggled($interaction));
-
+        });
-        return $interaction;
     }
 
     /**
@@ -97,14 +89,11 @@ class Interaction extends Model
      */
     public static function batchLike(array $songIds, User $user)
     {
-        $result = [];
+        return collect($songIds)->map(function ($songId) use ($user) {
-
+            return tap(self::firstOrCreate([
-        foreach ($songIds as $songId) {
-            $interaction = self::firstOrCreate([
                 'song_id' => $songId,
                 'user_id' => $user->id,
-            ]);
+            ]), function (Interaction $interaction) {
-
                 if (!$interaction->exists) {
                     $interaction->play_count = 0;
                 }
@@ -113,11 +102,8 @@ class Interaction extends Model
                 $interaction->save();
 
                 event(new SongLikeToggled($interaction));
-
+            });
-            $result[] = $interaction;
+        })->all();
-        }
-
-        return $result;
     }
 
     /**
@@ -130,7 +116,7 @@ class Interaction extends Model
      */
     public static function batchUnlike(array $songIds, User $user)
     {
-        self::whereIn('song_id', $songIds)->whereUserId($user->id)->get()->each(function ($interaction) {
+        self::whereIn('song_id', $songIds)->whereUserId($user->id)->get()->each(function (Interaction $interaction) {
             $interaction->liked = false;
             $interaction->save();
 

app/Models/Song.php

@@ -22,6 +22,7 @@ use YouTube;
  * @property int    track
  * @property int    album_id
  * @property int    id
+ * @property int    artist_id
  */
 class Song extends Model
 {
@@ -80,7 +81,7 @@ class Song extends Model
     public function scrobble(User $user, $timestamp)
     {
         // Don't scrobble the unknown guys. No one knows them.
-        if ($this->artist->isUnknown()) {
+        if ($this->artist->is_unknown) {
             return false;
         }
 

app/Models/User.php

@@ -152,15 +152,13 @@ class User extends Authenticatable
      */
     public function getPreferencesAttribute($value)
     {
-        $preferences = unserialize($value) ?: [];
+        return tap(unserialize($value) ?: [], function (array $preferences) {
-
             // Hide the user's secrets away!
             foreach ($this->hiddenPreferences as $key) {
                 if (array_key_exists($key, $preferences)) {
                     $preferences[$key] = 'hidden';
                 }
             }
-
+        });
-        return $preferences;
     }
 }

app/Services/Download.php

@@ -120,9 +120,9 @@ class Download
             // 'duplicated-name.mp3' => currentFileIndex
         ];
 
-        $songs->each(function ($s) use ($zip, &$localNames) {
+        $songs->each(function ($song) use ($zip, &$localNames) {
             try {
-                $path = $this->fromSong($s);
+                $path = $this->fromSong($song);
 
                 // We add all files into the zip archive as a flat structure.
                 // As a result, there can be duplicate file names.

app/Services/Lastfm.php

@@ -92,14 +92,7 @@ class Lastfm extends RESTfulService
                 return false;
             }
 
-            return [
+            return $this->buildArtistInfo($artist);
-                'url' => array_get($artist, 'url'),
-                'image' => count($artist['image']) > 3 ? $artist['image'][3] : $artist['image'][0],
-                'bio' => [
-                    'summary' => $this->formatText(array_get($artist, 'bio.summary')),
-                    'full' => $this->formatText(array_get($artist, 'bio.content')),
-                ],
-            ];
         } catch (Exception $e) {
             Log::error($e);
 
@@ -107,6 +100,25 @@ class Lastfm extends RESTfulService
         }
     }
 
+    /**
+     * Build a Koel-usable array of artist information using the data from Last.fm.
+     *
+     * @param array $lastfmArtist
+     *
+     * @return array
+     */
+    private function buildArtistInfo(array $lastfmArtist)
+    {
+        return [
+            'url' => array_get($lastfmArtist, 'url'),
+            'image' => count($lastfmArtist['image']) > 3 ? $lastfmArtist['image'][3] : $lastfmArtist['image'][0],
+            'bio' => [
+                'summary' => $this->formatText(array_get($lastfmArtist, 'bio.summary')),
+                'full' => $this->formatText(array_get($lastfmArtist, 'bio.content')),
+            ],
+        ];
+    }
+
     /**
      * Get information about an album.
      *
@@ -141,12 +153,29 @@ class Lastfm extends RESTfulService
                 return false;
             }
 
+            return $this->buildAlbumInfo($album);
+        } catch (Exception $e) {
+            Log::error($e);
+
+            return false;
+        }
+    }
+
+    /**
+     * Build a Koel-usable array of album information using the data from Last.fm.
+     *
+     * @param array $lastfmAlbum
+     *
+     * @return array
+     */
+    private function buildAlbumInfo(array $lastfmAlbum)
+    {
         return [
-                'url' => array_get($album, 'url'),
+            'url' => array_get($lastfmAlbum, 'url'),
-                'image' => count($album['image']) > 3 ? $album['image'][3] : $album['image'][0],
+            'image' => count($lastfmAlbum['image']) > 3 ? $lastfmAlbum['image'][3] : $lastfmAlbum['image'][0],
             'wiki' => [
-                    'summary' => $this->formatText(array_get($album, 'wiki.summary')),
+                'summary' => $this->formatText(array_get($lastfmAlbum, 'wiki.summary')),
-                    'full' => $this->formatText(array_get($album, 'wiki.content')),
+                'full' => $this->formatText(array_get($lastfmAlbum, 'wiki.content')),
             ],
             'tracks' => array_map(function ($track) {
                 return [
@@ -154,13 +183,8 @@ class Lastfm extends RESTfulService
                     'length' => (int) $track['duration'],
                     'url' => $track['url'],
                 ];
-                }, array_get($album, 'tracks.track', [])),
+            }, array_get($lastfmAlbum, 'tracks.track', [])),
         ];
-        } catch (Exception $e) {
-            Log::error($e);
-
-            return false;
-        }
     }
 
     /**
@@ -180,9 +204,7 @@ class Lastfm extends RESTfulService
         ], true);
 
         try {
-            $response = $this->get("/?$query", [], false);
+            return (string) $this->get("/?$query", [], false)->session->key;
-
-            return (string) $response->session->key;
         } catch (Exception $e) {
             Log::error($e);
 
@@ -289,19 +311,15 @@ class Lastfm extends RESTfulService
     public function buildAuthCallParams(array $params, $toString = false)
     {
         $params['api_key'] = $this->getKey();
-
         ksort($params);
 
         // Generate the API signature.
         // @link http://www.last.fm/api/webauth#6
         $str = '';
-
         foreach ($params as $name => $value) {
             $str .= $name.$value;
         }
-
         $str .= $this->getSecret();
-
         $params['api_sig'] = md5($str);
 
         if (!$toString) {

app/Services/Media.php

@@ -44,59 +44,57 @@ class Media
     /**
      * Sync the media. Oh sync the media.
      *
-     * @param string|null $path
+     * @param string|null $mediaPath
      * @param array       $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
      * @param SyncMedia   $syncCommand The SyncMedia command object, to log to console if executed by artisan.
      */
-    public function sync($path = null, $tags = [], $force = false, SyncMedia $syncCommand = null)
+    public function sync($mediaPath = null, $tags = [], $force = false, SyncMedia $syncCommand = null)
     {
         if (!app()->runningInConsole()) {
             set_time_limit(config('koel.sync.timeout'));
         }
 
-        $path = $path ?: Setting::get('media_path');
+        $mediaPath = $mediaPath ?: Setting::get('media_path');
         $this->setTags($tags);
 
         $results = [
-            'good' => [], // Updated or added files
+            'success' => [],
-            'bad' => [], // Bad files
+            'bad_files' => [],
-            'ugly' => [], // Unmodified files
+            'unmodified' => [],
         ];
 
         $getID3 = new getID3();
+        $songPaths = $this->gatherFiles($mediaPath);
+        $syncCommand && $syncCommand->createProgressBar(count($songPaths));
 
-        $files = $this->gatherFiles($path);
+        foreach ($songPaths as $path) {
+            $file = new File($path, $getID3);
 
-        if ($syncCommand) {
+            switch ($result = $file->sync($this->tags, $force)) {
-            $syncCommand->createProgressBar(count($files));
+                case File::SYNC_RESULT_SUCCESS:
-        }
+                    $results['success'][] = $file;
-
+                    break;
-        foreach ($files as $file) {
+                case File::SYNC_RESULT_UNMODIFIED:
-            $file = new File($file, $getID3);
+                    $results['unmodified'][] = $file;
-
+                    break;
-            $song = $file->sync($this->tags, $force);
+                default:
-
+                    $results['bad_files'][] = $file;
-            if ($song === true) {
+                    break;
-                $results['ugly'][] = $file;
-            } elseif ($song === false) {
-                $results['bad'][] = $file;
-            } else {
-                $results['good'][] = $file;
             }
 
             if ($syncCommand) {
                 $syncCommand->updateProgressBar();
-                $syncCommand->logToConsole($file->getPath(), $song, $file->getSyncError());
+                $syncCommand->logToConsole($file->getPath(), $result, $file->getSyncError());
             }
         }
 
         // Delete non-existing songs.
-        $hashes = array_map(function ($f) {
+        $hashes = array_map(function (File $file) {
-            return self::getHash($f->getPath());
+            return self::getHash($file->getPath());
-        }, array_merge($results['ugly'], $results['good']));
+        }, array_merge($results['unmodified'], $results['success']));
 
         Song::deleteWhereIDsNotIn($hashes);
 
@@ -132,9 +130,17 @@ class Media
     public function syncByWatchRecord(WatchRecordInterface $record)
     {
         Log::info("New watch record received: '$record'");
-        $path = $record->getPath();
+        $record->isFile() ? $this->syncFileRecord($record) : $this->syncDirectoryRecord($record);
+    }
 
-        if ($record->isFile()) {
+    /**
+     * Sync a file's watch record.
+     *
+     * @param WatchRecordInterface $record
+     */
+    private function syncFileRecord(WatchRecordInterface $record)
+    {
+        $path = $record->getPath();
         Log::info("'$path' is a file.");
 
         // If the file has been deleted...
@@ -142,7 +148,6 @@ class Media
             // ...and it has a record in our database, remove it.
             if ($song = Song::byPath($path)) {
                 $song->delete();
-
                 Log::info("$path deleted.");
 
                 event(new LibraryChanged());
@@ -154,22 +159,27 @@ class Media
         // File format etc. will be handled by File::sync().
         elseif ($record->isNewOrModified()) {
             $result = (new File($path))->sync($this->tags);
-
+            Log::info($result === File::SYNC_RESULT_SUCCESS ? "Synchronized $path" : "Invalid file $path");
-                Log::info($result instanceof Song ? "Synchronized $path" : "Invalid file $path");
 
             event(new LibraryChanged());
         }
-
-            return;
     }
 
-        // Record is a directory.
+    /**
+     * Sync a directory's watch record.
+     *
+     * @param WatchRecordInterface $record
+     */
+    private function syncDirectoryRecord(WatchRecordInterface $record)
+    {
+        $path = $record->getPath();
         Log::info("'$path' is a directory.");
 
         if ($record->isDeleted()) {
             // The directory is removed. We remove all songs in it.
             if ($count = Song::inDirectory($path)->delete()) {
                 Log::info("Deleted $count song(s) under $path");
+
                 event(new LibraryChanged());
             } else {
                 Log::info("$path is empty--no action needed.");
@@ -178,10 +188,9 @@ class Media
             foreach ($this->gatherFiles($path) as $file) {
                 (new File($file))->sync($this->tags);
             }
+            Log::info("Synced all song(s) under $path");
 
             event(new LibraryChanged());
-
-            Log::info("Synced all song(s) under $path");
         }
     }
 

app/Services/RESTfulService.php

@@ -10,10 +10,10 @@ use InvalidArgumentException;
  * Class RESTfulService.
  *
  * @method object get($uri)
- * @method object post($uri, array $data = [])
+ * @method object post($uri, ...$data)
- * @method object put($uri, array $data = [])
+ * @method object put($uri, ...$data)
- * @method object patch($uri, array $data = [])
+ * @method object patch($uri, ...$data)
- * @method object head($uri, array $data = [])
+ * @method object head($uri, ...$data)
  * @method object delete($uri)
  */
 class RESTfulService

app/Services/YouTube.php

@@ -46,8 +46,8 @@ class YouTube extends RESTfulService
         $q = $song->title;
 
         // If the artist is worth noticing, include them into the search.
-        if (!$song->artist->isUnknown() && !$song->artist->isVarious()) {
+        if (!$song->artist->is_unknown && !$song->artist->is_various) {
-            $q .= ' '.$song->artist->name;
+            $q .= " {$song->artist->name}";
         }
 
         return $this->search($q, $pageToken);

tests/Feature/SongTest.php

@@ -201,6 +201,7 @@ class SongTest extends BrowserKitTestCase
                     'compilationState' => 2,
                 ],
             ], $user)
+//            ->seeText('jaja')
             ->seeStatusCode(200);
 
         $compilationAlbum = Album::whereArtistIdAndName(Artist::VARIOUS_ID, 'Two by Two')->first();