Added fix to allow pivots to use classes on attach

[jaxwilko]

This PR ports some functionality from Laravel to allow for pivot models to be used during an attach call.

This means that events can now be processed on pivot models during the attach process.

[mjauvin]

Doesn't that require a bit more unit tests before merging?

[LukeTowers]

@mjauvin do we have one for the MorphToMany version of this? Merged it because @jaxwilko and I were both running into this issue and were able to confirm that this resolved the issue. Any concerns with it?

[mjauvin]

@LukeTowers @jaxwilko
My main concern is with the model.relation.beforeAttach event parameters being changed.

Any explanation for this?

[bennothommo]

I agree - that's a bit of a behaviour change for the event there.

[LukeTowers]

Hmm, I had thought that event was one of the recently added ones, although it looks like it has been around for a while, so I'll need to fix that.

[LukeTowers]

@bennothommo @mjauvin is this better: e6e3c5b?

[mjauvin]

@LukeTowers much better!

[LukeTowers]

Thanks for checking in on this guys :)

[ericp-mrel]

I just wanted to report after pulling in the latest commit from winter/storm the following code no longer works for me:

$model->bindEvent('model.relation.afterAttach', function (string $relationName, array $attachedIDList, array $insertData) use ($model) {
    // ...
}

Error:

[2022-10-03 18:03:34] local.ERROR: ArgumentCountError: Too few arguments to function AbstractModelRelationHandler::{closure}(), 2 passed and exactly 3 expected in

The relation it's failing on is a morphToMany.

Reverting back to commit 5c46aa3 allows my code to work again.

[LukeTowers]

@ericp-mrel can you toss a dd($eventArgs) on line 132 here: e6e3c5b#diff-2f413aba00b5b5f02604cf9c956287eb0656ab2ac79ef89ca9ee25ef6b324592R132 and report back the contents? The += might not be working correctly, might just have to switch to array_merge instead.

[ericp-mrel]

array:2 [ // vendor/winter/storm/src/Database/Relations/Concerns/BelongsOrMorphsToMany.php:133
  0 => "videos"
  1 => array:1 [
    0 => array:4 [
      "video_id" => 9
      "videoable_id" => 1
      "videoable_type" => "ra_community"
      "order" => 1
    ]
  ]
]

Here's the results when using array_merge

[ericp-mrel]

I think you'd have to use array_merge because of how the + operator works with arrays it will only add to the original array if their keys are different and the right-hand array will not override the left-hand one. Using the + operator with arrays is usually only useful with associative arrays in my experience because of how it behaves.

[LukeTowers]

Good point @ericp-mrel, I had forgotten that they weren't associative arrays. Since you reported the issue and tested the fix do you want to submit it as a PR to count towards Hacktoberfest for you?

[ericp-mrel]

@LukeTowers I can submit a PR, but one thing I'm really curious about why is the event signature different for custom pivot model vs non-custom pivot model? I guess I'm not sure why they need to be different for event purposes and wish I knew the reason for changing it.

[LukeTowers]

@ericp-mrel because the code that inserts the data with custom pivot models uses the unmodified values passed to the method and the code that inserts directly into the table uses the processed variables. Unfortunately when the events were originally added they were passed the processed variables instead of the raw ones, so we can't use just the raw variables for backwards compatibility reasons