SlackAttachment::timestamp() not setting date correctly
ainehickey opened this issue · 2 comments
- Slack Notification Channel Version: 2.3.0
- Laravel Version: 7.0
- PHP Version: 7.4.13
- Database Driver & Version:
Description:
To set a Slack message timestamp, I used Illuminate\Support\InteractsWithTime\SlackAttachment::timestamp(time());
This makes sense. It sets the timestamp, or so the phpdoc says... But in reality it doesn't.
I just sent a message today (20 Feb 2021), but Slack states that it was sent on 20 Feb 2072.
Let's look at the timestamp() method, in Illuminate\Notifications\Messages:
/**
* Set the timestamp.
*
* @param \DateTimeInterface|\DateInterval|int $timestamp
* @return $this
*/
public function timestamp($timestamp)
{
$this->timestamp = $this->availableAt($timestamp);
return $this;
}
This code calls Illuminate\Support\InteractsWithTime:: availableAt($delay):
/**
* Get the "available at" UNIX timestamp.
*
* @param \DateTimeInterface|\DateInterval|int $delay
* @return int
*/
protected function availableAt($delay = 0)
{
$delay = $this->parseDateInterval($delay);
return $delay instanceof DateTimeInterface
? $delay->getTimestamp()
: Carbon::now()->addRealSeconds($delay)->getTimestamp();
}
So what's happening is my time stamp of now is being added as a delay, and turned into the year 2072.
Steps To Reproduce:
Create a new Slack Attachement and use the timestamp() method.
I'm happy to submit a fix, by removing the calling of availableAt() method. I'm just wondering if there is some more logic needed here too?
What are you trying to achieve? I don't think changing the timestamp
function so that it no longer accepts a delay is a good idea. At most the timestamp docblock should clarify that if you pass in an int
it will be used as a delay instead of a timestamp.
If you pass in a DateTimeInterface
(like a Carbon instance) instead of an integer it should work fine.
@ainehickey you're correct, this currently does not do what it says it does. However, we can't change this behavior on the current stable release as that would introduce a breaking change for the people expecting the current behavior.
So I follow what @nathanheffley suggest and tried to clarify the method description and param here: #47
You can still set the actual timestamp integer by setting the $timestamp
property directly:
$attachment->timestamp = $timestamp;