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

Update amp-toolbox-php to 0.6.0 (almost) and update error page to support fatal errors (in PHP 7+) #6421

Merged
merged 8 commits into from
Jun 24, 2021
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"ext-json": "*",
"ext-libxml": "*",
"ext-spl": "*",
"ampproject/amp-toolbox": "0.5.2",
"ampproject/amp-toolbox": "dev-main",
"cweagans/composer-patches": "1.7.1",
"fasterimage/fasterimage": "1.5.0",
"sabberworm/php-css-parser": "dev-master#bfdd976"
Expand Down
27 changes: 15 additions & 12 deletions composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

65 changes: 38 additions & 27 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -1666,34 +1666,10 @@ public static function finish_output_buffering( $response ) {

try {
$response = self::prepare_response( $response );
} catch ( Error $error ) { // Only PHP 7+.
$response = self::render_error_page( $error );
} catch ( Exception $exception ) {
$title = __( 'Failed to prepare AMP page', 'amp' );
$message = __( 'A PHP error occurred while trying to prepare the AMP response. This may not be caused by the AMP plugin but by some other active plugin or the current theme. You will need to review the error details to determine the source of the error.', 'amp' );

$error_page = Services::get( 'dev_tools.error_page' );

$error_page
->with_title( $title )
->with_message( $message )
->with_exception( $exception )
->with_response_code( 500 );

// Add link to non-AMP version if not canonical.
if ( ! amp_is_canonical() ) {
$non_amp_url = amp_remove_paired_endpoint( amp_get_current_url() );

// Prevent user from being redirected back to AMP version.
if ( true === AMP_Options_Manager::get_option( Option::MOBILE_REDIRECT ) ) {
$non_amp_url = add_query_arg( QueryVar::NOAMP, QueryVar::NOAMP_MOBILE, $non_amp_url );
}

$error_page->with_back_link(
$non_amp_url,
__( 'Go to non-AMP version', 'amp' )
);
}

$response = $error_page->render();
$response = self::render_error_page( $exception );
}

/**
Expand All @@ -1708,6 +1684,41 @@ public static function finish_output_buffering( $response ) {
return $response;
}

/**
* Render error page.
*
* @param Throwable $throwable Exception or (as of PHP7) Error.
*/
private static function render_error_page( $throwable ) {
$title = __( 'Failed to prepare AMP page', 'amp' );
$message = __( 'A PHP error occurred while trying to prepare the AMP response. This may not be caused by the AMP plugin but by some other active plugin or the current theme. You will need to review the error details to determine the source of the error.', 'amp' );

$error_page = Services::get( 'dev_tools.error_page' );

$error_page
->with_title( $title )
->with_message( $message )
->with_throwable( $throwable )
->with_response_code( 500 );

// Add link to non-AMP version if not canonical.
if ( ! amp_is_canonical() ) {
$non_amp_url = amp_remove_paired_endpoint( amp_get_current_url() );

// Prevent user from being redirected back to AMP version.
if ( true === AMP_Options_Manager::get_option( Option::MOBILE_REDIRECT ) ) {
$non_amp_url = add_query_arg( QueryVar::NOAMP, QueryVar::NOAMP_MOBILE, $non_amp_url );
}

$error_page->with_back_link(
$non_amp_url,
__( 'Go to non-AMP version', 'amp' )
);
}

return $error_page->render();
}

/**
* Filter rendered partial to convert to AMP.
*
Expand Down
110 changes: 73 additions & 37 deletions src/DevTools/ErrorPage.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
namespace AmpProject\AmpWP\DevTools;

use AmpProject\AmpWP\Infrastructure\Service;
use Error;
use Exception;
use InvalidArgumentException;
use Throwable;

/**
* Produces an error page similar to wp_die().
Expand Down Expand Up @@ -52,11 +55,11 @@ final class ErrorPage implements Service {
private $link_text = '';

/**
* Exception of the error page.
* Throwable of the error page.
*
* @var Exception
* @var Throwable
*/
private $exception;
private $throwable;

/**
* Response code of the error page.
Expand Down Expand Up @@ -118,13 +121,18 @@ public function with_back_link( $link_url, $link_text ) {
}

/**
* Set the exception of the error page.
* Set the throwable of the error page.
*
* @param Exception $exception Exception to use.
* @param Throwable $throwable Exception or Error to use. The Throwable type does not exist in PHP 5,
* which is why type is absent from the function parameter.
* @throws InvalidArgumentException If $throwable is not an Exception or an Error.
* @return self
*/
public function with_exception( Exception $exception ) {
$this->exception = $exception;
public function with_throwable( $throwable ) {
if ( ! ( $throwable instanceof Exception || $throwable instanceof Error ) ) {
throw new InvalidArgumentException( 'Parameter must be Throwable (Exception or Error).' );
}
$this->throwable = $throwable;
return $this;
}

Expand Down Expand Up @@ -163,7 +171,7 @@ public function render() {
}

/**
* Send the exception that was caught to the error log.
* Send the throwable that was caught to the error log.
*/
private function send_to_error_log() {
// Don't send to error log if fatal errors are not to be reported.
Expand All @@ -172,16 +180,18 @@ private function send_to_error_log() {
return;
}

error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
sprintf(
"%s - %s (%s) [%s]\n%s",
$this->message,
$this->exception->getMessage(),
$this->exception->getCode(),
get_class( $this->exception ),
$this->exception->getTraceAsString()
)
);
if ( null !== $this->throwable ) {
error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
sprintf(
"%s - %s (%s) [%s]\n%s",
$this->message,
$this->throwable->getMessage(),
$this->throwable->getCode(),
get_class( $this->throwable ),
$this->throwable->getTraceAsString()
)
);
}
}

