Skip to content
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

lesson video allow html tag change with default wp kses #2950

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

dydrema
Copy link

@dydrema dydrema commented Mar 3, 2020

Fixes #

Changes proposed in this Pull Request:

  • i am getting issue with video allow html tags
    I have replaced allowed_html tag with default kses_allowed_html with the same structure

Testing instructions:

  • please check in allowed_html in file class-sensi-lesson.php and class-sensi fronted.php both are different so i make same like class-sensi-lesson.php in class-sensi fronted.php

Screenshot / Video

Proposed changelog entry for your changes:

@dydrema dydrema changed the title lesson video allow html tag change with defualt wp kses lesson video allow html tag change with default wp kses Mar 11, 2020
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further to my comment below, we should fix the Video Embed Code field of the lesson in the editor, so that custom HTML can be saved there too.

),
'video' => Sensei_Wp_Kses::get_video_html_tag_allowed_attributes(),
);
$this->allowed_html = Sensei_Wp_Kses::get_default_wp_kses_allowed_html();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider also allowing any tags here that wp_kses_post allows:

$this->allowed_html = array_merge( Sensei_Wp_Kses::get_default_wp_kses_allowed_html(), wp_kses_allowed_html( 'post' ) );

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you are right but this is a field for any video so we can't allow other tags, i thing does'e need to allow wp_kses_allowed_html( 'post' ),

Copy link
Author

@dydrema dydrema Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateds now #2992

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. For example, Vimeo embeds include HTML tags like <p> and <a> that would be stripped out by this.

Besides, if we're allowing people to save HTML in the video embed code for a lesson in WP Admin, but then don't display it on the front-end, what's the point?

@jmalko
Copy link
Contributor

jmalko commented Nov 9, 2022

The code that is in place presently prevents me from using a video host outside of YouTube, Vimeo, or Self-host. I happen to use a service called Wistia and have made workarounds in my templates in order to be able to embed videos. When I want to "allow HTML", there is a good reason for it.

The solution offered here would be both what would work and what I would expect with such a feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Author Reply Requires response from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants