Skip to content

Adding ability to create a DateTime or DateTimeImmutable from an inst… #5016

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

Closed

Conversation

mikeSimonson
Copy link
Contributor

…ance of the interface

@nikic nikic requested a review from derickr December 16, 2019 13:56
@nikic
Copy link
Member

nikic commented Dec 16, 2019

+1 on these methods. Ideally we'd have just added these instead of createFromMutable+createFromImmutable.

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'm OK with this idea, as Mike and I discussed this recently.

@mikeSimonson
Copy link
Contributor Author

@derickr @nikic How can I run the test only for certain phpt files with as much information as possible related to a failure ?

@derickr
Copy link
Member

derickr commented Dec 16, 2019

@mikeSimonson TESTS=ext/date/foo.phpt make test and if it fails, there should be a ext/date/foo.sh file which contains the full line that was used to execute that test

@mikeSimonson mikeSimonson force-pushed the datetime-create-from-interface branch from fbcad83 to fea4392 Compare December 22, 2019 00:56
@mikeSimonson mikeSimonson force-pushed the datetime-create-from-interface branch from fea4392 to 2bbf3d4 Compare December 22, 2019 08:37
@mikeSimonson
Copy link
Contributor Author

As far as I can tell this PR is ready tested and respect the CS ?
Any other issue to fix ?

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks good to me, but let's wait until Derick has had a look at this as well.

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 please fix the two nits before merging.

int(1)
["timezone"]=>
string(6) "+01:00"
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please make sure the last line also has a new line at the end (but not an extra empty line)

int(1)
["timezone"]=>
string(6) "+01:00"
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please make sure the last line also has a new line at the end (but not an extra empty line)

@cmb69
Copy link
Member

cmb69 commented Jan 3, 2020

It seems to me that this PR obsoletes PR #3394.

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

Successfully merging this pull request may close these issues.

5 participants