fix bug 63392 #224

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
@xuzhi

xuzhi commented Oct 31, 2012

i delete these code if(time->relative.weekday == 0)time->relative.weekday == 7; these code cause bug 63392 , bug test script blow

modify("Sunday this week"); var_dump($dt->format('r')); ## Expected result: string(31) "Sun, 13 May 2012 00:00:00 +0000" ## Actual result: string(31) "Sun, 20 May 2012 00:00:00 +0000"

xuzhi added some commits Oct 30, 2012

Update ext/date/lib/tm2unixtime.c
fix  Bug #63392  由于 我是第一次参与php源码的工作,有很多东西不太清楚。这个bug  不知道原始的作者
是怎么考虑的,会出现那么奇怪的赋值。
Update ext/date/lib/tm2unixtime.c
fix bug #63392 old code is if(time->relative.weekday == 0)time->relative.weekday == 7
i don't konw  why author write this code
@laruence

This comment has been minimized.

Show comment Hide comment
@laruence

laruence Oct 31, 2012

Member
  1. 如果你要删除这段代码, 就整个删除吧
  2. 合并成一个提交, 方便我们看到你到底改了什么
  3. 写一个phpt 测试文件, 至于怎么写phpt, 我想你可以根据现有的照猫画图.

thanks :)

Member

laruence commented Oct 31, 2012

  1. 如果你要删除这段代码, 就整个删除吧
  2. 合并成一个提交, 方便我们看到你到底改了什么
  3. 写一个phpt 测试文件, 至于怎么写phpt, 我想你可以根据现有的照猫画图.

thanks :)

@xuzhi

This comment has been minimized.

Show comment Hide comment
@xuzhi

xuzhi Oct 31, 2012

ok ,由于刚开始 参与,比较生疏。

------------------ 原始邮件 ------------------
发件人: "Xinchen Hui"notifications@github.com;
发送时间: 2012年10月31日(星期三) 下午4:52
收件人: "php/php-src"php-src@noreply.github.com;
抄送: "xuzhi"767994038@qq.com;
主题: Re: [php-src] fix bug 63392 (#224)

如果你要删除这段代码, 就整个删除吧
合并成一个提交, 方便我们看到你到底改了什么
写一个phpt 测试文件, 至于怎么写phpt, 我想你可以根据现有的照猫画图.

thanks :)


Reply to this email directly or view it on GitHub.

xuzhi commented Oct 31, 2012

ok ,由于刚开始 参与,比较生疏。

------------------ 原始邮件 ------------------
发件人: "Xinchen Hui"notifications@github.com;
发送时间: 2012年10月31日(星期三) 下午4:52
收件人: "php/php-src"php-src@noreply.github.com;
抄送: "xuzhi"767994038@qq.com;
主题: Re: [php-src] fix bug 63392 (#224)

如果你要删除这段代码, 就整个删除吧
合并成一个提交, 方便我们看到你到底改了什么
写一个phpt 测试文件, 至于怎么写phpt, 我想你可以根据现有的照猫画图.

thanks :)


Reply to this email directly or view it on GitHub.

@nikic

This comment has been minimized.

Show comment Hide comment
@nikic

nikic Oct 31, 2012

Member

Would be nice to keep PR comments in English so other people can read them too ;)

Member

nikic commented Oct 31, 2012

Would be nice to keep PR comments in English so other people can read them too ;)

@alkavan

This comment has been minimized.

Show comment Hide comment
@alkavan

alkavan Oct 31, 2012

Ya... try English please. I actually read that one on gmail, and instant translate option was available. but this is not the case for everyone :-)

alkavan commented Oct 31, 2012

Ya... try English please. I actually read that one on gmail, and instant translate option was available. but this is not the case for everyone :-)

@xuzhi

This comment has been minimized.

Show comment Hide comment
@xuzhi

xuzhi Nov 1, 2012

I had commit bug63392.phpt in /php-src/ext/date/tests/ i had delete " (time->relative.weekday==0) time->relative.weekday==7 ;" in tm2unixtime.c , these code cause bug63392.

xuzhi commented Nov 1, 2012

I had commit bug63392.phpt in /php-src/ext/date/tests/ i had delete " (time->relative.weekday==0) time->relative.weekday==7 ;" in tm2unixtime.c , these code cause bug63392.

@@ -0,0 +1,21 @@
+--TEST--
+Bug #63392 (DateTime::modify() start of week inconsistency)
+Description:

This comment has been minimized.

Show comment Hide comment
@reeze

reeze Nov 1, 2012

Contributor

You could use DESCRIPTION Section like this:
https://github.com/php/php-src/blob/master/Zend/tests/bug60825.phpt#L1

@reeze

reeze Nov 1, 2012

Contributor

You could use DESCRIPTION Section like this:
https://github.com/php/php-src/blob/master/Zend/tests/bug60825.phpt#L1

This comment has been minimized.

Show comment Hide comment
@xuzhi

xuzhi Nov 1, 2012

I already corrent the mistake that you said above.

@xuzhi

xuzhi Nov 1, 2012

I already corrent the mistake that you said above.

This comment has been minimized.

Show comment Hide comment
@reeze

reeze Nov 1, 2012

Contributor

Here, I mean you could move the description to a new section, or it will output with the test name, that looks ugly :)

--DESCRIPTION--
......

