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

Fix: Allow years lower than 1970 in DateTime component. #13602

Conversation

jorgefilipecosta
Copy link
Member

Description

Fixes: #13579

This PR changes the DateTime component to allow years between 0 and 1970 that were previously unallowed.
The classic editor allows this set of years so we are updating the component to be equivalent.
Years lower than 0 and greater than 9999 are not allowed because JS error happens inside moment functions if these years are used. The classic editor and the quick edit form also don't allow this set of years.

How has this been tested?

I checked I'm able to use years between 0 and 1970 on the publish date field with success

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Jan 30, 2019
@aduth
Copy link
Member

aduth commented Jan 30, 2019

This PR changes the DateTime component to allow years between 0 and 1970 that were previously unallowed.

For what reason was this previously disallowed? The correspondence to unix epoch signals to me a possible technical limitation.

@jorgefilipecosta
Copy link
Member Author

For what reason was this previously disallowed? The correspondence to unix epoch signals to me a possible technical limitation.

I also expected it may have been a technical limitation but everything seems to work correctly. Previously it was not possible to type directly a year lower than 1970 but using the calendar picker it was possible to select a date lower than 1970 e.g: a date in 1969.

@aduth
Copy link
Member

aduth commented Jan 30, 2019

There was some prior concern on the validation noted by @pento at #7621 (comment) , but I don't think it was ever properly addressed whether the validation was necessary. I'd be inclined to think it should be fine to change (especially also because the other similar validation will produce many inaccuracies, e.g. 31 days in non-31 day months). I'll do some final testing before approval.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Works well 👍

Future improvements could include:

  • Get rid of the validation altogether? Year -1 is valid, so is year 10000. The sanity checks seem unnecessary.
  • Some unit tests.

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

Let's remove the limits all together.

Technically, moment supports years from -270,000 to 270,000, but I think it's overkill to worry about that. The same reason we don't check the length of post_content before inserting it, despite it technically having a limit of 2^32 bytes.

@jorgefilipecosta
Copy link
Member Author

Hi @pento and @aduth thank you for the reviews.

I kept restricting to years between 0 and 9999 because if we select years outside of this range the application crashes:

index.js:25572 Uncaught TypeError: `month` must be a valid moment object
    at getCalendarMonthWeeks (index.js:25572)
    at new CalendarMonth (index.js:13787)
    at constructClassInstance (react-dom.60482cc7.js:12604)
    at updateClassComponent (react-dom.60482cc7.js:14379)
    at beginWork (react-dom.60482cc7.js:15206)
    at performUnitOfWork (react-dom.60482cc7.js:17944)
    at workLoop (react-dom.60482cc7.js:17984)
    at HTMLUnknownElement.callCallback (react-dom.60482cc7.js:143)
    at Object.invokeGuardedCallbackDev (react-dom.60482cc7.js:193)
    at invokeGuardedCallback (react-dom.60482cc7.js:250)
react-dom.60482cc7.js:15873 The above error occurred in the <CalendarMonth> component:
    in CalendarMonth (created by withStyles(CalendarMonth))
    in withStyles(CalendarMonth) (created by CalendarMonthGrid)

If moment supports this extensive range of years, it is probably the component CalendarMonth from react-dates that contains some problem. Our only option would be to try to submit a patch in their repository.

Given that classic also does not supports years outside of this range I thought it would be ok to have this restriction.

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

I kind of went down a bit of a rabbit hole on this. 🙃

The problem is that moment only supports negative years if they conform to the JavaScript implementation of ISO 8601. That is, according to ISO 8601 any year that isn't a positive 4-digit year must use an agreed upon number of digits to pad the year field. JavaScript uses two padding digits, years must be represented as 6 digits. So, the 3rd of February in the year 2 BC must be written as -000001-02-03 (yes, 2 BC is the ISO 8601 year -1). Unfortunately, WordPress (and PHP in general) formats years in 4 digits, even the ones that should be 6 digits.

I tried converting the dates between WordPress-style and JavaScript-style when they were passed from the PostSchedule component to DateTime, then DateTime would use JavaScript-style internally. This mostly worked, but there was some weird behaviour in converting timezone offsets.

It's likely possible to use JavaScript-style dates entirely within Gutenberg, but that's a significantly larger change than is necessary for this PR. Fixing the problem properly would require a fairly large scale rewrite of WordPress' internal date handling.

Let's go ahead with this, and thank you for indulging me. 🙂

@jorgefilipecosta
Copy link
Member Author

Thank you @pento for going deep in this issue and sharing your findings. In that case, I will advance to merge this PR.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/allow-years-lower-than-1970-in-DateTime-component branch from ca1f3c6 to fb7f79a Compare February 4, 2019 12:25
@jorgefilipecosta jorgefilipecosta merged commit 8889254 into master Feb 4, 2019
@youknowriad youknowriad added this to the 5.1 (Gutenberg) milestone Feb 4, 2019
@youknowriad youknowriad deleted the fix/allow-years-lower-than-1970-in-DateTime-component branch February 4, 2019 13:09
daniloercoli added a commit that referenced this pull request Feb 5, 2019
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Make the modal title styling consistent (#13669)
  Disable navigation block for text mode. (#12185)
  Fix: Linting problem in modal example code (#13671)
  Add myself as a code owner to the annotations (#13672)
  Add more reviewers to CODEOWNERS.md file (#13667)
  Plugin: Remove jQuery heartbeat-to-hooks proxying (#13576)
  Workflow: Add repository CODEOWNERS file (#13604)
  Add a mobile minimum size for form fields (#13639)
  Update edit-save documentation  (#13578)
  Alt image setting (#13631)
  Fix: Allow years lower than 1970 in DateTime component. (#13602)
  Using addQueryArgs to generate Manage All Reusable Blocks link (#13653)
  Bump plugin version to 5.0.0-rc.1 (#13652)
  Update lodash to 4.17.10 (#13651)
  Refreshed PR (#9469)
  Set default values of the width and height input fields according to the actual image dimensions (#7687)
  12647 fix css color picker (#12747)
  Remove "we" from messages (#13644)
  Fix: Font size picker max width on mobile (#13264)
  Fix/issue 12501 menu item aria label
  ...
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
## Description
Fixes: #13579

This PR changes the DateTime component to allow years between 0 and 1970 that were previously unallowed.
The classic editor allows this set of years so we are updating the component to be equivalent.
Years lower than 0 and greater than 9999 are not allowed because JS error happens inside moment functions if these years are used. The classic editor and the quick edit form also don't allow this set of years.

## How has this been tested?
I checked I'm able to use years between  0 and 1970 on the publish date field with success
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
## Description
Fixes: #13579

This PR changes the DateTime component to allow years between 0 and 1970 that were previously unallowed.
The classic editor allows this set of years so we are updating the component to be equivalent.
Years lower than 0 and greater than 9999 are not allowed because JS error happens inside moment functions if these years are used. The classic editor and the quick edit form also don't allow this set of years.

## How has this been tested?
I checked I'm able to use years between  0 and 1970 on the publish date field with success
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants