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

Replace microtime() usages with hrtime() #2242

Merged
merged 6 commits into from Apr 15, 2019
Merged

Replace microtime() usages with hrtime() #2242

merged 6 commits into from Apr 15, 2019

Conversation

Dzhuneyt
Copy link
Contributor

@Dzhuneyt Dzhuneyt commented Jan 3, 2019

Fixes #2214

There are 5 test failures, but they are not caused by this PR. They are failing on master as well.

@GrahamCampbell
Copy link
Member

Would be better to declare a guzzle specific time function in https://github.com/guzzle/guzzle/blob/master/src/functions.php, instead of copy-pasting this code all over the place.

@Dzhuneyt
Copy link
Contributor Author

Dzhuneyt commented Jan 4, 2019

Introduced a new wrapper function \GuzzleHttp\current_time().

src/functions.php Show resolved Hide resolved
@gmponos
Copy link
Member

gmponos commented Jan 7, 2019

I am not sure why the discussion is resolved.. but unless if guzzle maintainers say otherwise I still insist on my proposal to mark the function current_time as internal and have it with underscore as _current_time... In the same way that it is done here, here and here... we can wait feedback from someone.

@gmponos
Copy link
Member

gmponos commented Jan 7, 2019

The reason suggesting the underscore is only for consistency with the rest of them.. in general I am not in favor of it.. but none the less I believe that it should be marked as internal in order not to have it under BC promise.

@Llbe
Copy link

Llbe commented Jan 9, 2019

It must be 1e9 (nano) and not 1e7, please see https://3v4l.org/OidWu.

@Llbe
Copy link

Llbe commented Jan 9, 2019

I realized hrtime(true) will return a float on 32-bit systems. So it must be divided only if it's an int if I'm not mistaken? Or use hrtime(false) and handle the array.

I saw that the change to 1e7 pointed to https://3v4l.org/bZ1uo but I think the mistake is that hrtime isn't a UNIX timestamp, it's just a value. So they cannot be compared like that.

@Nyholm
Copy link
Member

Nyholm commented Jan 10, 2019

Thank you @Llbe. You are correct. One must never mix hrtime and microtime.

1e9 is correct. My bad.

@Dzhuneyt
Copy link
Contributor Author

@Nyholm reverted back to 1e9. Let me know if further changes are needed or we can merge.

@Nyholm
Copy link
Member

Nyholm commented Jan 10, 2019

Thank you.

I see that you have not resolved @gmponos concern yet. If we mark the function as internal we can easily change it later if we want to. Say that we want to introduce superSpecificTime() that is available in PHP7.4. If we dont mark it as internal there is no BC way to introduce that. Also, it will be impossible to drop microtime().

@Dzhuneyt
Copy link
Contributor Author

I see. Marked is as @internal now.

@Dzhuneyt
Copy link
Contributor Author

@Nyholm is there something else that stops these changes from being "mergeable"?

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Last thing then Im happy to merge.

* @return float|mixed UNIX timestamp
* @internal
*/
function current_time()
Copy link
Member

Choose a reason for hiding this comment

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

One minor thing. All our other internal methods starts with an underscore. Please rename to _current_time()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed.

@Nyholm

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@Nyholm Nyholm merged commit 3b0452a into guzzle:master Apr 15, 2019
@GrahamCampbell
Copy link
Member

Symfony has a polyfill for the hrtime function, actually. Maybe it would be better to use that.

@wirwolf
Copy link

wirwolf commented Oct 18, 2019

Hello @Dzhuneyt, i create a fork of this repo and release this marge request into the new version(6.4.0).

Namespace compatibility is 100%, and you can use my changes if you patch composer.json in your projects

  • replace "guzzlehttp/guzzle": "^6.3" to "someblackmagic/guzzle": "^6.4"
  • add "guzzlehttp/guzzle": "6.3.*" into replace block

Fork link: https://github.com/SomeBlackMagic/guzzle/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 7.3: use hrtime() instead of microtime()
6 participants