Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Date Period Iteration #227

Closed
wants to merge 2 commits into from
Closed

Date Period Iteration #227

wants to merge 2 commits into from

Conversation

rodnaph
Copy link

@rodnaph rodnaph commented Jul 10, 2020

This test-case is a bit indirect, but it looks like doing iteration with a \DatePeriod object messes up the internal state (so calling any methods that depend on it will fail, format in this example).

I don't have a fix... suggestions?

@Kharhamel
Copy link
Collaborator

Sorry i didn't see your PR until now since there was no issue attached ot it. I ll look into it.

@Kharhamel
Copy link
Collaborator

Kharhamel commented Aug 20, 2020

Ok, when we create a dateperiod the start and end safedates get their innerDateTime set to null, which obviously breaks everything.

I have no idea how to fix this since I cannot debug the DatePeriod. I need to understand how DatePeriod operates internally. @moufmouf @Girgias do you have an idea?

@Girgias
Copy link

Girgias commented Aug 20, 2020

Right, so I don't even know if this is fixable for PHP 7.x as it defines a custom internal iterator see:

The issue is those are not publicly available methods because it doesn't implement the Iterator or IteratorAggregate interface, so if you need to make a custom object to fix this issue it won't be usable out of the box even if you extend DatePeriod.
However this is not the case for PHP 8.0 as there has been some internal changes and the Zend engine provides a way for extension to easily make objects iterable using an InternalIterator which you can get by calling getIterator().

So for PHP 7.x it's or Wont Fix or you will need to implement a custom iterator on the object.

@moufmouf
Copy link
Member

The more I look at it, the more I think this is a bug in the DatePeriod implementation.

The DatePeriod tries to return classes of the same type as StartDate (or EndDate).
So when DatePeriod sees a \Safe\DateTimeImmutable, in the iterator, it tries to instantiate instances of the \Safe\DateTimeImmutable class.
But when you think about it, that makes no sense at all.

How could DatePeriod properly fill custom private properties of \Safe\DateTimeImmutable?
The only solution would be to pass to DatePeriod a factory, but DatePeriod does not work that way.

I believe the instances returned by DatePeriod should always be DateTime or ImmutableDateTime, but never a subclass.

We should probably file a bug in PHP regarding this behaviour.

@Kharhamel
Copy link
Collaborator

Kharhamel commented Sep 1, 2020

Yes, I think the only short term solution is to add methods to get a regular DatetimeImmutable from the safe version and vice-versa, and always to use the regular version with DatePeriod. The real question now is: how to enforce this behavior? Could we edit the phpstan rules for example?

See #229

@moufmouf
Copy link
Member

moufmouf commented Sep 2, 2020

If I understand correctly, you mean:

  • creating a Safe\DatePeriod class
  • when iterating over this class, we are getting Safe\DateTime or Safe\DateTimeImmutable

The question is: can we override the "internal iterator" implemented by \DatePeriod with a custom iterator? @Girgias seems to think this will not work:

The issue is those are not publicly available methods because it doesn't implement the Iterator or IteratorAggregate interface, so if you need to make a custom object to fix this issue it won't be usable out of the box even if you extend DatePeriod.

@Kharhamel you can still give it a shot, but there are chances that it won't work.

@Kharhamel
Copy link
Collaborator

No, not at all. I mean that we should use the regular DatePeriod and always give them regular DatetimeImmutable, which means we means methods to easily switch between regular and safe versions.
And it would be great if there was a way to enforce that way of doing

@Kharhamel
Copy link
Collaborator

The workarounds and the bug test were released in v1.2.0. I will close this PR and open an issue.

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

Successfully merging this pull request may close these issues.

4 participants