podlove / podlove-publisher

Podlove Podcast Publisher for WordPress
https://wordpress.org/plugins/podlove-podcasting-plugin-for-wordpress/
MIT License
296 stars 82 forks source link

Explict URL Support #1202

Closed douglaskastle closed 3 years ago

douglaskastle commented 3 years ago

In some cases it might be helpful to explictly set the url of the media file in the episode instead of just the slug.

This issue is a result of this conversation on the podlove forum:

https://community.podlove.org/t/multiple-hosting-sources/2218/4

And an initial working version can be found on this branch:

https://github.com/podonaut/podlove-publisher/tree/explicit_url

douglaskastle commented 3 years ago

Currently it works on webpages, rss feed and dashboard.

There is a weird issue when a full link is copied into the webpage form

https://www.buzzsprout.com/1008127/3515764-roy-and-hg.mp3

this is the link it tries to verify against:

https%3A//www.buzzsprout.com/1008127/3515764-roy-and-hg.mp3.mp3

Which fails.

However it does save correctly and then after that is verifies correctly

Investigating

eteubert commented 3 years ago

Looks interesting :) Some thoughts:

Make a dedicated function for the absolute url case to make the code more readable, maybe:

function is_absolute_url($slug) {
    return strpos($slug, '://') !== false;
}

Side benefit: if the logic turns out to be too naive it can be replaced in one place with parse_url.

There is a weird issue when a full link is copied into the webpage form

Some input fields have validation/sanitization and the slug field is not supposed to have a URL in it, so maybe that's the issue. See if it still happens when you remove the css classes from the input field (which enable the sanitization).

And this block confuses me to be honest 😊, maybe add a code comment?

            $slug = apply_filters('podlove_file_url_template', '%episode_slug%');
            if ($slug === '%episode_slug%') {
                $slug = $episode->slug;
            }
douglaskastle commented 3 years ago

Hi thanks for the reply. This code is working now (entering a full url into the form resolves correctly to a link that can both be saved and verified before saving), but not cleaned up and not ready for a merge, for the reasons above.

I think a function call is great, but it needs to be somewhere many files can call it, not just in lib/model/media_file.php i would need to reference it in lib/ajax/file_controller.php too.

I had to edit lib/ajax/file_controller.php, for the function simulate_temporary_episode_slug. I have edited that too, this is used (as you probably know) to catch the slug entry from a form, operate on it and provide it to something else, which is what then makes it available to be verifies as a valid link.

As I don't know all the places this code is being used, I have had to maintain the functionality it does in a straight slug case, and deviate only when it is a full path url. If there is nothing in the field it returns a string with the %episode_slug% in it. If other code in the plugin uses this fact then i have preserved this operation. However if this code is only used here and here alone. (which I am reviewing today, and it appears it is only used here) I would rewrite the interaction a little to be tidier and not as confusing as you have quite rightly stated above.

As to the slug sanitizing, I am not too sure if and why that would ever be done, do you have some use examples of where this was required? I am trying to test my code locally, but am unclear and all the corner cases that might have gotten one here.

douglaskastle commented 3 years ago

Ok Happy with the live website of the explicit URL for the pas week.

Below is the list of explicit URLs I used, (with one as a regular slug from buzzsprout)

image

Here is how they appear in the RSS feed, without tracking enabled (slug only one is 4th from the top):

