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

Ability to detect ISO8601 dates #614

Closed
tchapi opened this issue Jul 25, 2018 · 5 comments
Closed

Ability to detect ISO8601 dates #614

tchapi opened this issue Jul 25, 2018 · 5 comments

Comments

@tchapi
Copy link

tchapi commented Jul 25, 2018

This is:

- [x] a feature request
- [x] **not** a usage question

Related : #70 and #298 (comment) (kindof)

What is the expected behavior?

ISO 8601 date format should be recognised in https://github.com/PHPOffice/PhpSpreadsheet/blob/master/src/PhpSpreadsheet/Shared/Date.php#L435

How ?

We could change the regex to something along those lines :

^(\d{1,4}[ \.\/\-][A-Z]{3,9}([ \.\/\-]\d{1,4})?|[A-Z]{3,9}[ \.\/\-]\d{1,4}([ \.\/\-]\d{1,4})?|\d{1,4}[ \.\/\-]\d{1,4}([ \.\/\-]\d{1,4})?)((?: |T)\d{1,2}:\d{1,2}(:\d{1,2})?)?(?:\.\d{1,3})?Z?(?:\+\d{1,2}:?\d{1,2})?$

Adding the following blocks :

  • (?: |T) instead of (the space character) allows to retrieve the "T" delimiter. Group is not capturing
  • (?:\.\d{1,3})? gets the optional microsecs in +000 format. Group is not capturing
  • Z? gets the optional "Z" modifier following the date and time string
  • (?:\+\d{1,2}:?\d{1,2})? gets the optional timezone in +0200 or +02:00 format. Group is not capturing

This is a first naïve implementation that does not account for the timezone, but I think it's still a plus.

See this link for testing.

I can make a PR if this is something you'll be willing to add.
AFAIK this change is not BC breaking anything and is relatively safe.

(The DateTime::DATEVALUE function should be changed accordingly of course)

Happy to discuss it
Thanks

@tchapi
Copy link
Author

tchapi commented Jul 31, 2018

:tumbleweed:

@PowerKiKi
Copy link
Member

Sounds like a good idea to me, but you'll have to create unit test, including for non-ISO dates, so that the method is entirely covered.

I'd be OK without supporting timezone for a first version.

@MarkBaker any opinion on supporting ISO8601 format ?

@dkarlovi
Copy link
Contributor

Having gone through iterating over a large-ish Excel file, I'd advise against any automagical features based on regex.

They cause a lot of overhead when processing files already (just for figuring out the cell coordinates).

@tchapi
Copy link
Author

tchapi commented Aug 21, 2018

Well the regex is already there anyway. Moreover this is in the data → Excel direction, so not used when processing Excel files.

@stale
Copy link

stale bot commented Oct 20, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added the stale label Oct 20, 2018
@stale stale bot closed this as completed Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants