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.

Sign up for GitHub

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

[5.4] Refactor pivot fix for PHP7.2 #20336

Merged
merged 2 commits into from
Jul 31, 2017
Merged

[5.4] Refactor pivot fix for PHP7.2 #20336

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
edb2690
refactor
laurencei Jul 31, 2017
bf7ae71
change count to empty
laurencei Jul 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public function toggle($ids, $touch = true)
'attached' => [], 'detached' => [],
];

$records = $this->formatRecordsList((array) $this->parseIds($ids));
$records = $this->formatRecordsList($this->parseIds($ids));

// Next, we will determine which IDs should get removed from the join table by
// checking which of the given ID/records is in the list of current records
Expand Down Expand Up @@ -93,7 +93,7 @@ public function sync($ids, $detaching = true)
)->all();

$detach = array_diff($current, array_keys(
$records = $this->formatRecordsList((array) $this->parseIds($ids))
$records = $this->formatRecordsList($this->parseIds($ids))
));

// Next, we will take the differences of the currents and given IDs and detach
Expand Down Expand Up @@ -211,7 +211,7 @@ public function attach($id, array $attributes = [], $touch = true)
// inserted the records, we will touch the relationships if necessary and the
// function will return. We can parse the IDs before inserting the records.
$this->newPivotStatement()->insert($this->formatAttachRecords(
(array) $this->parseIds($id), $attributes
$this->parseIds($id), $attributes
));

if ($touch) {
Expand Down Expand Up @@ -348,12 +348,14 @@ public function detach($ids = null, $touch = true)
// If associated IDs were passed to the method we will only delete those
// associations, otherwise all of the association ties will be broken.
// We'll return the numbers of affected rows when we do the deletes.
if (! is_null($ids = $this->parseIds($ids))) {
if (count((array) $ids) === 0) {
if (! is_null($ids)) {
$ids = $this->parseIds($ids);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 The only change of behavior here is for the case where you'd pass a Model instance that isn't saved yet (thus id = null).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... let me check.

What would a failing test for that look like? Would be good to capture it...

Copy link
Contributor Author

@laurencei laurencei Jul 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is that because your saying inside parseIds() that $value->getKey() might return null itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about use empty instead of count? Need to test if not gonna break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmsantos - nice idea - that would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@laurencei Yep that's exactly it however IMO that seems more like a bug than a feature so the change shouldn't matter... 😄


if (empty($ids)) {
return 0;
}

$query->whereIn($this->relatedKey, (array) $ids);
$query->whereIn($this->relatedKey, $ids);
}

// Once we have all of the conditions set on the statement, we are ready
Expand Down Expand Up @@ -455,12 +457,12 @@ public function withPivot($columns)
* Get all of the IDs from the given mixed value.
*
* @param mixed $value
* @return mixed
* @return array
*/
protected function parseIds($value)
{
if ($value instanceof Model) {
return $value->getKey();
return [$value->getKey()];
}

if ($value instanceof Collection) {
Expand All @@ -471,7 +473,7 @@ protected function parseIds($value)
return $value->toArray();
}

return $value;
return (array) $value;
}

/**
Expand Down