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

Error template placeholder HTML comments can be stripped out by minifiers #709

Closed
westonruter opened this issue Feb 24, 2022 · 4 comments · Fixed by #720
Closed

Error template placeholder HTML comments can be stripped out by minifiers #709

westonruter opened this issue Feb 24, 2022 · 4 comments · Fixed by #720
Labels
Milestone

Comments

@westonruter
Copy link
Collaborator

As mentioned by @stack-exchange in a gist comment:

Placeholder comment inserted by wp_service_worker_error_message_placeholder() get stripped by AMP plugin.

The issue is the AMP Optimizer's MinifyHtml transformer which strips out HTML comments. This will also be an issue with other optimization plugins. Therefore, instead of wp_service_worker_error_message_placeholder() and wp_service_worker_error_details_template() outputting HTML comments, they should instead output HTML elements, identified by an ID.

/**
* Display service worker error message template tag.
*/
function wp_service_worker_error_message_placeholder() {
echo '<p><!--WP_SERVICE_WORKER_ERROR_MESSAGE--></p>';
}

/**
* Display service worker error details template.
*
* @param string $output Error details template output.
*/
function wp_service_worker_error_details_template( $output = '' ) {
if ( empty( $output ) ) {
$output = '<details id="error-details"><summary>' . esc_html__( 'More Details', 'pwa' ) . '</summary>{{{error_details_iframe}}}</details>'; // phpcs:ignore WordPressVIPMinimum.Security.Mustache.OutputNotation -- Variable includes iframe tag.
}
echo '<!--WP_SERVICE_WORKER_ERROR_TEMPLATE_BEGIN-->';
echo wp_kses_post( $output );
echo '<!--WP_SERVICE_WORKER_ERROR_TEMPLATE_END-->';
}

Note this will make the regex a bit more complicated here:

/([<]!--WP_SERVICE_WORKER_ERROR_TEMPLATE_BEGIN-->)((?:.|\n)+?)([<]!--WP_SERVICE_WORKER_ERROR_TEMPLATE_END-->)/,

@westonruter westonruter added bug Something isn't working service-workers labels Feb 24, 2022
@westonruter westonruter added this to the 0.7 milestone Feb 24, 2022
@thelovekesh thelovekesh self-assigned this Feb 24, 2022
@thelovekesh
Copy link
Collaborator

@westonruter I was working upon this and tried the following idea:
For error message

function wp_service_worker_error_message_placeholder() {
	// echo '<p><!--WP_SERVICE_WORKER_ERROR_MESSAGE--></p>';
	echo '<p><span id="wp-service-worker-error-message"></span></p>';
}

and replace whole span element with following reges

/[<]span id="wp-service-worker-error-message">(\s)*[<]\/span>/

For error message details:

function wp_service_worker_error_details_template( $output = '' ) {
	if ( empty( $output ) ) {
		$output = '<details id="error-details"><summary>' . esc_html__( 'More Details', 'pwa' ) . '</summary>{{{error_details_iframe}}}</details>'; // phpcs:ignore WordPressVIPMinimum.Security.Mustache.OutputNotation -- Variable includes iframe tag.
	}
	// echo '<!--WP_SERVICE_WORKER_ERROR_TEMPLATE_BEGIN-->';
	echo '<span id="wp-service-worker-error-template-begin"></span>';
	echo wp_kses_post( $output );
	// echo '<!--WP_SERVICE_WORKER_ERROR_TEMPLATE_END-->';
	echo '<span id="wp-service-worker-error-template-end"></span>';
}

and replace whole span element with following reges

/([<]span id="wp-service-worker-error-template-begin">(\s)*[<]\/span>)((?:.|\n)+?)([<]span id="wp-service-worker-error-template-end">(\s)*[<]\/span>)/

If you some another ideas please share and let me know your thoughts on the above method if it can work in our current scenerio.

@westonruter
Copy link
Collaborator Author

I just realized that we're using Mustache-like tokens in wp_service_worker_error_details_template() already (e.g. {{{error_details_iframe}}}). So what if we actually use Mustache tokens instead of HTML comments and instead of “milestone” HTML tags?

So that could be:

function wp_service_worker_error_message_placeholder() {
	echo '<p>{{{WP_SERVICE_WORKER_ERROR_MESSAGE}}}</p>';
}

And:

function wp_service_worker_error_details_template( $output = '' ) {
	if ( empty( $output ) ) {
		$output = '<details id="error-details"><summary>' . esc_html__( 'More Details', 'pwa' ) . '</summary>{{{error_details_iframe}}}</details>'; // phpcs:ignore WordPressVIPMinimum.Security.Mustache.OutputNotation -- Variable includes iframe tag.
	}
	echo '{{{WP_SERVICE_WORKER_ERROR_TEMPLATE_BEGIN}}}';
	echo wp_kses_post( $output );
	echo '{{{WP_SERVICE_WORKER_ERROR_TEMPLATE_END}}}';
}

@thelovekesh
Copy link
Collaborator

I found moustache template tokens more suitable in our use case. So using mustache- tokens at the place of HTML tags.

@pooja-muchandikar
Copy link

QA Passed ✅

Offline pages are rendered with proper as expected messages

Screenshot 2022-04-20 at 7 10 57 PM


Screenshot 2022-04-20 at 7 11 17 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants