Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Date refactorting to 5.5 #25

Merged
merged 4 commits into from
Sep 3, 2015
Merged

Date refactorting to 5.5 #25

merged 4 commits into from
Sep 3, 2015

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Jul 21, 2015

No description provided.

@Ocramius
Copy link
Member

Tests please!

@gianarb
Copy link
Contributor Author

gianarb commented Jul 21, 2015

thanks @Ocramius can you help me to write a good tests to coverage this feature?
it's already coverage IMO

@Ocramius
Copy link
Member

it's already coverage IMO

The particular path is not followed. You'd simply $this->getMock(DateTimeInterface::class) and work with that

@gianarb
Copy link
Contributor Author

gianarb commented Jul 21, 2015

@Ocramius
Copy link
Member

@gianarb but you are not testing that any implementation of DateTimeInterface passes the test.

@samsonasik
Copy link
Contributor

@gianarb thanks for this ;)

@gianarb
Copy link
Contributor Author

gianarb commented Jul 21, 2015

@Ocramius
Copy link
Member

@gianarb as it stands, you'd probably have to test with DateTime and DateTimeImmutable then. I raised the question at http://news.php.net/php.internals/87217

@gianarb
Copy link
Contributor Author

gianarb commented Jul 21, 2015

in this moment we have already wrote tests with DateTime, DateTime Immutable and stdClass

@samsonasik
Copy link
Contributor

@Ocramius tests updated by @gianarb ;)

@Maks3w
Copy link
Member

Maks3w commented Jul 22, 2015

This is pending of RFC with a proposal of Interface removal. This will take days or weeks.

@@ -127,8 +127,7 @@ public function isValid($value)
*/
protected function convertToDateTime($param, $addErrors = true)
{
// @TODO: when minimum dependency will be PHP 5.5, we can only keep check against DateTimeInterface
if ($param instanceof DateTime || $param instanceof DateTimeInterface) {
if ($param instanceof DateTimeInterface) {
Copy link
Member

Choose a reason for hiding this comment

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

As per discussion with the internal folks (sigh), this should probably be:

if ($param instanceof DateTime || $param instanceof DateTimeImmutable) {

According to internal folks, DateTimeInterface is not to be used.

@samsonasik
Copy link
Contributor

@Ocramius , @gianarb has updated to instanceof DateTimeImmutable ;)

@samsonasik
Copy link
Contributor

@Ocramius @gianarb I think the PR title can be updated

@gianarb gianarb changed the title DateTime instanceof DateTimeInterface for 5.5 Date refactorting to 5.5 Jul 25, 2015
@weierophinney weierophinney modified the milestones: 2.5.3, 2.4.8 Sep 3, 2015
@weierophinney weierophinney self-assigned this Sep 3, 2015
@weierophinney weierophinney merged commit efcb417 into zendframework:master Sep 3, 2015
weierophinney added a commit that referenced this pull request Sep 3, 2015
weierophinney added a commit that referenced this pull request Sep 3, 2015
weierophinney added a commit that referenced this pull request Sep 3, 2015
weierophinney added a commit that referenced this pull request Sep 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants