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

Ensure Schema.org meta data has correct date #1721

Merged
merged 3 commits into from Dec 12, 2018

Conversation

Projects
None yet
2 participants
@kienstra
Copy link
Collaborator

kienstra commented Dec 8, 2018

Thanks to Fernando Tellado for raising this in:
https://wordpress.org/support/topic/warning-date-expects-parameter-2-to-be-integer-after-1-0-0/

The documentation of date() requires its 2nd argument to be an int.

And the documentation of get_the_date() says it returns a string.

But locally, using PHP 7.2, it looked like get_the_date() returns an int.

I couldn't reproduce this, but it'll probably be good to ensure this is an int. Edge cases might need more thought, like if get_the_date() somehow returns '' or false, and intval() returns 0 for it.

Ensure the second argument of date() is an int
The documentation of date() requires the 2nd argument to be an int
And the documentation of get_the_date() says it returns a string.
But locally, using PHP 7.2, it looked like get_the_date()
returns an int.
I couldn't reproduce this,
but it'll probably be good to ensure this is an int.
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 9, 2018

This won't fix the problem. The issue is that a filter is incorrectly modifying the value in get_the_date(). This has come up before. I believe you can see the fix in support fourms. A better fix would be to avoid using this function which can have a filtered value.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 9, 2018

Here is the support topic that identifies the underlying problem: https://wordpress.org/support/topic/line-error-shown-with-wp_debug/

@westonruter
Copy link
Member

westonruter left a comment

I believe we can simplify things by just using a single mysql2date call. Also, I believe we should be using the GMT values.

Show resolved Hide resolved includes/amp-helper-functions.php Outdated
Show resolved Hide resolved includes/amp-helper-functions.php Outdated
@@ -870,8 +870,8 @@ function amp_get_schemaorg_metadata() {
'@type' => is_page() ? 'WebPage' : 'BlogPosting',
'mainEntityOfPage' => get_permalink(),
'headline' => get_the_title(),
'datePublished' => date( 'c', get_the_date( 'U', $post->ID ) ),
'dateModified' => date( 'c', get_the_date( 'U', $post->ID ) ),

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 11, 2018

Member

This was incorrectly using the published date as the modified date.

westonruter and others added some commits Dec 12, 2018

Update includes/amp-helper-functions.php
Apply @westonruter's suggested change.

Co-Authored-By: kienstra <kienstraryan@gmail.com>
Update includes/amp-helper-functions.php
Apply Weston's suggested change to use `mysql2date()` instead of `date()`

Co-Authored-By: kienstra <kienstraryan@gmail.com>

@westonruter westonruter added this to the v1.0.1 milestone Dec 12, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator Author

kienstra commented Dec 12, 2018

Looks Good

Hi @westonruter,
Your work here is a nice improvement. Like you pointed out, there are now different dates for datePublished and dateModified.

date-published-modified

And some pages that I've tested in the Google Structured Data Testing Tool look good:
structured-data-testing-tool

This should also prevent the issue in the support topic, as this doesn't use date() or get_the_date().

This works in my testing in Classic, Native, and Paired mode, though we wouldn't expect a difference there anyway.

@kienstra

This comment has been minimized.

Copy link
Collaborator Author

kienstra commented Dec 12, 2018

What do you think about including this in 1.0.1?

@kienstra kienstra changed the title [WIP] Ensure that date() has a correct second argument type Ensure that date() has a correct second argument type Dec 12, 2018

@westonruter westonruter changed the title Ensure that date() has a correct second argument type Ensure Schema Dec 12, 2018

@westonruter westonruter changed the title Ensure Schema Ensure Schema.org meta data has correct date Dec 12, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 12, 2018

Something that just came to mind is that it could be a site actually wants to filter the value of the date to be something other than it is stored in the database. Maybe this is an extreme edge case, but it is something that we should keep on the lookout for. A site could always filter the metadata itself in this case.

@westonruter westonruter merged commit 8e0d48a into 1.0 Dec 12, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/schema_org_metadata branch Dec 12, 2018

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.