Skip to content

NEW Required for silverstripe-cms/issue/965 #2945

Closed
wants to merge 1 commit into from

3 participants

@phptek
phptek commented Mar 12, 2014
  • Added FormatSettings() method to Datetime.
  • Flexible enough to take date set via setValue() or via argument.
@simonwelsh

Can you change the commit message to something that describes the change?

@simonwelsh simonwelsh and 1 other commented on an outdated diff Mar 12, 2014
model/fieldtypes/Date.php
@@ -152,6 +152,46 @@ public function FormatI18N($formattingString) {
}
}
+ /**
+ * Return the date formatted as per user CMS settings using the given $datetime
+ * or the field's current value if not passed.
+ *
+ * Accepts dates in the following arrangements:
+ * - <date> <time>
+ * - <timestamp>
+ * - <date> Will return the time as 12:00
+ * - <time> Will return the date as current date
+ *
+ * @param string $datetime A date/time string.
+ * @param Member $member
+ * @return boolean | string A time and date pair formatted as per user-defined settings.
+ */
+ public function FormatFromSettings($datetime = null, $member = null) {
@simonwelsh
simonwelsh added a note Mar 12, 2014

Why does this need to be able to take a date time? It's trivial to turn a date time string into a SS_Datetime object.

@phptek
phptek added a note Mar 12, 2014

Yes it is, but not from user's stored security date-preferences which are stored as Zend date-format.

@simonwelsh
simonwelsh added a note Mar 12, 2014

What does the user's display preferences have to do with this function taking a date time string? This method's for converting to that format, not from it, so it still doesn't make sense to take a string.

If this is to take a string it should be a static method, not an instance once.

@phptek
phptek added a note Mar 12, 2014

Hmmm see use-case in silverstripe/silverstripe-cms#968. Referencing a static here isn't going to work.

@simonwelsh
simonwelsh added a note Mar 12, 2014

That use case also doesn't pass in a value.

@phptek
phptek added a note Mar 12, 2014

Correct. It's flexible enough for both use cases. I've have had countless occasions recently where I've wanted a framework method that, given a date string, will return it formatted as per the current-user's settings, and was amazed that none existed. The fix I was working on for silverstripe-cms was the ideal opportunity to roll that in.

@simonwelsh
simonwelsh added a note Mar 13, 2014

DBField::create_field('Datetime', $value)->FormatFromSettings()

Taking a datetime string is still not needed and doing the above also means you get much better support for turning the string into a timestamp.

@phptek
phptek added a note Mar 13, 2014

OK. Fair enough, thanks for the feedback. That also therefore settles, in which class the method best sits :-).

I'll refactor, cancel the PR and issue a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@simonwelsh simonwelsh and 1 other commented on an outdated diff Mar 12, 2014
model/fieldtypes/Date.php
+
+ if(!$member) {
+ $member = Member::currentUser();
+ }
+ // Bail out if member not logged in
+ if(!$member) {
+ return false;
+ }
+
+ $input = trim($datetime ? $datetime : $this->value);
+ // Bail out if no passed $datetime or no value set
+ if(!$input) {
+ return false;
+ }
+ // Test for timestamp or date
+ $datetime = preg_match("#[^\d]#", $input) ? strtotime($input) : $input;
@simonwelsh
simonwelsh added a note Mar 12, 2014

Only using $this->value would make this check redundant as it's always Y-m-d[ H:i:s].

@phptek
phptek added a note Mar 12, 2014

Yes, but you can also pass a timestamp as a parameter, and have a date/time string returned as per the user's stored preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@simonwelsh simonwelsh and 2 others commented on an outdated diff Mar 12, 2014
model/fieldtypes/Date.php
+ return false;
+ }
+
+ $input = trim($datetime ? $datetime : $this->value);
+ // Bail out if no passed $datetime or no value set
+ if(!$input) {
+ return false;
+ }
+ // Test for timestamp or date
+ $datetime = preg_match("#[^\d]#", $input) ? strtotime($input) : $input;
+
+ $formatD = $member->getDateFormat();
+ $formatT = $member->getTimeFormat();
+
+ $zendDate = new Zend_Date($datetime);
+ return $zendDate->toString($formatD).' '.$zendDate->toString($formatT);
@simonwelsh
simonwelsh added a note Mar 12, 2014

This is a Date type. Why is the time being included?

@phptek
phptek added a note Mar 12, 2014

The class represents a Date type. The method is a date/time formatter. If you think this is best put into Datetime, just let me know.

@simonwelsh
simonwelsh added a note Mar 12, 2014

As it formats a Datetime, it should be in Datetime.

@phptek
phptek added a note Mar 12, 2014

It also formats a date, and a time. I really hope you scrutinise everyone's commits to this extent.

@tractorcow
tractorcow added a note Mar 12, 2014

I feel lucky if @simonwelsh spends as much time as this on any of my pull requests. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@phptek
phptek commented Mar 12, 2014

Commit message changed. Please let me know what else you'd like included.

@simonwelsh

Commit message doesn't match the method you're adding

Russell Michell NEW Added FormatFromSettings(). Formats dates as per user preferences.
- Required for silverstripe-cms/issue/965
- Flexible enough to take date set via setValue() or via argument.
4ce2925
@phptek phptek closed this Mar 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.