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

timezones.phpt test fails with v8 6.7+ #378

Closed
sactor opened this issue Sep 17, 2018 · 3 comments
Closed

timezones.phpt test fails with v8 6.7+ #378

sactor opened this issue Sep 17, 2018 · 3 comments
Labels

Comments

@sactor
Copy link

sactor commented Sep 17, 2018

Starting with v8 6.7 timezone don't seem to be updated when TZ environment variable is changed.

In addition timezones.phpt is testing for old style toString() in which timezone is abbreviated.

The passing output of the test with v8 6.6 and below is:

Thu Mar 20 2014 11:03:24 GMT+0200 (EET)
Thu Mar 20 2014 05:03:24 GMT-0400 (EDT)
Thu Mar 20 2014 11:03:24 GMT+0200 (EET)

The failing output of the test with v8 6.7 and above is:

Thu Mar 20 2014 11:03:24 GMT+0200 (Eastern European Standard Time)
Thu Mar 20 2014 11:03:24 GMT+0200 (Eastern European Standard Time)
Thu Mar 20 2014 11:03:24 GMT+0200 (Eastern European Standard Time)
@sactor
Copy link
Author

sactor commented Nov 5, 2018

I attempted to fix the timezone refresh problem myself by using a similar approach as this: bnoordhuis/io.js@2eb2b97

I couldn't get it to work even with using tzset() before v8::Date::DateTimeConfigurationChangeNotification(c->isolate) in v8js_v8.cc. Does anyone have an idea why this used to work (the test passes with v8 6.6) but now fails? Abbreviation problem is secondary, it's important that it's possible to switch the timezone on the fly.

There are ways to work around the problem, but there is a lot of legacy code that depends on it working like it used to. This is blocking from moving above v8 6.6.

@treyhan
Copy link

treyhan commented Feb 9, 2019

I'm working on this issue.

  • small V8 and V8Js modifications are needed to get this fixed
  • review for the needed V8 modification is open
  • as soon as it gets merged in, I will create PR for the V8Js modifications

@stesie
Copy link
Member

stesie commented May 31, 2022

closed via #481

@stesie stesie closed this as completed May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants