laravel/slack-notification-channel

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;