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

API Substitute Zend_Locale / Zend_Date with php-intl #6607

Merged
merged 5 commits into from
Feb 15, 2017

Conversation

tractorcow
Copy link
Contributor

@tractorcow tractorcow commented Feb 8, 2017

Fixes #6194

Requires:

Note: This PR removes about 1/4 of a million lines of code. Yay. :D

Includes a few other enhancements:

  • i18nTextCollector supports __CLASS__
  • Update FormField with a setSubmittedValue to differentiate between loading values from the database and loading values from user-submitted field values.
  • Add Resettable interface in place of having to manually invoke reset() on explicit classes between test resets.

Some minor functionality around currency names could not be ported across, and have been removed.

@tractorcow
Copy link
Contributor Author

Some immediate feedback is that Resettable is a bad design pattern. Perhaps we should address this at a later date with better locale caching.

@tractorcow tractorcow force-pushed the pulls/4.0/i18n-locale branch 3 times, most recently from 294070f to a9d39d2 Compare February 9, 2017 02:08
API Substitute Zend_Locale with Locale / NumberFormatter
API Substitute Zend_Date with IntlDateFormatter
API Added DBTIme::Nice12, FormatFromSettings
API Added Short() method to DBDate / DBTime / DBDatetime
API Add Date::getTimestamp()
API Added setSubmittedValue api for FormField
API Add second arg to base FormField::setValue()
API Major refactor of i18n into component data parts
API Implement Resettable interface to reset objects between tests
ENHANCEMENT Changed DBField::create_field return type to `static` to support better type hinting
ENHANCEMENT i18nTextCollector supports __CLASS__
@tractorcow tractorcow force-pushed the pulls/4.0/i18n-locale branch from a9d39d2 to 029a8b9 Compare February 9, 2017 02:29
Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

First round of review (got up to Form.php)

@@ -1051,6 +1052,26 @@ A very small number of methods were chosen for deprecation, and will be removed
* `ChangeSet` and `ChangeSetItem` have been added for batch publishing of versioned dataobjects.
* `DataObject.table_name` config can now be used to customise the database table for any record.
* `DataObjectSchema` class added to assist with mapping between classes and tables.
* `DBMoney` values are now treated as empty only Amount is null. Values without Currency
Copy link
Member

Choose a reason for hiding this comment

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

The first sentence doesn't make sense ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to

* `DBMoney` values are now treated as empty only if `Amount` field is null. If an `Amount` value
  is provided without a `Currency` specified, it will be formatted as per the current locale.

See http://userguide.icu-project.org/formatparse/datetime.
* getISOFormat() added which returns the standard date/time ISO 8601 pattern in CLDR format.
* Dates passed in m/d/y format will now raise a notice but will be parsed.
Dates passed to constructors should follow ISO 8601 (y-m-d).
Copy link
Member

Choose a reason for hiding this comment

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

You should use CLDR format strings to describe the dates here, for consistency: YYYY-MM-dd. Same with the line above.

* `Format()` method now use CLDR format strings, rather than PHP format strings.
See http://userguide.icu-project.org/formatparse/datetime.
* getISOFormat() added which returns the standard date/time ISO 8601 pattern in CLDR format.
* Dates passed in m/d/y format will now raise a notice but will be parsed.
Copy link
Member

Choose a reason for hiding this comment

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

"Dates passed with leading months (MM/dd/YYYY)..."


The below methods have been added or had their functionality updated to `DBDate`, `DBTime` and `DBDatetime`
* `getTimestamp()` added to get the respective date / time as unix timestamp (seconds since 1970-01-01)
* `Format()` method now use CLDR format strings, rather than PHP format strings.
Copy link
Member

Choose a reason for hiding this comment

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

* getISOFormat() added which returns the standard date/time ISO 8601 pattern in CLDR format.
* Dates passed in m/d/y format will now raise a notice but will be parsed.
Dates passed to constructors should follow ISO 8601 (y-m-d).
* 2-digit years will raise a notice.
Copy link
Member

Choose a reason for hiding this comment

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

"2-digit years in setValue() will raise a notice, since they're ambiguous without additional formatting hints"


`DBTime` specific changes:
* Added `DBTime::FormatFromSettings`
* Added `DBTime::Nice12`
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we remove Nice24 rather than adding Nice12, given that you do all of this via custom formatting now? The original purpose for this stuff was the lack of passing complex arguments via SSViewer, but that's since been resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. :)

@@ -1275,6 +1314,28 @@ handle field-level and form-level messages. This has the following properties:

* `$message` second constructor parameter is removed. Constructor only accepts `$result`,
which may be a string, and optional `$code`

