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

Allow RocketChatAttachment class accept DateTimeImmutable as timestamp argument #9

Conversation

antonkomarev
Copy link
Collaborator

@antonkomarev antonkomarev commented Feb 23, 2020

This PR adds failing tests for DateTimeImmutable timestamp. This need to be fixed.

@antonkomarev antonkomarev added the enhancement New feature or request label Feb 23, 2020
@antonkomarev antonkomarev force-pushed the allow-attachment-class-timestamp-method-accept-datetime-immutable branch from a9fa2ad to 58ac0d6 Compare February 23, 2020 06:24
@antonkomarev
Copy link
Collaborator Author

I think we could separate this method on 2 strict typed methods:

  • timestamp(DateTimeInterface $dateTime)
  • timestampFromString(string $dateTime)

@atymic
Copy link
Member

atymic commented Feb 23, 2020

It's pretty common for laravel to accept mixed types (eg, a date string or a DateTimeInterface).
I generally end up checking if it's a DateTimeInterface and falling back to parsing as a string.

@antonkomarev
Copy link
Collaborator Author

antonkomarev commented Feb 24, 2020

@atymic yeah, but DateTimeInterface doesn't have setTimezone method:
https://www.php.net/manual/en/class.datetimeinterface.php

@Funfare
Copy link
Collaborator

Funfare commented Feb 25, 2020

RocketChat also accepts DATE_ATOM timestamp, so I changed the code to using that. Then DateTimeInterface works fine

@antonkomarev
Copy link
Collaborator Author

antonkomarev commented Feb 25, 2020

I've replaced ATOM with RFC3339 because its name of the standard (their values are same).

@antonkomarev antonkomarev merged commit 689fee0 into master Mar 1, 2020
@antonkomarev antonkomarev deleted the allow-attachment-class-timestamp-method-accept-datetime-immutable branch March 1, 2020 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants