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

Expand Time for interface #4633

Merged
merged 3 commits into from
May 1, 2021
Merged

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented May 1, 2021

Description
Many libraries use DateTimeInterface to allow DateTime or DateTimeImmutable. In a few cases in Time our functions are already compatible with either yet the docblocks limit it to DateTime. This PR expands the use for the interface, with one deprecation for an explicit method type.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@iRedds
Copy link
Collaborator

iRedds commented May 1, 2021

I suggest changes within this PR.
Replace this code:

if ($time instanceof Time)
{
$time = $time->toDateTime()
->setTimezone(new DateTimeZone('UTC'));
}
elseif ($time instanceof DateTime)
{
$time = $time->setTimezone(new DateTimeZone('UTC'));
}
elseif (is_string($time))
{
$timezone = $timezone ?: $this->timezone;
$timezone = $timezone instanceof DateTimeZone ? $timezone : new DateTimeZone($timezone);
$time = new DateTime($time, $timezone);
$time = $time->setTimezone(new DateTimeZone('UTC'));
}

On cleaned of duplication.

 if ($time instanceof Time) 
 { 
 	$time = $time->toDateTime(); 
 } 
 elseif (is_string($time)) 
 { 
 	$timezone = $timezone ?: $this->timezone; 
 	$timezone = $timezone instanceof DateTimeZone ? $timezone : new DateTimeZone($timezone); 
 	$time     = new DateTime($time, $timezone); 
  } 

 if ($time instanceof DateTimeInterface) 
 { 
 	$time = $time->setTimezone(new DateTimeZone('UTC')); 
 } 

@MGatner
Copy link
Member Author

MGatner commented May 1, 2021

I did it like this because DateTimeInterface doesn't actually have a setTimeZone() method. It might be okay to assume it since the interface is internal:

It is not possible to implement this interface with userland classes.

I'm not sure if PHPStan would complain though.

@iRedds
Copy link
Collaborator

iRedds commented May 1, 2021

This is just a suggestion and I'm not really against using your condition.

$time instanceof DateTime || $time instanceof DateTimeImmutable

The main thing is to get rid of duplication.

@MGatner
Copy link
Member Author

MGatner commented May 1, 2021

Got it! If you can add it as an inline suggestion I can accept it on mobile, otherwise it will need to wait until I'm back on desktop.

@MGatner
Copy link
Member Author

MGatner commented May 1, 2021

@iRedds I think I got that taken care of.

@MGatner MGatner merged commit 7742812 into codeigniter4:develop May 1, 2021
@MGatner MGatner deleted the time-instance branch May 1, 2021 17:03
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.

3 participants