<enclosure url="https://ipfspodcasting.net/ipfs/QmaQcrJoWNNtFZt8VydNfRzTiEjHDMTnrXY3sNfbfjKxru/8191163-medieval-comedy-and-bakhtin-s-carnival-theory.mp3" length="41586483" type="audio/mpeg"/>
<enclosure url="https://gateway.pinata.cloud/ipfs/Qme1kfAj57uK4MPBj7Acm1PRddUcoKof4SgVCakPDgZ4is" length="32541500" type="audio/mpeg"/>
<enclosure url="https://cloudflare-ipfs.com/ipfs/QmeiMaKzX4jKkunFnq2XhGQsrNH9moDYbzjR5oxe65VjiW" length="1" type="audio/mpeg"/>
<enclosure url="https://www.buzzsprout.com/1008127/3746501-joker-with-seizure-kaiser.mp3" length="18374927" type="audio/mpeg"/>
<enclosure url="https://www.buzzsprout.com/1008127/3515764-roy-and-hg-with-mark-williamson.mp3" length="19958898" type="audio/mpeg"/>
<enclosure url="https://cloudflare-ipfs.com/ipfs/QmRhsUm9yAxv7VYNXEFF3EwBSoCpoeDwyCoW18jcQNZtkE?filename=goc02.mp3" length="1" type="audio/mpeg"/>
<enclosure url="https://archive.org/download/gurucomedy/goc01.mp3" length="14625860" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/wp-content/uploads/audio/GOC-01-Episode-1.mp3" length="14625860" type="audio/mpeg"/>
<enclosure url="https://castopod.nomadtechies.com/media/podcasts/test2/episode-i.mp3" length="14693306" type="audio/mpeg"/>

And this is what they look like with tracking enabled:

<enclosure url="https://grinncomedy.com/podlove/file/1/s/feed/c/podcast/ipfspodcasting.net/ipfs/QmaQcrJoWNNtFZt8VydNfRzTiEjHDMTnrXY3sNfbfjKxru/8191163-medieval-comedy-and-bakhtin-s-carnival-theory.mp3" length="41586483" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/podlove/file/9/s/feed/c/podcast/gateway.pinata.cloud/ipfs/Qme1kfAj57uK4MPBj7Acm1PRddUcoKof4SgVCakPDgZ4is" length="32541500" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/podlove/file/8/s/feed/c/podcast/cloudflare-ipfs.com/ipfs/QmeiMaKzX4jKkunFnq2XhGQsrNH9moDYbzjR5oxe65VjiW" length="1" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/podlove/file/6/s/feed/c/podcast/3746501-joker-with-seizure-kaiser.mp3" length="18374927" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/podlove/file/5/s/feed/c/podcast/www.buzzsprout.com/1008127/3515764-roy-and-hg-with-mark-williamson.mp3" length="19958898" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/podlove/file/7/s/feed/c/podcast/cloudflare-ipfs.com/ipfs/QmRhsUm9yAxv7VYNXEFF3EwBSoCpoeDwyCoW18jcQNZtkE?filename=goc02.mp3" length="1" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/podlove/file/4/s/feed/c/podcast/archive.org/download/gurucomedy/goc01.mp3" length="14625860" type="audio/mpeg"/>
<enclosure url="https://grinncomedy.com/podlove/file/10/s/feed/c/podcast/grinncomedy.com/wp-content/uploads/audio/GOC-01-Episode-1.mp3" length="14625860" type="audio/mpeg"/>

Of note, it is possible to add a link that resolved to an mp3, but does not end in an mp3:

https://cloudflare-ipfs.com/ipfs/QmeiMaKzX4jKkunFnq2XhGQsrNH9moDYbzjR5oxe65VjiW for example, this does validate as a link, but it is not possible for podlove to detect the duration. In the case of these IPFS links it is possible to as an argument to the end of the link with an mp3 ending, and it does work:

https://cloudflare-ipfs.com/ipfs/QmRhsUm9yAxv7VYNXEFF3EwBSoCpoeDwyCoW18jcQNZtkE?filename=goc02.mp3 This seems to be a convention used over in IPFS land to help track what the files themselves actually are.

They all play correctly in the web player.

This version of the plugin can be seen live at this address at the time of the writing of this comment:

https://grinncomedy.com/feed/podcast/

douglaskastle commented 3 years ago

I am ready to merge these changes, what branch is the correct one to do merge requests against? masteror beta?

eteubert commented 3 years ago

beta is best. I'll probably have a close look at this after the next release 👍

douglaskastle commented 3 years ago

OK created a pull request #1207

douglaskastle commented 3 years ago

Closed that pull requests and opened an new one: #1211

douglaskastle commented 3 years ago

Closing issue as code was merged