New `DatetimeField` methods replace `getConfig()` / `setConfig()`:
Copy link
Member

Choose a reason for hiding this comment

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

What happened with datavalueformat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fixed to iso format. No more custom format for internal values.

Copy link
Contributor Author

@tractorcow tractorcow Feb 14, 2017

Choose a reason for hiding this comment

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

Added * ``datavaluefield`` config is removed as internal data value is now fixed to ISO 8601 format

@@ -1320,12 +1383,30 @@ handle field-level and form-level messages. This has the following properties:
for all DataObject subclasses, rather than just the basename without namespace.
* i18n key for locale-respective pluralisation rules added as '.PLURALS'. These can be configured
within yaml in array format as per [ruby i18n pluralization rules](http://guides.rubyonrails.org/i18n.html#pluralization).
* `i18n.all_locales` config moved to `Locales.locales`
Copy link
Member

Choose a reason for hiding this comment

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

All of these should mention the correct namespace, otherwise it's harder to fix via copy/paste.

* Designed so that tests can automatically flush these objects between runs in lieu
* of a real change.
*/
interface Resettable
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we move this stuff to explicit registration via YAML config? My understanding was that ClassInfo::implementorsOf() is on it's way out, because it forces us to parse all PHP files on a flush (which is no longer required for autoloading thanks to PSR-7). The interface Resettable is fine, but I wouldn't use ClassInfo::implementorsOf('Resettable') to retrieve those classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a problem I'm not sure we can solve in this pull request.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that introducing more use cases is taking it in the wrong direction. But you're right, this is best addressed separately - the interface itself is good, we might just move all those to explicit registration at some point.

Copy link
Member

Choose a reason for hiding this comment

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

My thinking is that we should introduce some kind of Application object that contains any in-memory caches and other objects that we might want to re-set, and have our tests create new Application objects. But it's a separate PR.

*/
protected $config;
protected $showCalendar = false;
Copy link
Member

Choose a reason for hiding this comment

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

One of the reasons I've avoided elevating these settings to class properties is that I didn't want to lock in the API to that extent - it's a bit like using FormField->setAttributes() to influence client-side react component presentation.

So rather than increasing the API surface of DateField, what do you think about separating out DateFieldSeparated extends DateField and DateFieldJQuery extends DateField? Then we can set the DateField injector service name to resolve toDateFieldJQuery by default, in order to get correct behaviour for built-in features like DBDate->scaffoldFormField()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've split out DateFieldSeparated, although I'm a bit hesitant to make a jquery-specific class which may limit usability (e.g. in react-formschema environments).

Copy link
Member

Choose a reason for hiding this comment

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

OK, I've reinstated format-based ordering and custom separators now

Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

Second round (up to DBDate.php)

* @return $this
*/
public function setValue($value)
public function setValue($value, $data = null)
Copy link
Member

Choose a reason for hiding this comment

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

You should note the change of method signature in the changelog, since it'll only cause a Strict notice when the field is actually used (meaning you hit a page with this particular form on it). Also, recommend setSubmittedValue() over setValue() there.

The forms.md docs are slightly wrong now:

The data value of the FormField submitted is not passed into validate. It is stored in the value property through the setValue method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where forms.md is; I'll have a look at refreshing the forms docs dir though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found and fixed. :)

public function setValue($value, $data = null)
{
if (is_array($value)) {
throw new InvalidArgumentException("Invalid value");
Copy link
Member

Choose a reason for hiding this comment

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

Hint that it's "Invalid because it's an array, expected X"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

throw new InvalidArgumentException("Invalid array value: Expected string");

if ($allowedCurrencies) {
$field = new DropdownField(
if (count($allowedCurrencies) === 1) {
// Dropdown field for multiple currencies
Copy link
Member

Choose a reason for hiding this comment

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

Inaccurate comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear, will fix!

// Convert string to array
// E.g. `44.00 NZD`
if (is_string($value) &&
preg_match('/^(?<amount>[\\d\\.]+)( (?<currency>\w{3}))?$/i', $value, $matches)
Copy link
Member

Choose a reason for hiding this comment

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

This hardcodes assumptions about decimal point formatting (Germans use a comma instead of a dot). Other countries will place the currency before the amount, or shorten it with a symbol. We didn't do string parsing before - why has it become necessary now? Any why did you remove support for the array notation which was designed to solve this issue? (consistent array payloads in form submission, rather than inconsistent string payloads)

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, setValue() is now only supposed to be used for hydration from the database, and setSubmittedValue() still supports arrays. And the assumption here is that hydration can either be from a DBMoney composite database fields (two columns), or a single string database field (which is a new feature you've added). This means we devs can control the formatting of the string field (not defined by user input), but it's still a bit odd to support this when we have a superior way with the more normalised DBMoney? It also means you can't properly prepopulate a field with values, without casting into DBMoney first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also means you can't properly prepopulate a field with values, without casting into DBMoney first.

You can pass in an array with Amount and Currency keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear, I'm throwing an exception... I'd intended to support that array format, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed error case to

} elseif (!is_array($value)) {
    throw new InvalidArgumentException("Invalid currency format");
}

public function setLocale($locale)
{
$this->_locale = $locale;
$this->fieldAmount->setLocale($locale);
Copy link
Member

Choose a reason for hiding this comment

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

This breaks for readonly versions of this field:

[Emergency] Uncaught BadMethodCallException: Object->__call(): the method 'getLocale' does not exist on 'SilverStripe\Forms\NumericField_Readonly'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified this so that fieldAmount remains a NumericField, but is assigned setReadonly(true) instead of being type-shifted.

} // Default text input field
else {
$html = parent::Field();
$valArr = $this->iso8601ToArray($this->Value());
Copy link
Member

Choose a reason for hiding this comment

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

This breaks when locale isn't en_us, since Value() uses iso8601ToLocalised(), but iso8601ToArray() expects ISO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Can fix with $this->dataValue() instead .:)

*/
public function performReadonlyTransformation()
Copy link
Member

Choose a reason for hiding this comment

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

Somehow the readonly version doesn't respect setLocale('de') or setScale(4). The disabled version is fine, weird. Example from a slightly modified frameworktest:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleting NumericField_Readonly helped. :)

* @return $this
*/
public function setLocale($locale)
public function setScale($scale)
Copy link
Member

Choose a reason for hiding this comment

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

Where did you get the "scale" terminology from? The ICU docs behind PHP's NumberFormatter refer to this as the "significant digit": http://www.icu-project.org/apiref/icu4c/classDecimalFormat.html#details

Copy link
Contributor Author

@tractorcow tractorcow Feb 14, 2017

Choose a reason for hiding this comment

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

Nope nothing to do with significant digits. scale is number of digits to the RIGHT of the decimal point. significant digits means significant digits from the left of the number. E.g. 1340.0 is 1335.00 to 3 significant digits. What i want is the number of digits (including zeros) after ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find the correct term, to be honest. Closest reference I could find is https://msdn.microsoft.com/en-us/library/ms190476.aspx

Scale is the number of digits to the right of the decimal point in a number. For example, the number 123.45 has a precision of 5 and a scale of 2.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, both left/right digits are "significant" - quoting https://en.wikipedia.org/wiki/Significant_figures#Rounding_and_decimal_places

Trailing zeros in a number containing a decimal point are significant. For example, 12.2300 has six significant figures: 1, 2, 2, 3, 0 and 0

But yeah, you're correct that "significant digits" is the wrong term here. I'm used to "decimal places", but since you've documented it well, I'm also happy with "scale"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, significant figures are counted FROM the left, but potentially including digits on the right. Not ONLY whole number values. :)

@@ -271,18 +309,23 @@ public function Rfc822()
public function Rfc2822()
{
if ($this->value) {
return date('Y-m-d H:i:s', strtotime($this->value));
return date('Y-m-d H:i:s', $this->getTimestamp());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use getFormatter() consistently here, rather than relying on date() for some of the features? It'll be confusing to refer back to two different symbol systems (date() and ICU)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was too lazy to re-write the strings...

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, there's still around two dozen occurrences of date() in core - low priority, not a merge blocker.

Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

Third and final part of the review

// US date format with 4-digit year first
if (preg_match('#^(?<year>\\d{4})/(?<day>\\d+)/(?<month>\\d+)(?<time>.*)$#', $value, $matches)) {
trigger_error(
"Implicit y/d/m conversion. Use " . self::ISO_DATE . " to prevent this notice.",
Copy link
Member

Choose a reason for hiding this comment

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

Add value to error message (since y/d/m is more of a guess at this point, could well be y/m/d)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

who would do such a thing though? =(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just going to stop falling back and fail any non-iso 8601 format. Too much maintenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DBDate / DBDatetime now fail if passed not-iso8601 date as values

$pastDate = mktime(0, 0, 0, $fmonth, $fday, $fyear);
$curDate = mktime(0, 0, 0, date('m'), date('d'), $fyear);
// US date format without 4-digit year first: assume m/d/y
if (preg_match('#^(?<month>\\d+)/(?<day>\\d+)/(?<year>\\d+)(?<time>.*)$#', $value, $matches)) {
Copy link
Member

Choose a reason for hiding this comment

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

How do you know that it's m/d/y here? Could be NZ date format with a short year (d/m/y), e.g. 31/07/17. I'd change the error message to "Ambiguous short year format detected. Use <ISO_DATE> to prevent this notice". Also, add the value to the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slashes = US date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://php.net/manual/en/function.strtotime.php#100144

The best way to compensate for this is by modifying your joining characters. Forward slash (/) signifies American M/D/Y formatting, a dash (-) signifies European D-M-Y and a period (.) signifies ISO Y.M.D. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to simplify the hell out of this.

*/
public static function past_date($fmonth, $fday = 1, $fyear = null)
protected function explodeDateString($value)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you replace the new DateTime($value) based parsing with regexes here? DateTime is based on strtotime(), which supports more formats than you cover here. Is this an attempt to warn when passing invalid or ambiguous formats like two-digit years, and narrow down edge cases? Seems reasonable. but will require a changelog entry. The PHPDoc should also explain why it's not using strtotime()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As with DateField, I'm not trying to support arbitrary custom formats. DBDate / DBDatetime are intended to load records from the database, and I've chosen a restrictive set of date formats based on this restriction.

If you are passing in raw user into into DBDate / DBDatetime then something is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

We've discussed this a bit more on Slack, and Damian confirmed that the intent was to narrow down supported cases. Given this is populated from the database or somewhat filtered/sanitized user input, that's OK

if (!$lang || !$region) {
return false;
}
return strcasecmp($lang, $region)
Copy link
Member

Choose a reason for hiding this comment

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

Is multiplication really what you want here? Seems like a roundabout way of expressing a conditional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make it more explicit.

foreach ($tokens as $token) {
if (is_array($token)) {
list($id, $text) = $token;

// Check class
Copy link
Member

Choose a reason for hiding this comment

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

What are these text collector changes 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.

I'll add to 4.0.0.md. This is so that you can use _t(__CLASS__.'.SOMEKEY') instead of the FQN.

Copy link
Member

Choose a reason for hiding this comment

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

i18nTextCollector could probably use a rewrite to make use of php-parser. Separate optional PR, though.

}

return $paths;
return static::getData()->scriptDirection($locale);
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't you move this to Locale as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because i18n has a global template provider that needs this method.

@tractorcow
Copy link
Contributor Author

I've added another commit with changes requested. Will fix tests tomorrow.

API Restrict allowed values parsed via DBDate::setValue
API Remove NumericField_Readonly
API Remove DBTime::Nice12 / Nice24
@tractorcow tractorcow force-pushed the pulls/4.0/i18n-locale branch from 6e08a18 to 014f0d2 Compare February 14, 2017 22:08
@tractorcow
Copy link
Contributor Author

Ok, if tests pass this is good to merge.

@tractorcow
Copy link
Contributor Author

ok, all non-cms tests passing. CMS tests will pass with cms pr merged.

* @config
* @var array
*/
private static $tinymce_lang = [
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this static could only list the exceptions to the rule strtok($input, '_') and be a lot shorter.

Copy link
Member

@sminnee sminnee left a comment

Choose a reason for hiding this comment

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

Assuming Ingo's okay with this, I am too :-)

foreach ($tokens as $token) {
if (is_array($token)) {
list($id, $text) = $token;

// Check class
Copy link
Member

Choose a reason for hiding this comment

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

i18nTextCollector could probably use a rewrite to make use of php-parser. Separate optional PR, though.


## Separate Day / Month / Year Fields

The following setting will display your DateField as three input fields for day, month and year separately. HTML5
placeholders 'day', 'month' and 'year' are enabled by default.
To display separate input fields for day, month and year separately you can use the `DateFieldSeparated` subclass`.
Copy link
Member

Choose a reason for hiding this comment

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

This references DateFieldSeparated rather than SeparatedDateField - I've fixed this in a follow-up commit.

}

// Join all fields
// @todo custom ordering based on locale
Copy link
Member

Choose a reason for hiding this comment

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

I'm just implementing this - it's a needless feature regression compared to 3.x

@chillu chillu merged commit 2805a13 into silverstripe:master Feb 15, 2017
@chillu chillu deleted the pulls/4.0/i18n-locale branch February 15, 2017 21:56
@dhensby
Copy link
Contributor

dhensby commented Feb 15, 2017

whoop

@chillu
Copy link
Member

chillu commented Feb 15, 2017

Created a follow up issue with #6626

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants