Skip to content

Commit

Permalink
Ensure validation_error_callback is registered prior to sanitizers be…
Browse files Browse the repository at this point in the history 
…ing created

Prevent output buffering to locate sources if overall output buffering for the response has not started
  • Loading branch information
@westonruter
westonruter committed Apr 29, 2018
1 parent f075298 commit 1e4ccf0
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 19 deletions.
26 changes: 21 additions & 5 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ class AMP_Theme_Support {
public static $headers_sent = array();

/**
* Output buffering level when starting.
* Whether output buffering has started.
*
* @since 0.7
* @var int
* @var bool
*/
protected static $initial_ob_level = 0;
protected static $is_output_buffering = false;

/**
* Initialize.
Expand Down Expand Up @@ -261,11 +261,12 @@ public static function add_hooks() {
* Start output buffering at very low priority for sake of plugins and themes that use template_redirect
* instead of template_include.
*/
add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ), 0 );
$priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.PHP.NewConstants.php_int_minFound
add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ), $priority );

// Add validation hooks *after* output buffering has started for the response.
if ( AMP_Validation_Utils::should_validate_response() ) {
add_action( 'template_redirect', array( 'AMP_Validation_Utils', 'add_validation_hooks' ), 1 );
AMP_Validation_Utils::add_validation_hooks();
}

// Commenting hooks.
Expand Down Expand Up @@ -978,6 +979,20 @@ public static function start_output_buffering() {
}

ob_start( array( __CLASS__, 'finish_output_buffering' ) );
self::$is_output_buffering = true;
}

/**
* Determine whether output buffering has started.
*
* @since 0.7
* @see AMP_Theme_Support::start_output_buffering()
* @see AMP_Theme_Support::finish_output_buffering()
*
* @return bool Whether output buffering has started.
*/
public static function is_output_buffering() {
return self::$is_output_buffering;
}

/**
Expand All @@ -990,6 +1005,7 @@ public static function start_output_buffering() {
* @return string Processed Response.
*/
public static function finish_output_buffering( $response ) {
self::$is_output_buffering = false;
return self::prepare_response( $response );
}