@reeze

reeze Nov 1, 2012

Contributor

Here, I mean you could move the description to a new section, or it will output with the test name, that looks ugly :)

--DESCRIPTION--
......

ext/date/tests/bug63392.phpt
+
+$dt = new DateTime("2012-05-13");
+$dt->modify("Sunday this week");
+var_dump($dt->format('r')

This comment has been minimized.

Show comment Hide comment
@reeze

reeze Nov 1, 2012

Contributor

seems you didn't try to test it, here miss a ';)', please make sure the tests passes before push :)

@reeze

reeze Nov 1, 2012

Contributor

seems you didn't try to test it, here miss a ';)', please make sure the tests passes before push :)

This comment has been minimized.

Show comment Hide comment
@xuzhi

xuzhi Nov 1, 2012

i test it already,but i paste these code from vim,maybe miss copy ";" .i

@xuzhi

xuzhi Nov 1, 2012

i test it already,but i paste these code from vim,maybe miss copy ";" .i

+$dt->modify("Sunday this week");
+var_dump($dt->format('r'));
+?>
+--EXPECT--

This comment has been minimized.

Show comment Hide comment
@reeze

reeze Nov 1, 2012

Contributor

The output is timezone dependent, you could add a timezone section, eg:

--INI--
date.timezone=UTC
--FILE--
<?php
....

on my machine test failed before set timezone:

001+ string(31) "Sun, 13 May 2012 00:00:00 +0800"
001- string(31) "Sun, 13 May 2012 00:00:00 +0000"
@reeze

reeze Nov 1, 2012

Contributor

The output is timezone dependent, you could add a timezone section, eg:

--INI--
date.timezone=UTC
--FILE--
<?php
....

on my machine test failed before set timezone:

001+ string(31) "Sun, 13 May 2012 00:00:00 +0800"
001- string(31) "Sun, 13 May 2012 00:00:00 +0000"

This comment has been minimized.

Show comment Hide comment
@xuzhi

xuzhi Nov 1, 2012

php's version that you used is error plesae use the last version. php 5.3.x will cause timezone dependent

@xuzhi

xuzhi Nov 1, 2012

php's version that you used is error plesae use the last version. php 5.3.x will cause timezone dependent

This comment has been minimized.

Show comment Hide comment
@reeze

reeze Nov 1, 2012

Contributor

I use PHP5.4-lastest and test should pass in all currently maintained version :)

@reeze

reeze Nov 1, 2012

Contributor

I use PHP5.4-lastest and test should pass in all currently maintained version :)

This comment has been minimized.

Show comment Hide comment
@xuzhi

xuzhi Nov 1, 2012

my mistake ,you are right,many php version on my PC,i confuse the version,sorry.

@xuzhi

xuzhi Nov 1, 2012

my mistake ,you are right,many php version on my PC,i confuse the version,sorry.

This comment has been minimized.

Show comment Hide comment
@xuzhi

xuzhi Nov 1, 2012

already add ini date.timezone=UTC.

@xuzhi

xuzhi Nov 1, 2012

already add ini date.timezone=UTC.

@laruence

This comment has been minimized.

Show comment Hide comment
@laruence

laruence Nov 2, 2012

Member

the codes you removed seems a intentional logic, so I really doubt this fix.
so RM, please don't merge this, leave it to the author

Member

laruence commented Nov 2, 2012

the codes you removed seems a intentional logic, so I really doubt this fix.
so RM, please don't merge this, leave it to the author

@lstrojny

This comment has been minimized.

Show comment Hide comment
@lstrojny

lstrojny Nov 4, 2012

Contributor

Ping @derickr

Contributor

lstrojny commented Nov 4, 2012

Ping @derickr

@derickr

This comment has been minimized.

Show comment Hide comment
@derickr

derickr Nov 4, 2012

Contributor

Laruence had pinged me about this on IRC, and I would need to re-double check if this is broken in the first place. I am doubting that at the moment, but can't find my copy of my book :-)

Contributor

derickr commented Nov 4, 2012

Laruence had pinged me about this on IRC, and I would need to re-double check if this is broken in the first place. I am doubting that at the moment, but can't find my copy of my book :-)

@lstrojny

This comment has been minimized.

Show comment Hide comment
@lstrojny

lstrojny Jan 14, 2013

Contributor

@derickr ping again :)

Contributor

lstrojny commented Jan 14, 2013

@derickr ping again :)

@cmbuckley

This comment has been minimized.

Show comment Hide comment
@cmbuckley

cmbuckley Jan 14, 2013

For anyone who's interested, the change was introduced in 357292a.

For anyone who's interested, the change was introduced in 357292a.

@dsp

This comment has been minimized.

Show comment Hide comment
@dsp

dsp Sep 16, 2013

Member

ping @derickr

Member

dsp commented Sep 16, 2013

ping @derickr

@smalyshev smalyshev added the Bugfix label Nov 24, 2014

@php-pulls

This comment has been minimized.

Show comment Hide comment
@php-pulls

php-pulls Aug 7, 2016

Comment on behalf of cmb at php.net:

This PR is obsolete, as the issue has already been fixed with commit f43f6fc.

Comment on behalf of cmb at php.net:

This PR is obsolete, as the issue has already been fixed with commit f43f6fc.

@php-pulls php-pulls closed this Aug 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment