laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

Inconsistency between Collection::merge() and Collection::union() #1101

Open jeanlucnguyen opened 6 years ago

jeanlucnguyen commented 6 years ago

Description:

Inconsistency between Collection::merge() and Collection::union()

Steps To Reproduce:

collect(['a'])->union(collect(['b']))
=> Illuminate\Support\Collection {#1202
     all: [
       "a",
     ],
   }
collect(['a'])->merge(collect(['b']))
=> Illuminate\Support\Collection {#1205
     all: [
       "a",
       "b",
     ],
   }

From the documentation:

The union method adds the given array to the collection. If the given array contains keys that are already in the original collection, the original collection's values will be preferred

The merge method merges the given array or collection with the original collection. If a string key in the given items matches a string key in the original collection, the given items's value will overwrite the value in the original collection:

It seems that the union and merge should do the same, except that if the keys are similar in the 2 collections, then in the union method, the one from the original collection is kept, and in the merge method, the one from the added collection is kept.

Shouldn't we have this result?

collect(['a'])->merge(collect(['b']))
=> Illuminate\Support\Collection {#1205
     all: [
       "b",
     ],
   }

Also, I'm not sure if the 2 non associatives collections with index 0 are supposed to be considered as the same key or if 2 non associatives collections should just be added to each other. What is the expected behavior?

thomasruiz commented 6 years ago

This is the expected behaviour. Union and merge have different purposes.

Union basically loops, on B, checks if the key is in A. If it is, then skip, otherwise add it to A. Merge does something quite similar on associative arrays: loops on B, but upserts the value to A. For non-associatives arrays, it simply adds the values to the first array.

jeanlucnguyen commented 6 years ago

OK, so I have something that seems like an odd behaviour to me:

>>> SubscribedVoicePlanComponent::where('subscribed_product_id', 948711)->get()
=> Illuminate\Database\Eloquent\Collection {#18579
     all: [
       Facturation\SubscribedVoicePlanComponent {#40915
         subscribed_product_id: 948711,
         destinations: "[126,128]",
       },
       Facturation\SubscribedVoicePlanComponent {#40911
         subscribed_product_id: 948711,
         destinations: "[127]",
       },
     ],
   }
>>> SubscribedVoicePlanComponent::where('subscribed_product_id', 948711)->get()->merge(collect([]))
=> Illuminate\Database\Eloquent\Collection {#21065
     all: [
       Facturation\SubscribedVoicePlanComponent {#40910
         subscribed_product_id: 948711,
         destinations: "[127]",
       },
     ],
   }

Shouldn't I have 2 items in the second example?

One thing that might explain this behavior is that the SubscribedProductVoicePlanComponent model doesn't have a primary key, it's a one to many relationship with just a foreign_key from subscribed_product_vp_components.subscribed_product_id to subscribed_products.id

class SubscribedProduct extends Model
{
    public function voicePlanComponents()
    {
        return $this->hasMany('Facturation\SubscribedVoicePlanComponent');
    }
}

Is it the expected behavior?

jeanlucnguyen commented 6 years ago

Just tried something: I added to SubscribedVoicePlanComponent model the following properties:

protected $primaryKey = null;
public $incrementing = false;

Still the same behaviour, after the merge, it only returns 1 value out of 2

Then I added a primary key to the model, and the merge method returned 2 values.

imbrish commented 6 years ago

@jeanlucnguyen you just discovered feature of Illuminate\Database\Eloquent\Collection: it merges items based on unique model key not index in the array. It irritated me for ages. Use base Illuminate\Support\Collection and it will behave as expected.