-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds transcript block and meta #221
Conversation
Hi @peterwilsoncc ! This is now ready for review. There is much more discussion on the approach to this in the original issue thread: #28 I added two unit tests, but my functions do not have full coverage. I'm happy to add more if given a bit of direction. I did not add any new E2E tests. Again, happy to add those in if you can tell me what coverage you'd like to see. When building the new transcript block, I tried to write the block using a more updated approach to block-building without changing the original block methodology (except when I did make an addition to those files, the prettier config made some formatting edits). One thing to note is that this update will require flushing rewrite rules. How do you usually like to approach that? Upgrade instructions? Upgrade routine? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few notes inline.
I'm not sure it is possible to flush the rewrite rules automatically upon updating a plugin. According to the docs, the upgrader_process_complete
hook runs using the old version of the plugin, not the new.
It's a little ugly but you might be able to use an option simple_podcasting_db_version
and flush rewrite rules if it's not set or less than two. If this is the approach taken, I'd recommend limiting the check so it only happens within the dashboard. The rules don't need to be flushed prior to someone visiting the admin and creating/editing a new podcast.
Other people may have better suggestions though, maybe ask around a bit wider.
$podcast_slug = get_query_var( 'podcasting-episode' ); | ||
$post_object = get_page_by_path( $podcast_slug, OBJECT, 'post' ); | ||
if ( $post_object instanceof WP_Post ) { | ||
echo wp_kses_post( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can run the transcript through kses on save rather than render, and then treat it as trusted data on output. This is what WP Core does for post content, etc as kses is a relatively expensive function to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc I've addressed this feedback in this commit: 9264fa4. Thanks
@@ -0,0 +1,31 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to use the post.php
template for the transcript when displaying it via a link. I don't really understand the comment Intentionally barebones with the minimum html for use by tools
, why have you done that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc The intention here was to follow a spec defined by Podcastindex.org. Intentionally barebones because there may be players that are able to read the HTML. Using the post.php
template makes sense for humans reading this, should there be two versions? Original comment here: #28 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention here was to follow a spec defined by Podcastindex.org
@nateconley Understood and makes sense.
Do you think it's worth noindexing the transcript page so it doesn't end up in search engines? To allow developers to filter the results for their site, the code could be:
if ( function_exists( 'wp_robots' ) && function_exists( 'wp_robots_no_robots' ) && function_exists( 'add_filter' ) ) {
add_filter( 'wp_robots', 'wp_robots_no_robots' );
wp_robots();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great suggestion. This has been added.
templates/transcript.php
Outdated
<meta charset="UTF-8"> | ||
<meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
<title><?php esc_html_e( 'Transcript', 'simple-podcasting' ); ?> - <?php echo esc_html( get_the_title() ); ?></title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better for internationalization.
I've removed the escaping for get_the_title()
as it's escaped on save and considered safe.
<title><?php esc_html_e( 'Transcript', 'simple-podcasting' ); ?> - <?php echo esc_html( get_the_title() ); ?></title> | |
<title><?php | |
printf( | |
esc_html__( 'Transcript - %s', 'simple-podcasting' ), | |
get_the_title() | |
); | |
?></title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc, Most of the data is sanitised/escaped on save, and this is a very small concern, but in case an attacker would get hold of DB or is able to modify values of wp_posts, I think an XSS attack would be easy to do here, and I don't see much of an issue with escaping here(unless we're facing double escaping issues). So I'm in favour of keeping the escape, but let me know what you think about it.
@peterwilsoncc I left a comment on one of your concerns. I do not have time until at least two weeks from now to dig into this more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few notes following some branch testing.
The taxonomy name constant was renamed so I pushed c8216c25f70ef48c7b0971576a2758c550e98768 to resolve that.
@@ -0,0 +1,31 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention here was to follow a spec defined by Podcastindex.org
@nateconley Understood and makes sense.
Do you think it's worth noindexing the transcript page so it doesn't end up in search engines? To allow developers to filter the results for their site, the code could be:
if ( function_exists( 'wp_robots' ) && function_exists( 'wp_robots_no_robots' ) && function_exists( 'add_filter' ) ) {
add_filter( 'wp_robots', 'wp_robots_no_robots' );
wp_robots();
}
templates/transcript.php
Outdated
printf( | ||
/* translators: %s: The page title */ | ||
esc_html__( 'Transcript - %s', 'simple-podcasting' ), | ||
get_the_title() // phpcs:ignore WordPress.Security.EscapeOutput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
|
||
registerFormatType('podcasting/transcript-time', { | ||
title: __('Time', 'simple-podcasting'), | ||
tagName: 'time', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you are using kses to sanitize, you'll need to add the time
element to the list of allowed tags using the wp_kses_allowed_html
filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this filter.
includes/transcripts.php
Outdated
foreach ( $body_node->childNodes as $node ) { | ||
// phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase | ||
if ( XML_TEXT_NODE === $node->nodeType ) { | ||
$filtered_text .= '<p>' . $doc->saveHTML( $node ) . '</p>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing some strange markup here when using the citation button.
The markup is rendered as
<cite>Person 1</cite>
<p>: Welcome to Show</p>
<br>
<cite>Person 2</cite>
<p>: I'm P2</p>
<br>
<cite>Person 1:</cite>
<p> and I'm P1.</p>
<br>
</body>
The markup is saved in post meta as:
<cite>Person 1</cite>: Welcome to Show<br>
<cite>Person 2</cite>: I'm P2<br>
<cite>Person 1:</cite> and I'm P1.<br>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to make RichText do too much. This should have been InnerBlocks, and has been changed. The added benefit is that this is much more user friendly now.
Quick update: I will have all comments addressed here by EOW. |
@peterwilsoncc This is ready for a re-review. |
I could also use a bit of help with the failing e2e tests. It looks like those might be caused by a missing dependency. |
Sorry Nate, I'm still having problems while testing. If I start a citation within a transcript block then the editor stays as a citataion after I press return Reviewing some documentation, it looks like having them as blocks rather than inline is a good improvement. |
@peterwilsoncc Thanks for the catch! I have modified the cite and time inner blocks to behave similar to other core blocks; when you press return a paragraph block is created. Again, looks like cypress test are failing, but this seems unrelated to this branch from what I can see. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nateconley, this looks good to me.
I pushed a fix for the failing E2E tests by downgrading mochawesome-json-to-md
. For some reason version 1.x isn't working properly with our GitHub actions.
Description of the Change
Adds a block that saves the transcript to post meta and allows editor to either link to the transcript, display the transcript, or only have the transcript accessible via the endpoint.
Closes #28
TODO:
<podcast:transcript url="https://example.com/podcasts/podcast-name/episode-slug/transcript.html" type="text/html" language="en" />
Alternate Designs
This approach is as a block, also discussed were this functionality a meta field.
Possible Drawbacks
This approach does not account for the classic editor.
Verification Process
Checklist:
Changelog Entry
Credits
@nateconley