Expand Down
7 changes: 6 additions & 1 deletion includes/utils/class-amp-validation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,11 @@ public static function get_source( $callback ) {
*/
public static function can_output_buffer() {

// Output buffering for validation can only be done while overall output buffering is being done for the response.
if ( ! AMP_Theme_Support::is_output_buffering() ) {
return false;
}

// Abort when in shutdown since printing we know to be in buffering display handler, and no output allowed anyway.
if ( did_action( 'shutdown' ) ) {
return false;
Expand Down Expand Up @@ -1003,7 +1008,7 @@ public static function wrapped_callback( $callback ) {
}
$result = call_user_func_array( $function, array_slice( $args, 0, intval( $accepted_args ) ) );
if ( $has_buffer_started ) {
@ob_end_flush(); // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged
ob_end_flush();
}
array_pop( self::$hook_source_stack );

Expand Down
3 changes: 2 additions & 1 deletion tests/test-class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,8 @@ public function test_add_hooks() {
$this->assertEquals( 10, has_filter( 'customize_partial_render', array( self::TESTED_CLASS, 'filter_customize_partial_render' ) ) );
$this->assertEquals( 10, has_action( 'wp_footer', 'amp_print_analytics' ) );
$this->assertEquals( 100, has_filter( 'show_admin_bar', '__return_false' ) );
$this->assertEquals( 0, has_action( 'template_redirect', array( self::TESTED_CLASS, 'start_output_buffering' ) ) );
$priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.PHP.NewConstants.php_int_minFound
$this->assertEquals( $priority, has_action( 'template_redirect', array( self::TESTED_CLASS, 'start_output_buffering' ) ) );

$this->assertEquals( PHP_INT_MAX, has_filter( 'wp_list_comments_args', array( self::TESTED_CLASS, 'set_comments_walker' ) ) );
$this->assertEquals( 10, has_filter( 'comment_form_defaults', array( self::TESTED_CLASS, 'filter_comment_form_defaults' ) ) );
Expand Down
24 changes: 12 additions & 12 deletions tests/test-class-amp-validation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ public function test_wrap_widget_callbacks() {
) );
$_wp_sidebars_widgets[ $sidebar_id ] = array( $widget_id ); // WPCS: global override ok.

ob_start();
AMP_Theme_Support::start_output_buffering();
dynamic_sidebar( $sidebar_id );
$output = ob_get_clean();

Expand Down Expand Up @@ -440,54 +440,54 @@ public function test_callback_wrappers() {
$this->assertEquals( 10, has_action( $action_one_argument, array( $this, 'output_notice' ) ) );
$this->assertEquals( 10, has_action( $action_two_arguments, array( $this, 'output_message' ) ) );

ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( $action_function_callback );
$output = ob_get_clean();
$this->assertContains( '<div class="notice notice-error">', $output );
$this->assertContains( '<!--amp-source-stack {"type":"plugin","name":"amp"', $output );
$this->assertContains( '<!--/amp-source-stack {"type":"plugin","name":"amp"', $output );

ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( $action_no_argument );
$output = ob_get_clean();
$this->assertContains( '<div></div>', $output );
$this->assertContains( '<!--amp-source-stack {"type":"plugin","name":"amp"', $output );
$this->assertContains( '<!--/amp-source-stack {"type":"plugin","name":"amp"', $output );

ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( $action_one_argument, $notice );
$output = ob_get_clean();
$this->assertContains( $notice, $output );
$this->assertContains( sprintf( '<div class="notice notice-warning"><p>%s</p></div>', $notice ), $output );
$this->assertContains( '<!--amp-source-stack {"type":"plugin","name":"amp"', $output );
$this->assertContains( '<!--/amp-source-stack {"type":"plugin","name":"amp"', $output );

ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( $action_two_arguments, $notice, get_the_ID() );
$output = ob_get_clean();
ob_start();
AMP_Theme_Support::start_output_buffering();
self::output_message( $notice, get_the_ID() );
$expected_output = ob_get_clean();
$this->assertContains( $expected_output, $output );
$this->assertContains( '<!--amp-source-stack {"type":"plugin","name":"amp"', $output );
$this->assertContains( '<!--/amp-source-stack {"type":"plugin","name":"amp"', $output );

// This action's callback doesn't output any HTML tags, so no HTML should be present.
ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( $action_no_tag_output );
$output = ob_get_clean();
$this->assertNotContains( '<!--amp-source-stack ', $output );
$this->assertNotContains( '<!--/amp-source-stack ', $output );

// This action's callback comes from core.
ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( $action_core_output );
$output = ob_get_clean();
$this->assertContains( '<!--amp-source-stack {"type":"core","name":"wp-includes"', $output );
$this->assertContains( '<!--/amp-source-stack {"type":"core","name":"wp-includes"', $output );

// This action's callback doesn't echo any markup, so it shouldn't be wrapped in comments.
ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( $action_no_output );
$output = ob_get_clean();
$this->assertNotContains( '<!--amp-source-stack ', $output );
Expand All @@ -507,7 +507,7 @@ public function test_callback_wrappers() {
};
add_action( 'outer_action', $handle_outer_action );
add_action( 'inner_action', $handle_inner_action );
ob_start();
AMP_Theme_Support::start_output_buffering();
do_action( 'outer_action' );
$output = ob_get_clean();
$this->assertEquals(
Expand Down Expand Up @@ -579,7 +579,7 @@ public function test_wrapped_callback() {

$wrapped_callback = AMP_Validation_Utils::wrapped_callback( $callback );
$this->assertTrue( $wrapped_callback instanceof Closure );
ob_start();
AMP_Theme_Support::start_output_buffering();
call_user_func( $wrapped_callback );
$output = ob_get_clean();

Expand All @@ -600,7 +600,7 @@ public function test_wrapped_callback() {

$wrapped_callback = AMP_Validation_Utils::wrapped_callback( $callback );
$this->assertTrue( $wrapped_callback instanceof Closure );
ob_start();
AMP_Theme_Support::start_output_buffering();
$result = call_user_func( $wrapped_callback );
$output = ob_get_clean();
$this->assertEquals( 'Closure', get_class( $wrapped_callback ) );
Expand Down

0 comments on commit 1e4ccf0

Please sign in to comment.