LibriVox / librivox-catalog

LibriVox catalog and reader workflow application
https://librivox.org
MIT License
36 stars 17 forks source link

Fix API giving uploader URLs instead of Archive #217

Closed redrun45 closed 3 months ago

redrun45 commented 3 months ago

Fixes #211. Calling /api/feed/audiobooks with extended=1 will now give the Arhive.org URLs for sections in completed projects, and empty strings for projects in progress. From the database, we're substituting mp3_64_url for the previously used listen_url. I opted to leave the API name the same, in case someone actually has been using it successfully.

Side note: I've added several COALESCE() calls, to get rid of some warnings. They might not do anything but clutter the logs in prod, but on localdev they get rendered as HTML, making the API more difficult to test.

notartom commented 3 months ago

I'm not convinced the use of COALESCE is correct here...

So, for context, the errors that you speak of are from passing a $value of NULL to https://github.com/LibriVox/librivox-catalog/blob/master/application/libraries/Format.php#L142. So the errors only show up when looking at the results in XML format - passing &format=json in the API URL makes them go away.

I feel like the better fix is to add an if ($value) { ... } else { $value = '' }; - it addresses the error closest to its source, and makes it easier to understand why it's being done. Using COALESCE in the SQL query is "spooky action at a distance".

Also, at least in my tests on staging, using COALESCE doesn't make all the errors disappear.

And finally, because COALESCE returns the first non-NULL element in the list, I think that the values that these COALESCE statements return aren't even guaranteed to come from the same section. All you need is a different number of NULLs in font of the first non-NULL, and suddenly you're pulling the URL from the 3rd section, the language from the 42nd, etc.

redrun45 commented 3 months ago

Alright! I'll give this one another stab. I saw COALESCE apparently used for this purpose, and figured we should probably handle any other 'nullable' values the same way.

Marking as [WIP] until this is addressed. Ran out of coding time for the evening. 😉

notartom commented 3 months ago

I was convinced isset() on a NULL value wouldn't work, and it doesn't, but isset() on a variable that is NULL works as expected. Gdi PHP:

php > isset(NULL);
PHP Fatal error:  Cannot use isset() on the result of an expression (you can use "null !== expression" instead) in php shell code on line 1
php > $foo = NULL;
php > if (isset($foo)) { print("set"); }
php > if (! isset($foo)) { print("not set"); }
not set
php > 
redrun45 commented 3 months ago

Ah, I see. So isset() is specifically for checking environment variables (does $x exist, and is it something other than null), not for checking values directly entered, as in that first interactive shell example. Learned something new!

notartom commented 3 months ago

Yeah - so anyways, apologies for the hasty review, and I'm not sure what happened to your branch, but please re-open and repropose :)