Move enqueuing of assets to 'wp_enqueue_scripts' #1023
Conversation
@kopepasah found an issue, where the 'amp-runtime' script appeared in the admin menu. As Weston mentioned, this should be enqueued in 'wp_enqeue_scripts.' Also, this wraps the enqueuing in if ( is_amp_endpoint() ). This also appeared where they were enqueued before.
| /** | ||
| * Enqueue AMP assets if this is an AMP endpoint. | ||
| * | ||
| * @since 0.7 |
There was a problem hiding this comment.
Maybe this should be 1.0 instead. I'm happy to change it.
There was a problem hiding this comment.
0.7 is correct. I changed the branch to merge this PR.
As Weston mentioned, finish_init already checks for that. @todo: make is_amp_endpoint() return false if: is_admin() or is_feed(), and if it's a REST response.
|
Working On Hi @westonruter, function is_amp_endpoint() {
+ if ( is_admin() || is_feed() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {
+ return false;
+ }
if ( amp_is_canonical() ) {
return true;
}I need to address some failed unit tests from that change, and update the unit tests for that function. If it's alright, I'll come back to this in about an hour. |
is_admin(), is_feed(), and REST_REQUEST. As Weston mentioned, this shouldn't be true for admin pages. Also, modify PHPUnit tests for this.
|
With the commit above, this PR is ready review. |
| @@ -67,6 +67,7 @@ public function test_enqueue_admin_assets() { | |||
| // Test inline script boot. | |||
| $this->assertTrue( false !== stripos( wp_json_encode( $script_data ), 'ampPostMetaBox.boot(' ) ); | |||
| unset( $GLOBALS['post'] ); | |||
| unset( $GLOBALS['current_screen'] ); | |||
There was a problem hiding this comment.
This is needed to counteract set_current_screen( 'post.php' ) above. This caused is_admin() to be true on later tests.
There was a problem hiding this comment.
FYI: Probably better to such things in tearDown to ensure they are run regardless of whether the tests fail in this function. If a test fails, then the vars wouldn't get cleared.
| @@ -171,6 +171,10 @@ function post_supports_amp( $post ) { | |||
| * @return bool Whether it is the AMP endpoint. | |||
| */ | |||
| function is_amp_endpoint() { | |||
| if ( is_admin() || is_feed() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) { | |||
There was a problem hiding this comment.
We may need to revisit this with #1014 since we may want to render AMP content for a single property in the REST API response. We don't need to worry about it yet.
| @@ -100,6 +100,8 @@ public static function init() { | |||
| * action to template_redirect--the wp action--is used instead. | |||
| */ | |||
| add_action( 'wp', array( __CLASS__, 'finish_init' ), PHP_INT_MAX ); | |||
|
|
|||
| add_action( 'wp_enqueue_scripts', array( __CLASS__, 'enqueue_assets' ) ); | |||
There was a problem hiding this comment.
Putting this here caused a regression where the amp-runtime is enqueued even on non-AMP pages. PR to fix in #1033
Request For Code Review
Hi @westonruter,
@kopepasah found an issue where the
amp-runtimescript appeared in thewp-admin/edit.phppage.As you mentioned, this should be enqueued on the action
wp_enqeue_scripts.So this does that, and wraps the enqueuing in
if ( is_amp_endpoint() )This also appeared where they were enqueued before.