Implement Last.fm scrobbling

[mattwellss]

Resolves #339

• When configured, sends scrobbles to Last.fm
• Adds new config.php keys music.scrobble_api_key, music.scrobble_api_secret, music.scrobble_api_service
• Updates settings UI to add buttons for creating a session
• Adds a new route for handling a token response callback from Last.fm
• Adds another new route for clearing an existing Last.fm session
• Updates the Track Business Layer to become an emitter (emits scrobble events)

Screenshots!

Settings update overview:

Authentication flow:

Screencast.From.2025-11-21.09-29-53.mp4

Not configured view:

[mattwellss]

The inspection completed: 11 new issues, 23 updated code elements

Plenty to review and fix here, I oughta have time this weekend.

In the static analysis, PHPStan calls out my annotation of the return type of curl_init (https://github.com/owncloud/music/actions/runs/19574013634/job/56054388897?pr=1266#step:7:10). In PHP8+ that function returns an instance of CurlHandle and I'm trying to account for that. Could add an "ignore" directive... curious about your thoughts!

[paulijar]

Thanks! I'll test this and have a closer look as soon as I have time.

[paulijar]

In the static analysis, PHPStan calls out my annotation of the return type of curl_init (https://github.com/owncloud/music/actions/runs/19574013634/job/56054388897?pr=1266#step:7:10). In PHP8+ that function returns an instance of CurlHandle and I'm trying to account for that. Could add an "ignore" directive... curious about your thoughts!

There's discussion phpstan/phpstan#4212 about this type of problems but it seems that there's no easy and clean way to handle this properly. Maybe the least bad option would be to turn part of that php-doc into ordinary comment not understood by PHPStan:

/**
 * @return false|resource Or on PHP8.0+, false|CurlHandle
 */

[mattwellss]

Some stuff I was considering that didn't make it in (but might be considered blockers)

• Storing scrobbles in a new DB table to be sent on a cron, decoupling the scrobble service's availability/performance from the scrobble call's performance
• Implementing more scrobble backends. I spent a bit of time on ListenBrainz (https://listenbrainz.readthedocs.io/en/latest/users/api-compat.html) but kept hitting 410 Gone responses. I think that's because of some broad IP blocking they do but I gave up on it pretty quick
• Building an admin-level UI for configuring API key/secret. Would allow encrypting the secret, which is currently impossible afaik

[paulijar]

I tested this feature yesterday, and it seemed to work well. It was not a small task so great job!

There are still some smallish details which I'd like to see improved. I'll add more comments about these once I have time to write the down, probably during the next few days.

[paulijar]

Overall, it's a very good PR, and with some improvements on the commented topics, it will be excellent.

[mattwellss]

Thanks for the feedback! Hoping to get the fixes done this weekend

[mattwellss]

😬 dipping my toe into dependency injection has really exposed my ignorance! I'll get these fixed

[paulijar]

From my point of view, this starts to be ready to be merged. Is there anything more you are planning to do before this should be merged?

[mattwellss]

From my point of view, this starts to be ready to be merged. Is there anything more you are planning to do before this should be merged?

I mentioned a few ideas at the start of the code review process (#1266 (comment)), but IMO those are best kept as improvements if we find they're needed/wanted. I've been exploring libre.fm, which seems to have a handcrafted API application approval process. If that all works out of the box as a new instance of the ExternalScrobbler, it'll be a very simple enhancement.

I've got nothing else planned for the PR.

The Last.fm and Admin settings wiki pages will need updating. I'd like to help with that as well, however I can.

[paulijar]

I agree that things in #1266 (comment) can be handled as separate PRs if ever. Regarding the possibility to encrypt the API secret, I don't see that this would actually improve the security in any meaningful way. The fact is that even if the secret was encrypted on the server, the key to open the encrypted secret should still be available somewhere on the server. Like the secret used to encrypt and decrypt the session key is right there in config.php. It's anyway "game over" if a malicious actor has got a full read access to the files of your server. Hence, the secret can as well be in plain text on the server.

Most of the issues reported by Scrutinizer could still be addressed. The one about CurlHandle we can ignore for now. But for example for the return values of curl_exec, we know for fact that the way it is used, it cannot return true. We could hint that to Scrutinizer for example by writing

$response = \curl_exec($ch);
\assert(\is_string($response));
$xml = \simplexml_load_string($response);

On the other hand, I think curl_exec could return false on error although that is not reported by Scrutinizer. So maybe we need here an actual error handling instead of just the assert.

[mattwellss]

Most of the issues reported by Scrutinizer could still be addressed. The one about CurlHandle we can ignore for now. But for example for the return values of curl_exec, we know for fact that the way it is used, it cannot return true. We could hint that to Scrutinizer for example by writing

Appreciate the push. I realized there's no reason to expose the use of curl outside the function which uses it, meaning the type change is no longer relevant. That change plus your suggestion of using assert cleaned up the curl-related issues.

At this point, the curl calling functionality of ExternalScrobbler can be extracted into a separate class. Doing so would unlock unit testing, which wasn't possible before.

Scrutinizer is now complaining about $xml ?: null (ensure a nullable SimpleXMLElement return type for execRequest). PHP's docs say simplexml_load_string can return false, but Scrutinizer doesn't recognize that. 🤷

Final inspection note: Scrutinizer doesn't like my use of catch (\TypeError when user id or token is missing (https://scrutinizer-ci.com/g/owncloud/music/inspections/ab5b74ff-f2c7-4a85-b70d-c5295c572578/issues/files/lib/Controller/ScrobblerController.php?status=new&orderField=path&order=asc&honorSelectedPaths=0). This approach definitely works (https://3v4l.org/uSJIX#v7.4.32), but I guess it's a little outside the norm.

[paulijar]

I realized there's no reason to expose the use of curl outside the function which uses it, meaning the type change is no longer relevant. That change plus your suggestion of using assert cleaned up the curl-related issues.

Definitely much better now. But now, if curl_exec would return false, the execution would be terminated instead of returning a null or throwing like in other error cases. So maybe we should instead write it like this:

	$xmlString = \curl_exec($ch);
	$xml = \is_string($xmlString) ? \simplexml_load_string($xmlString) : null;
	return $xml ?: null;

Scrutinizer is now complaining about $xml ?: null (ensure a nullable SimpleXMLElement return type for execRequest). PHP's docs say simplexml_load_string can return false, but Scrutinizer doesn't recognize that.

Maybe it would help Scrutinizer if an explicit phpdoc type-hint was added /** @var SimpleXMLElement|false $xml */. But if not, then we will just live with it.

Final inspection note: Scrutinizer doesn't like my use of catch (\TypeError when user id or token is missing (https://scrutinizer-ci.com/g/owncloud/music/inspections/ab5b74ff-f2c7-4a85-b70d-c5295c572578/issues/files/lib/Controller/ScrobblerController.php?status=new&orderField=path&order=asc&honorSelectedPaths=0). This approach definitely works (https://3v4l.org/uSJIX#v7.4.32), but I guess it's a little outside the norm.

I have been struggling myself with how the type checking for URL arguments should be made in the Controller interface and I still haven't find a solution I would always be happy to apply. For one thing, often passing the arguments is mandatory for the meaningful operation of the REST endpoint. But if we mark the parameters as non-nullable, then trying to call the endpoint without proper arguments is responded with "500 Internal server error" which isn't very nice. It probably also gets logged on the server as an error. Something like "400 Bad request" would be must more correct, but to achieve that, we need to mark the parameters as nullable and then null-check manually and return the error response. That's a bit tedious if done extensively on all API endpoints, but maybe that would still be the way to go.

In case of ScrobblerController, also the argument $serviceIdentifier could be technically omitted and probably should be nullable in the function signature.

Another similar problem is the dependency injected $userId. It should be non-null always when there is a logged-in user. But sometimes the Controller may get instantiated in a situation where the user session has already been terminated. In that case, there would be an ugly error logged if the $userId was not nullable. I think that the actual endpoint methods should not get called in situations like this.

[mattwellss]

I believe I've resolved the issues you highlighted. Marking $serviceIdentifier as nullable and providing a default allows the extant error handling code to highlight the missing data rather than rendering a 500 error page.

Regarding controller parameters, maybe in the future annotations are a good option for expressing how a missing or invalid parameter can be handled by the dispatcher. Here's how Symfony does it https://symfony.com/doc/current/controller.html#automatic-mapping-of-the-request

edit: OK, Scrutinizer still has an issue with the return type of simplexml_load_string. I don't really get it, even back in php5 it'd return false on failure. It's also still bringing up an issue with $token's use in handleToken (potentially passing null to a string-only argument). In my opinion that's handled well enough for a situation that can only be encountered through misuse. Here's a screenshot of that situation, let me know if it needs improvement

[paulijar]

OK, Scrutinizer still has an issue with the return type of simplexml_load_string

Might be a bug in Scrutinizer and apparently this can't even be suppressed. Let's just forget it.

It's also still bringing up an issue with $token's use in handleToken

In my opinion, it would be cleaner to have a null-check block for this than rely on the TypeError and it would also make Scrutinizer happy. And it wouldn't really need almost any more code.

Also the possible null $this->userId is still nagged on by Scrutinizer. This could be null-checked with a separate block, or just silenced with the null coalescing: $scrobbler->generateSession($token, $this->userId ?? '');.

In the latter case, if the code ever got executed with the null $userId (which is doubtful), the result would probably be InvalidArgumentException which is already caught.

[mattwellss]

In my opinion, it would be cleaner to have a null-check block for this than rely on the TypeError and it would also make Scrutinizer happy. And it wouldn't really need almost any more code.

Gotcha, I'll update the controller to proactively handle invalid $token values.

Also the possible null $this->userId is still nagged on by Scrutinizer. This could be null-checked with a separate block, or just silenced with the null coalescing: $scrobbler->generateSession($token, $this->userId ?? '');.

In the latter case, if the code ever got executed with the null $userId (which is doubtful), the result would probably be InvalidArgumentException which is already caught.

This might not change your recommendation, but I checked this and forgot to report back. When not logged in, all controller actions in NextCloud get routed to the login page with a redirect_url param. No clue what OwnCloud does, though, so it's probably better to take steps to avoid potentially logging non-useful errors.

[scrutinizer-notifier]

The inspection completed: 2 new issues, 26 updated code elements

[paulijar]

@mattwellss As you probably noticed, I rebased and merged the PR yesterday. Thanks again for your excellent work! It was a long haul. But that's nothing compared to the decade which the feature request sat on the back-log waiting for someone to pick it up ;).

Btw, I did the small scrobbling-related updates to the wiki pages already on Sunday.

I'm somewhat busy until Christmas but I hope I can get the Music v2.5.0 out still this year, including also this feature.

[paulijar]

Music v2.5.0 including this feature is now released.