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.

Sign up for GitHub

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

Prevent schema.org duplicates #992

Merged
merged 4 commits into from
Mar 13, 2018
Merged

Prevent schema.org duplicates #992

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1357330
Prevent schema.org duplicates
oscarssanchez Mar 7, 2018
c66bfc4
Prevent schema.org duplicates
oscarssanchez Mar 9, 2018
67de7c3
Schema.org prevent duplicates
oscarssanchez Mar 11, 2018
fda9f87
Merge branch 'develop' into add/962-prevent-schema-duplicates
westonruter Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion includes/amp-post-template-actions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ function amp_post_template_init_hooks() {
add_action( 'amp_post_template_head', 'amp_post_template_add_scripts' );
add_action( 'amp_post_template_head', 'amp_post_template_add_fonts' );
add_action( 'amp_post_template_head', 'amp_post_template_add_boilerplate_css' );
add_action( 'amp_post_template_head', 'amp_print_schemaorg_metadata' );
Copy link
Member

Choose a reason for hiding this comment

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

This should not be removed. The legacy post templates should remain as-is, I believe. The ensure_required_markup method below does not apply for the legacy post template actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @westonruter

Thanks for your feedback. I corrected that mistake and restored the line.

add_action( 'amp_post_template_head', 'amp_add_generator_metadata' );
add_action( 'amp_post_template_css', 'amp_post_template_add_styles', 99 );
add_action( 'amp_post_template_data', 'amp_post_template_add_analytics_script' );
Expand Down
24 changes: 23 additions & 1 deletion includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ public static function register_hooks() {
add_action( 'wp_print_styles', array( __CLASS__, 'print_amp_styles' ), 0 ); // Print boilerplate before theme and plugin stylesheets.
add_action( 'wp_enqueue_scripts', array( __CLASS__, 'enqueue_amp_default_styles' ), 9 );
add_action( 'wp_head', 'amp_add_generator_metadata', 20 );
add_action( 'wp_head', 'amp_print_schemaorg_metadata' );

if ( is_customize_preview() ) {
add_action( 'wp_enqueue_scripts', array( __CLASS__, 'dequeue_customize_preview_scripts' ), 1000 );
Expand Down Expand Up @@ -829,6 +828,13 @@ protected static function ensure_required_markup( DOMDocument $dom ) {
) );
$head->insertBefore( $meta_viewport, $meta_charset->nextSibling );
}
if ( ! self::schema_org_present( $dom ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

As noted below, I think this logic should be inlined (for now) as it is done above for $meta_charset and $meta_viewport and $rel_canonical below.

$script = $dom->createElement( 'script', wp_json_encode( amp_get_schemaorg_metadata() ) );
AMP_DOM_Utils::add_attributes_to_node( $script, array(
Copy link
Member

Choose a reason for hiding this comment

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

Since you're using $dom->createElement() here then I think it makes more sense to just do $script->setAttribute( 'type', 'application/ld+json' )

Copy link
Contributor Author

@oscarssanchez oscarssanchez Mar 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also true. Corrected it.

'type' => 'application/ld+json',
) );
$head->appendChild( $script );
}

// Ensure rel=canonical link.
$rel_canonical = null;
Expand Down Expand Up @@ -1007,4 +1013,20 @@ public static function prepare_response( $response, $args = array() ) {

return $response;
}

/**
* Checks if there is already a schema.org script in the head.
*
* @param DOMDocument $dom Representation of the document.
* @return bool
*/
public static function schema_org_present( DOMDocument $dom ) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename this to is_schema_org_metadata_present. But actually, I think maybe the logic should just be inlined in ensure_required_markup like the other checks are done. True that it may be better design to break up the logic into separate methods, but since the existing logic is not in methods then I don't know if we should split out this one separately.

If you changed ensure_required_markup from protected to be public and then added the logic from schema_org_present to it, along with unit tests for ensure_required_markup, then that would be great. Later we may very well want to break up ensure_required_markup into smaller methods, but I think that would probably be done along with breaking up the AMP_Theme_Support class into separate classes.

Copy link
Contributor Author

@oscarssanchez oscarssanchez Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I added your suggestions.

I also did the test for ensure_required_markup, however, I only made tests for the schema.org script part. Could you use tests for the entire function?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this method needs more tests, so if that's something you want to add that would be great.

$head = $dom->getElementsByTagName( 'head' )->item( 0 );
foreach ( $head->getElementsByTagName( 'script' ) as $script ) {
if ( 1 === preg_match( '/{"@context":"https?:[\\\]\/[\\\]\/schema\.org/', $script->nodeValue ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Before looking at the content of the element ($script->nodeValue) I think it should first check 'application/ld+json' === $script->getAttribute( 'type' ).

Also, I think the 1 === should be removed. As long as preg_match() returns a truthy value that's all we really care about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the integer comparison and added your suggestion, indeed it makes it more precise.

return true;
}
}
return false;
}
}
45 changes: 45 additions & 0 deletions tests/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,4 +330,49 @@ public function test_handle_xhr_request() {
AMP_Theme_Support::handle_xhr_request();
$this->assertContains( 'AMP-Access-Control-Allow-Source-Origin: https://example.org', xdebug_get_headers() );
}

/**
* Test schema_org_present().
*
* @dataProvider get_script_data
* @covers AMP_Theme_Support::schema_org_present()
* @param string $script The value of the script.
* @param boolean $expected The expected result.
*/
public function test_schema_org_present( $script, $expected ) {
$page = '<html><head><script>%s</script></head><body>Test</body></html>';
Copy link
Member

Choose a reason for hiding this comment

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

The <script> here is missing the type="application/ld+json".

Copy link
Contributor Author

@oscarssanchez oscarssanchez Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Very good observation, I added the type attribute on the new tests for ensure_required_markup()

$dom = new DOMDocument();
$dom->loadHTML( sprintf( $page, $script ) );
$this->assertEquals( $expected, AMP_Theme_Support::schema_org_present( $dom ) );
}

/**
* Data provider for test_schema_org_present().
*
* @return array
*/
public function get_script_data() {
return array(
'string_schema_value' => array(
'schema.org',
false,
),
'string_not_schema' => array(
'somethinghere.org',
false,
),
'json_schema_present_https' => array(
wp_json_encode( array( '@context' => 'https://schema.org' ) ),
true,
),
'json_schema_present_http' => array(
wp_json_encode( array( '@context' => 'http://schema.org' ) ),
true,
),
'json_schema_not_present' => array(
wp_json_encode( array( '@anothercontext' => 'https://schema.org' ) ),
false,
),
);
}
}