-
Notifications
You must be signed in to change notification settings - Fork 381
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
Adding sanitizer to support amp-o2-player #1202
Conversation
*/ | ||
public function __construct( DOMDocument $dom, array $args = array() ) { | ||
parent::__construct( $dom, $args ); | ||
$this->xpath = new DOMXPath( $dom ); |
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 xpath
setting here could just as well be moved to the sanitize
method since that is where it is used. This would avoid needing to override the constructor at all, and if the sanitizers are instantiated but never call sanitize
then we could avoid needlessly instantiating this object (however rare that may be).
private static $height = '270'; | ||
|
||
/** | ||
* Width to set for 02 Player elements. |
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.
Should be O2
not 02
.
public static $xpath_selector = '//div[ contains( @class, \'vdb_player\' ) ]/script'; | ||
|
||
/** | ||
* Height to set for 02 Player elements. |
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.
Should be O2
not 02
.
* @see https://www.ampproject.org/docs/reference/components/amp-o2-player | ||
*/ | ||
class AMP_O2_Player_Sanitizer extends AMP_Base_Sanitizer { | ||
const URL_PATTERN = '#.*delivery.vidible.tv\/jsonp\/pid=(.*)\/vid=(.*)\/(.*).js.*#i'; |
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.
Would be good to add phpdoc for this.
* @see https://www.ampproject.org/docs/reference/components/amp-o2-player | ||
*/ | ||
class AMP_O2_Player_Sanitizer extends AMP_Base_Sanitizer { | ||
const URL_PATTERN = '#.*delivery.vidible.tv\/jsonp\/pid=(.*)\/vid=(.*)\/(.*).js.*#i'; |
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.
Might want to use named capture groups to make it easier to reference the $matches
.
* Class AMP_O2_Player_Sanitizer | ||
* | ||
* Converts <div class="vdb_player><script></script></div> embed to <amp-o2-player> | ||
* |
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.
Could you add @since 1.0
tag to the class (at least)?
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.
done
…dding-amp-o2-player-sanitizer
Adding support for amp-o2-player
e.g: If we have the following markup
We should be able to render: