[5.8] Correctly modify updated_at on custom pivot model

[mpyw]

This PR fixes #29321.

When we use default pivot, updated_at field is filled by InteractsWithPivotTable::addTimestampToAttachment() and the UPDATE query is directly executed on Eloquent Builder.

if (in_array($this->updatedAt(), $this->pivotColumns)) {
    $attributes = $this->addTimestampsToAttachment($attributes, true);
}

$updated = $this->newPivotStatementForId($this->parseId($id))->update(
    $this->castAttributes($attributes)
);

When we use custom pivot class, the behavior is quitely different. updated_at field is filled using Eloquent Model $timestamps feature. AsPivot::hasTimestampAttributes() dynamically determines $timestamps whether attributes from InteractsWithPivotTable::newPivot() contains created_at field.

$instance->timestamps = $instance->hasTimestampAttributes($attributes);

public function hasTimestampAttributes($attributes = null)
{
    return array_key_exists($this->getCreatedAtColumn(), $attributes ?? $this->attributes);
}

However, there is always no created_at attribute on updating. So we must set $timestamps after calling InteractsWithPivotTable::newPivot().

- $this->newPivot([
-     $this->foreignPivotKey => $this->parent->{$this->parentKey},
-     $this->relatedPivotKey => $this->parseId($id),
- ], true)->fill($attributes)->save();
- 
+ $pivot = $this->newPivot([
+     $this->foreignPivotKey => $this->parent->{$this->parentKey},
+     $this->relatedPivotKey => $this->parseId($id),
+ ], true);
+ 
+ $pivot->timestamps = in_array($this->updatedAt(), $this->pivotColumns);
+ 
+ $pivot->fill($attributes)->save();

[mpyw]

@staudenmeir Would you review this PR? thanks

[staudenmeir]

/cc @ralphschindler

[mpyw]

Rerunning job

[mpyw]

Umm...

[Composer\Downloader\TransportException]                                     
The "https://getcomposer.org/download/1.8.6/composer.phar" file could not be downloaded: failed to open stream: Connection timed out            

---

Still passed

[lk77]

For me it's fixed, my test project is now working fine.

[driesvints]

@mpyw it's best that you thoroughly describe what this pr is fixing (as noted in the PR template) in your first comment instead of linking to an issue.

[mpyw]

@driesvints Updated

[taylorotwell]

Does this contain breaking changes? It looks like it could since you were forced to change an existing test.

[mpyw]

No, it doesn't contain breaking changes. I just reused the existing test.
Also the changed test is newly created in #29362 and the PR changes are still not released.

[lk77]

It's more like a new test, a behaviour is now tested, and previously it was not. Perhaps it's best to add a new test and don't modify the existing one, to ensure that the old test and the new test passes.

[taylorotwell]

Please create an entirely new test without touching any existing tests. You can resubmit this when that is done.

[mpyw]

I was so busy that I couldn't fix it in this PR. I'll resubmit this, thanks

[driesvints]

@mpyw no worries. Just sent in a new pr 👍

[lk77]

Hello,

any news on this ?

thanks.

[mpyw]

@driesvints Which branch should I newly target?

[driesvints]

@mpyw 6.x I believe