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

Fix localized date time input #1795

Merged
merged 1 commit into from Jun 20, 2016
Merged

Fix localized date time input #1795

merged 1 commit into from Jun 20, 2016

Conversation

Evenprime
Copy link
Contributor

Related Thread on Forum: http://community.mybb.com/thread-165257.html

The target of this pull request was two-fold:

  1. Respect the "$mybb->settings['timeformat']" value (set in Admin-CP) at all locations that display time or allow the input of time as text (and don't/can't use my_date for date/time formatting). This affects:
    Admin-CP Forum Announcement creation/editing
    Admin-CP User banning preset bantimes selectbox entries
    Admin-CP Mass Mail creation/editing
    Mod-CP Forum Announcement creation/editing
    Delayed Moderation
  2. Respect current user timezone and dst when entering date/time. This affects:
    Admin-CP Mass Mail creation/editing
    Mod-CP Forum Announcement creation/editing
    Delayed Moderation (original reason for this pull request as mentioned in the corresponding thread)
    (Admin-CP Forum Announcement creation was intentionally left "GMT" only, because there is a label that states that inputs are always interpreted as GMT.)
    (The Calendar already offers to set the used timezone explicitely, therefore I didn't touch the logic there. I only shortened the time patterns at two locations, because the hour/minute part that is generated isn't used at these locations at all.)
  3. In addition minor related bugs are fixed:
    Admin-CP Mass Mail added the current users timezone when saving data. If at all, the timezone data should have been subtracted at that point. This bug lead to strange behaviour, where repeatedly editing and saving the same mass-mail would push its delivery time further into the future (when timezone offset is positive) or the past (the other case).
    Admin-CP Mass Mail would interpret "11:00 PM" as "11:00", because the matching for "pm" only checked for lowercase "pm". Everywhere else the same logic was case insensitive, now it is here too.
    The function get_event_date($event) in functions.php would have applied timezones twice, if the server has a timezone/location set that's not "+0", because mktime is used where gmmktime should've been used (The correct timezone information is applied one line later during a call to "my_date"). This method seems to not be used at all currently.

@Destroy666x Destroy666x added the b:1.8 Branch: 1.8.x label Jan 28, 2015
@Evenprime
Copy link
Contributor Author

Fix for #1796

@JoshHarmon JoshHarmon added the s:feedback Status: Feedback. Further information/input needed label Apr 16, 2015
@JN-Jones
Copy link
Contributor

Any reason you added the feedback label here @JoshHarmon? (Also note that those labels should be added to the issue not the PR ;))

@JoshHarmon
Copy link
Contributor

Was probably me interpreting the thing the label was for as something other than it really is. My interpretation was that feedback is for the verification/review/suggestions of/to a PR before merging.

On Jun 21, 2015, at 11:46 PM, Jones notifications@github.com wrote:

Any reason you added the feedback label here @JoshHarmon? (Also note that those labels should be added to the issue not the PR ;))


Reply to this email directly or view it on GitHub.

@JN-Jones
Copy link
Contributor

Here's a list of labels and how they should be used: http://community.mybb.com/thread-153813.html :P

@JN-Jones JN-Jones removed the s:feedback Status: Feedback. Further information/input needed label Jun 22, 2015
@@ -899,7 +899,7 @@
{
$privatecheck = '';
}
$start_date = explode("-", gmdate("j-n-Y-g:i A", $event['starttime']+$event['timezone']*3600));
$start_date = explode("-", gmdate("j-n-Y", $event['starttime']+$event['timezone']*3600));
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason time was completely removed instead of using the timeformat setting? Same applies to the line below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The date string that is generated by the gmdate() at these locations is immediatly split at the '-' characters.

Of the resulting array only the first (day of month), second (month) and third (year) elements are used in the code afterwards. The last element of the array (time) isn't used anywhere, therefore I removed it in all of these locations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, only 0, 1 and 2 are accessed. @Destroy666x was there anything else or is this merge ready?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, I haven't tested it and checked whether all time inputs are covered yet

@JN-Jones
Copy link
Contributor

Ok, whoever checks this then (don't have time to look at it atm) should also check whether the places where gmdate is used should probably use my_date. It's probably not needed for date select boxes but especially where time is used it should be rechecked.

@Eldenroot
Copy link
Contributor

@Evenprime - can you please solve conflicts? It seems to be fine, at least for me

@dequeues
Copy link
Contributor

dequeues commented Jun 9, 2016

This PR has merge conflicts that need to be resolved.This PR has merge conflicts that need to be resolved.

@dequeues dequeues added the s:in-progress Status: In Progress. Some work completed label Jun 9, 2016
Admin-CP announcements edit view

Use $mybb->settings['timeformat'] in announcement preview in admin-cp

Fix inconsistent timezone handling and time formatting in mass mails

Data was read applying the systems timezone with the "date()" function,
but was written and interpreted as GMT. Leading to "time leaps"
everytime a mass mail is loaded for editing and immediatly saved again.

Allow both "am/pm" and "AM/PM" like everywhere else as valid input

Always use the 'timeformat' setting for time of day displays and input
field presets.

Avoid applying timezones twice by using gmmktime (GMT) and then my_date
(respects timezone settings and DST of users)

Respect Timezone and DST of the user settings for mass mail sending

Fix starttime/endtime presets

Respect user timezone and dst for delayed moderation input and espect
'timeformat'-setting for display of existing delayed moderations.

Respect 'timeformat' for display of bantime selection box

Respect 'timeformat' for bantimes selection box display

Accidential commit

The time part is not used, therefore don't include it in the pattern

Announcement editing in ModCP respects user timezone and DST settings

Whitespace cleanup
@Evenprime
Copy link
Contributor Author

Squashed and rebased changes, fixed merge conflicts

@dequeues
Copy link
Contributor

dequeues commented Jun 9, 2016

Just looked through and all looks fine to me. @Destroy666x see anything else before I merge?

@Destroy666x
Copy link
Contributor

Destroy666x commented Jun 13, 2016

If you tested (not only looked through) it and everything works as supposed to - feel free to merge.

@dequeues dequeues self-assigned this Jun 20, 2016
@dequeues dequeues removed the s:in-progress Status: In Progress. Some work completed label Jun 20, 2016
@dequeues dequeues merged commit b1ae479 into mybb:feature Jun 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b:1.8 Branch: 1.8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants