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

Added meta tag for identifying AMP pages generated by plugin #810

Merged
merged 7 commits into from Dec 5, 2017

Conversation

Projects
None yet
3 participants
@vaporwavre
Copy link
Contributor

commented Nov 22, 2017

Fixes #807.

@westonruter
Copy link
Member

left a comment

@vaporwavre I think it may be better to add a function that outputs the new meta tag at the amp_post_template_head action. This will “guarantee” that the meta tag will be added to Custom Templates, as they are required to do this action: https://github.com/Automattic/amp-wp#custom-template

The Travis build is failing because the commit lacks spaces around the parameters in the function call, and this is required by WordPress Coding Standards.

@vaporwavre

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

Regarding Travis errors, I've corrected the SpacingBeforeClose error locally, but why am I getting the FunctionComment.Missing errors? I see no function documentation comments for the other functions in those files.

@westonruter

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

@vaporwavre the FunctionComment.Missing error is coming from a lack of there being phpDoc of the new method you're adding. There are a lot of phpDoc comments missing, so we added Travis CI checks to start enforcing phpDoc so we can start adding them bit by bit in the code that we touch.

For what to include in phpDoc, see https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/

@ThierryA
Copy link
Collaborator

left a comment

@vaporwavre I would advise to change all instances of generatedby with generator (ex. add_generatedby_metadata vs add_generator_metadata).

It seems like you don't have the dev-lib pre-commit hook installed on your machine, it would be helpful to add in order to validate WPCS before pushing commits to avoid Travis build from failing. More info here.

Thanks,

@@ -104,4 +105,10 @@ public static function add_analytics_data( $amp_template ) {
echo AMP_HTML_Utils::build_tag( 'amp-analytics', $amp_analytics_attr, $script_element );
}
}
public static function add_generatedby_metadata( $amp_template ) {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 27, 2017

Collaborator

Fix WPCS error (Squiz.Commenting.FunctionComment.Missing). Here is a draft:

/**
 * Add AMP generator metadata.
 *
 * @param object $amp_template AMP_Post_Template object.
 * @since 0.6
 */
public static function add_generatedby_metadata( $amp_template ) {
?>
<meta name="generator" content="<?php echo esc_attr( $amp_template->get( 'generatedby_metadata' ) );?>" />

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 27, 2017

Collaborator

Fix WPCS error (Squiz.PHP.EmbeddedPhp.SpacingBeforeClose).

@@ -57,6 +57,7 @@ public function __construct( $post_id ) {
'canonical_url' => get_permalink( $post_id ),
'home_url' => home_url(),
'blog_name' => get_bloginfo( 'name' ),
'generatedby_metadata' => 'AMP Plugin v' . AMP__VERSION,

This comment has been minimized.

Copy link
@ThierryA

ThierryA Nov 27, 2017

Collaborator

Fix WPCS error (WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned).

@westonruter westonruter changed the base branch from master to develop Nov 27, 2017

vaporwavre added some commits Nov 28, 2017

Fix WPCS error (Squiz.Commenting.FunctionComment.Missing)
Fix WPCS error (Squiz.PHP.EmbeddedPhp.SpacingBeforeClose)
Fix WPCS error (WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)
Fix WPCS error (WordPress.Arrays.MultipleStatementAlignment.DoubleArr…
…owNotAligned)

Fix WPCS error (Squiz.Commenting.FunctionComment.Missing)
@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Dec 3, 2017

Thanks for your contribution @vaporwavre, I see that the build is still failing. For it to pass, you will have to add PHPDoc here and align all array operators here (this will need slight refactoring).

Please don't hesitate to ask if you need a hand with that, I am happy to help out with the final touches if you would prefer to 😄

@vaporwavre

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

Sorry, I couldn't get pre-commit to work locally.

@ThierryA
Copy link
Collaborator

left a comment

Thanks for your contribution @vaporwavre, this is good to go.

@ThierryA ThierryA merged commit 6df01e9 into ampproject:develop Dec 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@westonruter westonruter changed the title Added meta tag for identifying AMP pages generated by plugin. Issue #807 Added meta tag for identifying AMP pages generated by plugin Dec 6, 2017

@westonruter westonruter added this to the v0.6 milestone Dec 13, 2017

westonruter added a commit that referenced this pull request Jan 13, 2018

Revert "Merge pull request #725 from Automattic/amedina/add-amp-actio…
…ns-class-hierarchy"

This reverts commit c5237e6, reversing
changes made to 41af096.

Fixes merge conflicts introduced by #810 and #828.

westonruter added a commit that referenced this pull request Jan 13, 2018

Revert "Merge pull request #725 from Automattic/amedina/add-amp-actio…
…ns-class-hierarchy"

This reverts commit c5237e6, reversing
changes made to 41af096.

Fixes merge conflicts introduced by #810 and #828.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.