/**
Expand Down Expand Up @@ -226,9 +236,10 @@ private function get_html( $styles, $text_direction ) {
</head>
<body id="error-page">
<h1>{$this->render_title()}</h1>
<p>{$this->render_message()}</p>
{$this->render_message()}
{$this->render_source()}
{$this->render_exception()}
{$this->render_support_link()}
{$this->render_throwable()}
{$this->render_back_link()}
</body>
</html>
Expand All @@ -250,16 +261,42 @@ private function render_title() {
* @return string KSES-sanitized message.
*/
private function render_message() {
return wp_kses_post( $this->message );
return '<p>' . wp_kses_post( $this->message ) . '</p>';
}

/**
* Render the source of the exception file.
* Render support link.
*
* @return string
*/
private function render_support_link() {
return '<p>' . wp_kses(
sprintf(
/* translators: %s is the AMP support forum URL. */
__( 'If you get stuck, you may want to share any details in a new topic on the plugin\'s <a href="%s" target="_blank" rel="noreferrer noopener">support forum</a>.', 'amp' ),
esc_url( __( 'https://wordpress.org/support/plugin/amp/', 'amp' ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we actually want this link to be translatable? It could potentially allow an attacker to send the user to a "fishing support forum" where they post their logs to a third-party... Yes, it is far-fetched, I know.

Copy link
Member Author

Choose a reason for hiding this comment

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

The URL was made translatable in #6320 as well. There are these instances of translatable URLs:

includes/options/class-amp-options-manager.php
470:						esc_url( __( 'https://wordpress.org/support/article/why-should-i-use-https/', 'amp' ) )

assets/src/components/error-screen/index.js
49:						__( 'https://wordpress.org/support/plugin/amp/', 'amp' ),

assets/src/settings-page/analytics.js
26:	__( 'https://wordpress.org/plugins/google-site-kit/', 'amp' ),
27:	__( 'https://developers.google.com/analytics/devguides/collection/amp-analytics/', 'amp' ),
213:							__( 'https://amp.dev/documentation/components/amp-analytics/', 'amp' ),
215:							__( 'https://amp-wp.org/documentation/getting-started/analytics/', 'amp' ),
220:							__( 'https://amp.dev/documentation/guides-and-tutorials/optimize-and-measure/configure-analytics/analytics-vendors/', 'amp' ),

assets/src/settings-page/paired-url-structure.js
337:						__( 'https://amp-wp.org/?p=10004', 'amp' ),

src/Admin/SiteHealth.php
248:				esc_url( __( 'https://www.php.net/manual/en/book.curl.php', 'amp' ) ),

src/DevTools/ErrorPage.php
277:				esc_url( __( 'https://wordpress.org/support/plugin/amp/', 'amp' ) )

src/LoadingError.php
47:							esc_url( __( 'https://wordpress.org/support/plugin/amp/', 'amp' ) )

Granted, we aren't going to have alternative support forums so I'm fine with removing the translation in both places.

cc @delawski

),
[
'a' => [
'href' => true,
'target' => true,
'rel' => true,
],
]
) . '</p>';
}

/**
* Render the source of the throwable.
*
* @return string File source data.
*/
private function render_source() {
$source = $this->likely_culprit_detector->analyze_exception( $this->exception );
if ( null === $this->throwable ) {
return '';
}

$source = $this->likely_culprit_detector->analyze_throwable( $this->throwable );

if ( ! empty( $source['type'] ) && ! empty( $source['name'] ) ) {
$name_markup = "<strong><code>{$source['name']}</code></strong>";
Expand Down Expand Up @@ -291,15 +328,14 @@ private function render_source() {
}

/**
* Render the exception of the error page.
* Render the throwable of the error page.
*
* The exception details are only rendered if both WP_DEBUG and
* WP_DEBUG_DISPLAY are true.
* The exception/error details are only rendered if both WP_DEBUG and WP_DEBUG_DISPLAY are true.
*
* @return string HTML describing the exception that was thrown.
* @return string HTML describing the exception/error that was thrown.
*/
private function render_exception() {
if ( null === $this->exception ) {
private function render_throwable() {
if ( null === $this->throwable ) {
return '';
}

Expand All @@ -321,24 +357,24 @@ private function render_exception() {
[
sprintf(
'<strong>%s</strong> (%s) [<em>%s</em>]',
esc_html( $this->exception->getMessage() ),
esc_html( $this->exception->getCode() ),
esc_html( get_class( $this->exception ) )
esc_html( $this->throwable->getMessage() ),
esc_html( $this->throwable->getCode() ),
esc_html( get_class( $this->throwable ) )
),
sprintf(
'<em>%s:%d</em>',
esc_html( $this->exception->getFile() ),
esc_html( $this->exception->getLine() )
esc_html( $this->throwable->getFile() ),
esc_html( $this->throwable->getLine() )
),
'',
sprintf(
'<small>%s</small>',
esc_html( $this->exception->getTraceAsString() )
esc_html( $this->throwable->getTraceAsString() )
),
]
);

return "<hr><pre class='exception'>{$contents}</pre>";
return "<hr><pre class='throwable'>{$contents}</pre>";
}

/**
Expand Down Expand Up @@ -408,7 +444,7 @@ private function get_styles( $text_direction ) {
line-height: 1.5;
margin: 25px 0 20px;
}
#error-page .exception,
#error-page .throwable,
code {
font-family: Consolas, Monaco, monospace;
overflow-x: auto;
Expand Down
Loading