Skip to content
This repository was archived by the owner on Mar 26, 2021. It is now read-only.

Fixes for invalid GMT offset when creating new DateTimeZone in post-edit widget - #53

Merged
benlk merged 9 commits into
masterfrom
52-gmt-offset
Sep 19, 2018
Merged

benlk merged 9 commits into
masterfrom
52-gmt-offset

Conversation

@benlk

@benlk benlk commented May 23, 2018

Copy link
Copy Markdown
Contributor

Changes

  • Where the plugin generates a DateTimeZone object from the database, moves that construction into its own separate testable function nprstory_get_datetime()
  • Includes a test for said function that iterates over the known valid GMT offsets from WordPress's database. This doesn't cover all possible GMT offsets, of which there are a ton. This covers almost every offset seen in WordPress's selector drop-down, and a few 45-minute-off timezones. It does not cover the 20- and 40-minute-off time zones. It does cover the '' empty-string option_value of 'gmt_offset', which is what this plugin was tested under in the first place.

Notes

  • This does not need to check the option_value 'timezone_string' because WordPress uses that value in the function wp_timezone_override_offset which is a filter upon the output of get_option( 'gmt_offset' );

Why

#52 is described based on an email from Erin, and is caused by DateTimeZone::__construct() not accepting GMT offsets in the format that WordPress saves them in the database. WordPress saves them as decimal hours; PHP's DateTimeZone::__construct() seems to want the offset in seconds. I say "seems" because it's not explicitly described as such in any php.net documentation, but the documentation for DateTimeZone::listAbbreviations() and the output of that function both describe offsets in numbers that only make sense as whole hours if the number displayed is a number of hours multiplied by 360.

As a sample:

[0] => Array
        (
            [dst] => 1
            [offset] => -14400
            [timezone_id] => America/Porto_Acre
        )

The time zone "America/Porto_Acre" has an offset -14400. -14400 divided by powers of ten doesn't seem correct. -14400 divided by 60 is -240; divided by 60*60=360 is -4. Checking https://en.wikipedia.org/wiki/List_of_tz_database_time_zones shows that the offset for "America/Porto_Acre" is presently -05:00. The one-hour discrepancy between -4 and -05:00 is explained by "America/Porto_Acre" being subject to daylight saving time.

Fixes #52.

benlk added 2 commits May 22, 2018 21:46
…ction, and write a test for that that iterates over the known valid GMT offsets from WordPress's database.

This is for #52
@benlk benlk added the bug label May 23, 2018
@benlk benlk added this to the 1.7.2 milestone May 23, 2018
@benlk benlk self-assigned this May 23, 2018
@benlk
benlk requested a review from BoomieBunmiNPR May 23, 2018 02:03
… the boilerplate from the 'wp scaffold plugin' command, without removing existing bootstrap.php functionality.
…story_publish_meta_box

Fixes the error seen in https://travis-ci.com/npr/nprapi-wordpress/jobs/146555595

1) Test_MetaBoxes::test_nprstory_publish_meta_box
Missing argument 1 for nprstory_publish_meta_box(), called in /tmp/wordpress/src/wp-content/plugins/nprapi-wordpress/tests/test-meta-boxes.php on line 17 and defined
/tmp/wordpress/src/wp-content/plugins/nprapi-wordpress/meta-boxes.php:18
/tmp/wordpress/src/wp-content/plugins/nprapi-wordpress/tests/test-meta-boxes.php:17
…prstory_get_datetimezone_all

The error message:

2) Test_PushStory::test_nprstory_get_datetimezone_all
Use of undefined constant DateTimeZone - assumed 'DateTimeZone'
/tmp/wordpress/src/wp-content/plugins/nprapi-wordpress/tests/test-push_story.php:163

Must pass the class.
this does not fix anything, but it does make the code in this file more readable, which helped with debugging the tests that were broken in #53 and that were fixed by commits 7db701d 2c74c8b
@benlk

benlk commented Sep 19, 2018

Copy link
Copy Markdown
Contributor Author

Automated tests pass.

Human-run tests should perform the following tasks:

  1. Activate the plugin
  2. Configure the plugin with test API credentials
  3. Create a new post.
    1. Does the NPR Story API meta box appear on the page?
    2. Are there no errors in the server console?
    3. Change the time zone in Dashboard > Settings > General to a new time zone. Repeat step 3 until satisfied.
  4. Push a post to the API.
  5. Pull a post from the API.

@ghost
ghost self-requested a review September 19, 2018 14:49

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code looks good to me

@benlk
benlk merged commit 086e047 into master Sep 19, 2018
benlk added a commit that referenced this pull request Sep 19, 2018
@eteare

eteare commented Sep 20, 2018

Copy link
Copy Markdown
Contributor

I have uploaded the master repo to our test WP site. I made posts and changed dates, but I am unable to get stories to or pull stories from the API. There could be some operator error here, however, when I tried to pull just one story, I got this error message.

screen shot 2018-09-20 at 12 33 44 pm

Do you think this error is related to plugin updates or something in our WP site's config? Any insights helpful. Thank you.

@benlk

benlk commented Sep 20, 2018

Copy link
Copy Markdown
Contributor Author

There's two issues there:

"non-static method DS_NPR_API::load_page_hook() should not be called statically" — this should not prevent the page from loading.

"call to undefined function simplexml_load_string()" — this has me puzzled, because that function appears to be a standard part of PHP. Was your test server's PHP compiled without the SimpleXML extension? https://secure.php.net/manual/en/simplexml.setup.php

@ghost
ghost deleted the 52-gmt-offset branch September 28, 2018 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants