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

BUGFIX: DateTime node property with defaultValue #4902

Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Feb 20, 2024

Resolves: #4901

Upgrade instructions

In case you already used defaultValue with a relative string like now, you have to manually migrate the events by hand.
Not doing so is not critical as the node property will still work, just a little unexpected as now will always be now.
If there is much demand for it, we can provide an optional migration to handle DateTime in initialProperties correctly.
But it is to be suspected that this feature was not yet used as its a niche thing.

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign marked this pull request as ready for review February 20, 2024 07:46
@mhsdesign mhsdesign changed the title BUGFIX: DateTime node property inconsistencies BUGFIX: DateTime node property with defaultValue Feb 22, 2024
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Does this only fix now or other relative strings?

@mhsdesign
Copy link
Member Author

Does this only fix now or other relative strings?

no this fixes EVERYTHING.
Every default value will be first deserialised via symphony serialiser and in the next second serialised. That converts everything into the correct representation.

the property should be a `DateTimeImmutable` and thus NOT mutable

- Add test for that `type: DateTime` is now a `DateTimeImmutable`
That will lead to relative `DateTime` values like `now` reflecting the actual time when the command was handled, rather glitching around and reflecting the date when being fetched.
…CreateNodeAggregateWithNode`

This is now part of the inner `handleCreateNodeAggregateWithNodeAndSerializedProperties` via
`SerializedPropertyValues::defaultFromNodeType`
With neos#4251 `NodeType::getDefaultValuesForProperties` doesnt return a `DateTime` anymore for dateTime default values
@mhsdesign mhsdesign force-pushed the bugfix/dateTimeNodePropertyInconsistencies branch from b9b843b to 41bc886 Compare March 1, 2024 21:44
@mhsdesign mhsdesign force-pushed the bugfix/dateTimeNodePropertyInconsistencies branch from ede965b to 3117ab0 Compare March 4, 2024 07:43
@mhsdesign mhsdesign merged commit a49ef7a into neos:9.0 Mar 4, 2024
9 checks passed
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.

9.0 DataTime property with string (defaultValue) like now
3 participants