Introduce a new configuration field to set the DateTime Format at least for the import/export #114
Conversation
…st for the future
|
Ulf Tietze seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
|
@ulftietze I think your git user and github user are not the same or linked. Could you please check that and get back to me on this. |
| /** | ||
| * @api | ||
| */ | ||
| interface GetDateTimeFormatFromConfigInterface |
There was a problem hiding this comment.
I think there are two issues in this class/interface:
- It's designed as a command - thing that changes the state. However, the intent of the interface is to return a format by some scope. Usually, we use
noun::verbnaming for such things. getDefaultDatetimeFormatPathmethod is an implementation details. Some implementations ofGetDateTimeFormatFromConfigInterfacemay store the datetime formats in DB or files without any XML paths.- Inteface and class contain a part of namespace in the names.
Considering points above I suggest to drop the getDefaultDatetimeFormatPath method from inteface and rename interface + class + execute method to something like Magento\Framework\Stdlib\DateTime\ConfigInterface::getFormat($scopeType, $scopeCode)
| @@ -37,6 +37,7 @@ class Data extends \Magento\Framework\App\Helper\AbstractHelper | |||
| const XML_PATH_DEFAULT_COUNTRY = 'general/country/default'; | |||
| const XML_PATH_DEFAULT_LOCALE = 'general/locale/code'; | |||
| const XML_PATH_DEFAULT_TIMEZONE = 'general/locale/timezone'; | |||
| const XML_PATH_DEFAULT_DATETIME_FORMAT = 'general/locale/locale_date_format'; | |||
There was a problem hiding this comment.
The implemntation of interface is better place for this constant
| @@ -121,6 +121,10 @@ | |||
| <label>Weight Unit</label> | |||
| <source_model>Magento\Directory\Model\Config\Source\WeightUnit</source_model> | |||
| </field> | |||
| <field id="locale_date_format" translate="label" type="text" sortOrder="8" showInDefault="1" showInWebsite="1" showInStore="1"> | |||
There was a problem hiding this comment.
Currently, there is a circular dependency in Store-Directory modules pair. Any dependencies between these two modules are not welcome according to the desired state.
This PR defines: interface and implementation in framework; constant and system config setting in Directory module; di configuration in Store module with dependency on Directory module.
I suggest to put everything except the interface in Directory module. Alternatively, it's possible to have both interface and class in framework, and all other things in Directory module.
- Merge Pull Request magento/graphql-ce#114 from magento/graphql-ce:grqphql-mutations-74 - Merged commits: 1. db3f2c7 2. eb2a5a3
|
Hi @ulftietze I am going to close this PR due to inactivity. If you have time and want to come back to this PR please feel free to address the issues raised in the review and reopen this PR. Thanks |
This pull request is one step of the general problem, that the import/export is facing, when it's come from export to reimport the same file. One related issue is the #61. This PR fixes one of the reasons, that this could not work.
We ran here into the problem, that the dateformat in standart format looks like eg.
German: 12.11.18 15:42:45
US: 11/12/18 15:42:45
We can't reimport this, because of the PHP "strtotime()" function.
If you called this function with this dateformat, we ran into the issue, that this date could not resolve the "18" as year at the end. Probably PHP tried to lookup for the exact year, and this could be 1918, 2018, 2118, and so on. So this breaks everything.
Now you can set your default datetime in Stores > Configuration > General > Locale > DateTime Format.
It's saved under 'general/locale/locale_date_format'.
Fixed Issues (if relevant)
Contribution checklist