Skip to content

Move AMP initialization logic into after_setup_theme, conditioned on amp_is_enabled filter#826

Merged
ThierryA merged 2 commits into
developfrom
fix/disable-amp-availability
Dec 11, 2017
Merged

Move AMP initialization logic into after_setup_theme, conditioned on amp_is_enabled filter#826
ThierryA merged 2 commits into
developfrom
fix/disable-amp-availability

Conversation

@westonruter

@westonruter westonruter commented Dec 7, 2017

Copy link
Copy Markdown
Member

Move AMP initialization logic to after_setup_theme to only run if amp_is_enabled filter applies as true.

Amends #813.
See #709.

@westonruter westonruter requested a review from ThierryA December 7, 2017 21:22
@westonruter

Copy link
Copy Markdown
Member Author

@ThierryA In 856bb27 I fixed a defect across the AMP plugin whereby if you added a add_filter( 'amp_is_enabled', '__return_false' ) to your theme, only parts of the plugin would be disabled.

@westonruter

Copy link
Copy Markdown
Member Author

I cherry-picked 36881a4 into #825, so this can be rebased once that PR is merged.

@ThierryA ThierryA left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@westonruter how about reverting 36881a4 since it is handle (and modified) in #825?

Comment thread amp.php Outdated
load_plugin_textdomain( 'amp', false, plugin_basename( AMP__DIR__ ) . '/languages' );

amp_define_query_var();
amp_after_setup_theme();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Any specific reason to call amp_after_setup_theme() here since the amp_init() is invoked in a init action which is declared in the amp_after_setup_theme()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch. Yeah, this should have already been called so it should be removed entirely.

Comment thread amp.php
amp_render_post( get_queried_object() );
exit;
$post = get_queried_object();
if ( $post instanceof WP_Post ) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should it be exciting even if the page can't be rendered?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it is very exciting. 😉

In practice this function only ever gets called if the queried object is already guaranteed to be a post, so this is more for the sake of static analysis. But yeah, it could be better to not exit here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Haha, very exciting indeed 😂

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we can just leave this as-is right now. We'll need to revisit how this works when we start rendering non-posts in AMP anyway.

@westonruter

Copy link
Copy Markdown
Member Author

@ThierryA yes, I'll rebase 36881a4 out of the PR.

@westonruter westonruter force-pushed the fix/disable-amp-availability branch from 856bb27 to 0627442 Compare December 8, 2017 23:39
@westonruter westonruter changed the title Show AMP as disabled without UI to make enabled if post is password protected or skipped Move AMP initialization logic into after_setup_theme, conditioned on amp_is_enabled filter Dec 9, 2017
@ThierryA ThierryA merged commit cee3876 into develop Dec 11, 2017
@westonruter westonruter deleted the fix/disable-amp-availability branch December 11, 2017 21:25
@westonruter westonruter added this to the v0.6 milestone Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants