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

ChronosInterval #444

Open
dereuromark opened this issue Oct 26, 2023 · 4 comments
Open

ChronosInterval #444

dereuromark opened this issue Oct 26, 2023 · 4 comments

Comments

@dereuromark
Copy link
Member

dereuromark commented Oct 26, 2023

trigger_error(
'Since 2.4 ChronosInterval is deprecated. Use Chronos::createInterval() instead.',
E_USER_DEPRECATED
);

Why is that?
The suggested method doesnt exist in the code

I would recommend we keep this method as it is useful and better than using the native DateInternal class and its methods.
E.g. (string) casting to the native format is super important IMO.

I see that 3449eca dropped it as there were some issues with the native class and compatibility.

I have the same issue when trying to actually create any kind of improved wrapper around it.
Maybe instead of extending it, we could encapsulate it using the proxy pattern?

But according to e.g. https://github.com/pauci/datetime/blob/master/src/DateInterval.php it should work with extension.

@dereuromark
Copy link
Member Author

I sth like this viable?
2.x...2.x-interval

@markstory
Copy link
Member

I have the same issue when trying to actually create any kind of improved wrapper around it.
Maybe instead of extending it, we could encapsulate it using the proxy pattern?

Given how thorny datetime can be, I think the proxy/decorator pattern is the only good option we have. We could include a toNative() or toDateInterval method so that working with other libraries is reasonable.

@dereuromark
Copy link
Member Author

Downside is that the types on methods wouldnt work, if something expects a native one.

@markstory
Copy link
Member

Downside is that the types on methods wouldnt work, if something expects a native one.

Which is why we'd have a toNative() type method to provide compatibility with non-application code.

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

No branches or pull requests

2 participants