Skip to content

[5.8] fix MorphTo Relation ignores parent $timestamp when touching #28670

Merged
taylorotwell merged 5 commits intolaravel:5.8from
mx2s:relation-touch-ignores-timestamps
Jun 5, 2019
Merged

[5.8] fix MorphTo Relation ignores parent $timestamp when touching #28670
taylorotwell merged 5 commits intolaravel:5.8from
mx2s:relation-touch-ignores-timestamps

Conversation

@mx2s
Copy link
Copy Markdown
Contributor

@mx2s mx2s commented May 30, 2019

Fixed bug with touching model which has $timestamps set to false resulting in Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 no such column: updated_at

for source issue and more details see #28638

@mx2s mx2s changed the title [WIP] fix MorphTo Relation ignores parent $timestamp when touching [5.8] fix MorphTo Relation ignores parent $timestamp when touching May 30, 2019
@mx2s
Copy link
Copy Markdown
Contributor Author

mx2s commented May 30, 2019

@driesvints reopened #28658

@staudenmeir
Copy link
Copy Markdown
Contributor

staudenmeir commented Jun 1, 2019

We also have to cover cases where timestamps are enabled, but UPDATED_AT is null:

if (! $this->model->usesTimestamps() ||
is_null($this->model->getUpdatedAtColumn())) {
return $values;
}

{
$class = $class ?: static::class;

if (! get_class_vars($class)['timestamps']) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer (new $class)->usesTimestamps() to be consistent with existing code.

Copy link
Copy Markdown
Contributor Author

@mx2s mx2s Jun 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have to cover cases where timestamps are enabled, but UPDATED_AT is null:

just added test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staudenmeir in some cases we're getting abstract classes here for ex.

$this->assertFalse(Model::isIgnoringTouch());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but only in the tests. You can't get an abstract class in a real application. Please adjust the test accordingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but only in the tests. You can't get an abstract class in a real application.

OK, but If I use (new $class)->usesTimestamps() 6 more test are failing (not including mine) - what to do with that?
For example this one:

$this->assertFalse(Model::isIgnoringTouch());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move the new code to Relation::touch(). Model::isIgnoringTouch() is only about $ignoreOnTouch, not about timestamps.


public function testTouchingModelWithUpdatedAtNull()
{
$this->assertFalse(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case has to return true. You need to cover it by adjusting isIgnoringTouch().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: Max Kovalenko <mxss1998@yandex.ru>
@taylorotwell taylorotwell merged commit 26a22a2 into laravel:5.8 Jun 5, 2019
@mx2s mx2s deleted the relation-touch-ignores-timestamps branch July 26, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants