Skip to content

Convert string to DateTime object for languages other than English #18083

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

Merged
merged 24 commits into from
Nov 6, 2018
Merged

Convert string to DateTime object for languages other than English #18083

merged 24 commits into from
Nov 6, 2018

Conversation

bnymn
Copy link
Contributor

@bnymn bnymn commented Sep 16, 2018

Converting from string into PHP DateTime object is failing for locales other than en_US. This PR aims to solve this problem.

Description

DateTime class can parse a given string if it is in English, but it fails for other languages. To solve this issue, we have used IntlDateFormatter class for parsing string dates.

For cases when locale cannot be resolved, we explicitly have defined the pattern. Please refer to this line for the edge case.
https://github.com/magento/magento2/pull/18083/files#diff-9f6dc57683917f529571d34de6f14d15R335

Fixed Issues (if relevant)

  1. Datetime Error on Newsletter Template #5037: Datetime Error on Newsletter Template

Manual testing scenarios

  1. Login into Magento admin
  2. On the top right corner, click on your admin username, then click Account Settings.
  3. Change admin Interface locale to any language other than English.
  4. Create a newsletter template
    4.1. Go to Marketing -> Newsletter Template.
    4.2. Add a new template if there is not any.
  5. Go to Marketing -> Newsletter Template.
  6. Select "Queue Newsletter" action for any template.
  7. Set a date for Queue Date Start field.
  8. Save newsletter.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team magento-engcom-team added Partner: Webjump partners-contribution Pull Request is created by Magento Partner labels Sep 16, 2018
@magento-engcom-team
Copy link
Contributor

Hi @bnymn. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me $VERSION instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@bnymn
Copy link
Contributor Author

bnymn commented Sep 16, 2018

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @bnymn. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @bnymn, here is your new Magento instance.
Admin access: https://pr-18083.engcom.dev.magento.com/admin
Login: admin Password: 123123q

@bnymn bnymn changed the title Feature/issue 5037 Convert string to DateTime object for languages other than English Sep 16, 2018
@ishakhsuvarov ishakhsuvarov added this to the Release: 2.3.1 milestone Sep 18, 2018
Copy link
Member

@slavvka slavvka left a comment

Choose a reason for hiding this comment

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

Please fix the problems and failed tests.

@@ -85,23 +85,31 @@ public function __construct(
}

/**
* {@inheritdoc}
* Return path to default timezone
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't duplicate descriptions in the implementation. They are in the interface. Please return them to {@inheritdoc}

Copy link
Contributor Author

@bnymn bnymn Sep 24, 2018

Choose a reason for hiding this comment

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

Thank you for the explanation @slavvka . So should I just ignore these warnings then?
https://travis-ci.org/magento/magento2/jobs/429304963

Copy link
Member

Choose a reason for hiding this comment

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

Basically you could ignore those since {@inheritdoc} may be used in such cases and there's a bug in our sniffer. But it would be good if you make changes according to this guide https://devdocs.magento.com/guides/v2.3/coding-standards/docblock-standard-general.html#inheritdoc. As you could see there should be enough to remove curly braces {}

* @return string
* @deprecated
*/
public function convertConfigTimeToUtcWithPattern($date, $format = 'Y-m-d H:i:s', $pattern = 'Y-m-d H:i:s');
Copy link
Member

Choose a reason for hiding this comment

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

Adding a public method into interface marked as API is backward incompatible and therefore is forbidden (please look BIC guide https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/). Please reimplement it. You can use the possible approach how to do that in the mentioned guide.

@bnymn
Copy link
Contributor Author

bnymn commented Oct 8, 2018

@magento-engcom-team give me test instance

* @return string
*/
public function convertLocalizedDateToUtc($date);
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add a new line in the end

@magento-engcom-team
Copy link
Contributor

Hi @slavvka, thank you for the review.
ENGCOM-3256 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

Hi @bnymn. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

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.

5 participants