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

Added DateTime[Immutable]::[get|set]Microseconds #12557

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

marc-mabe
Copy link
Contributor

Follow up of #12413 to manage handling microseconds without precision loss or stringifying/parsing from string.

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

I am a little on the fence about the API here.

How often do you need to set only microseconds? Should, instead of adding setMicroseconds, this be a second optional argument on setTimestamp?

And of course, setTime() already has the microsecond argument too.

We don't have any setters for any of the other single date/time fields either.

ext/date/php_date.c Show resolved Hide resolved
ext/date/php_date.c Outdated Show resolved Hide resolved
@marc-mabe
Copy link
Contributor Author

Hi @derickr,

I am a little on the fence about the API here.

How often do you need to set only microseconds?

There are two different cases based on what to create a DateTime object from:

  1. Parsing from string
  2. Instantiating from another computer time source

The second case might by another DateTime object, which is covered by createFrom[Interface|Mutable] but else you are basically forced to stringify and parse instead. Most of the time you have something as simple as unix timestamps evtl. together with fractions in whatever precision.
#12413 should already address most cases.
With this PR I'm trying to improve even more to also cover cases for passing timestamps and fractions part around.
-> DateTime::createFromTimestamp($ts)->setMicroseconds($usec)

Should, instead of adding setMicroseconds, this be a second optional argument on setTimestamp?

For me there are to reasons here to not add a second argument to setTimestamp:

  1. Setter would have additional feature over getTimestamp but this can not simply done on a getter
  2. Similar to createFromTimestamp(int|float $ts) I would like to allow float for setTimestamp as well which on the other hand opens questions on handling fractions part twice (e.g. setTimestamp(1.1, 1))

And of course, setTime() already has the microsecond argument too.

With setTime() you can't set the microseconds without knowing the hour, minute and second in your TZ as well which might be cumbersome.

We don't have any setters for any of the other single date/time fields either.

Personally I would like to have other getters and setters as well but this is definitively out of scope of this PR and might be better maintainable for libs like Carbon. Not sure.
For now I'm focusing on improving the case of passing times around different sources / targets.

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

You have me convinced with the API :-)

However, we don't have any tests in this PR yet! And these we need.

@marc-mabe
Copy link
Contributor Author

@derickr

You have me convinced with the API :-)

nice :)

However, we don't have any tests in this PR yet! And these we need.

True, now added

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

One nit about the test file, and after my earlier merge this now has a conflict. So could you please rebase (and feel free to force-push to your personal branch) and fix the conflicts?

ext/date/tests/getSetMicroseconds.phpt Outdated Show resolved Hide resolved
@marc-mabe marc-mabe force-pushed the DateTime-getsetMicroseconds branch from 44f9651 to 6bf195e Compare November 25, 2023 12:46
@marc-mabe
Copy link
Contributor Author

@derickr rebased and fixed your comments now

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

LGTM, but trying to sort out the test failure before merging.

@marc-mabe marc-mabe force-pushed the DateTime-getsetMicroseconds branch from 6bf195e to aa19dd1 Compare December 20, 2023 13:26
@marc-mabe
Copy link
Contributor Author

marc-mabe commented Dec 20, 2023

@derickr This was unrelated - Just rebased again and now everything is green 🎉

@marc-mabe
Copy link
Contributor Author

Also use zend_argument_error as suggested by @iluuu1994 here #12775 (comment) and added NEWS and UPGRADING entry.

@vudaltsov
Copy link
Contributor

Just an idea. What if we add new features to DateTimeImmutable only? That would encourage people to stop using mutable DateTime.

@marc-mabe
Copy link
Contributor Author

Introducing inconsistency isn't making anything better. If you want to see the mutable class go away, an RFC deprecating it would be a better way and if you are on it deprecating the function interface would be something I support.

@vudaltsov
Copy link
Contributor

@marc-mabe, yes, agree.

@derickr, I remember your tweet about datetime deprecations. Have you decided anything?

@marc-mabe marc-mabe force-pushed the DateTime-getsetMicroseconds branch from 9b5bc67 to 55204bd Compare February 2, 2024 18:56
@marc-mabe marc-mabe requested a review from kocsismate as a code owner February 2, 2024 18:56
@marc-mabe marc-mabe force-pushed the DateTime-getsetMicroseconds branch from 55204bd to 7316d8c Compare February 2, 2024 20:34
@marc-mabe
Copy link
Contributor Author

Rebased the PR. The test failure is unrelated due to #13299.

@derickr @iluuu1994 can this PR be merged now?

@derickr derickr merged commit 0016b30 into php:master Feb 5, 2024
9 of 10 checks passed
@marc-mabe marc-mabe deleted the DateTime-getsetMicroseconds branch February 5, 2024 16:08
@@ -3844,12 +3844,76 @@ PHP_METHOD(DateTimeImmutable, setTimestamp)
}
/* }}} */

/* {{{ */
PHP_METHOD(DateTimeImmutable, setMicroseconds)
Copy link
Member

Choose a reason for hiding this comment

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

I know I'm late to the party, but the 2 methods should share the same implementation, shouldn't they? Only ~2-3 lines differ, which could be handled easily one way or another.

ext/date/php_date.stub.php Show resolved Hide resolved
fabpot added a commit to symfony/symfony that referenced this pull request Feb 23, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[Clock] Add get/setMicroseconds()

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | -
| License       | MIT

Polyfilling php/php-src#12557

Commits
-------

3e5b677 [Clock] Add get/setMicroseconds()
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