Skip to content

Conversation

sileence
Copy link
Contributor

@sileence sileence commented Apr 22, 2020

Allow a developer to get a sub-collection from an array of arrays:

$data = collect([
    'users' => [
        [
            'name' => 'Person 1',
            'email' => 'sample@example.com',
            'is_admin' => true,
        ]
    ],
    'settings' => [
         'registrations' => '1',
         'burn_server' => '1',
    ],
]);

$settings = $data->get('settings'); // returns a boring array 😞 
$settings = $data->pack('settings'); // returns a collection 👍 
$settings = $data->pack('settings', ['registrations']);
// returns a collection the specified keys 💃 

$users = $data->get('users'); // returns a plain array of arrays 😭 
$users = $data->packAll('users'); // returns a collection of collections 😄  
$users = $data->packAll('users', ['name', 'email']);
// returns a collection of collections with specific keys 🕺   

Following #32379

This PR add two methods that should be very useful when dealing with nested validated data.

For example:

Assuming input looks like this:

$response = $this->post('/validate', [
    'users' => [
        [
            'name' => 'Person 1',
            'email' => 'sample@example.com',
            'is_admin' => true,
        ]
    ],
    'settings' => [
         'registrations' => '1',
         'burn_server' => '1',
    ],
]);

and validation looks like this:

$data = $request->validate([
        'users' => ['required', 'array'],
        'users.*' => ['required', 'array'],
        'users.*.name' => ['required'],
        'users.*.email' => ['required', 'email'],
        'users.*.password' => ['required', 'min:8'],
        'settings' => ['required', 'array:registrations'],
]);

$data will contain a Validated object that extends Collection but if we call get like this:

$settings = $data->get('settings') then $settings will be a plain array that cannot be passed to a model without $fillable.

But calling $data->pack('settings') will "pack" the underlying array as another instance of Validated that can be passed to a Setting model without fillable.

Furthermore, calling $data->pack('settings', ['registrations', '...']); will pack a new collection with whitelisted keys only.

Calling $data->packAll('users') will get a Validated collection containing Validated collections with users data.

Calling $data->packAll('users', ['name', 'email', 'password']) will get a Validated collection containing Validated collections of user data with those 3 attributes only.

The developer can choose whether to add more strict validation with:

'users.*' => ['required', 'array:name,email,password'], (#32452)

or selecting specific keys just before "packing" the data to pass it to an Eloquent model or even to different Eloquent models!

@gocanto
Copy link
Contributor

gocanto commented Apr 23, 2020

yes, this is pretty cool @sileence - thus, we do not have to be calling new Collection inside loops

@GrahamCampbell
Copy link
Member

Please rebase your PR.

@driesvints
Copy link
Member

Ping @JosephSilber

@JosephSilber
Copy link
Contributor

These pack and packAll methods don't really describe what they're doing.

Also, packAll is shallow, which is just an arbitrary decision.


If we are to do something like this, I think it would make more sense to have a generic method that just recursively converts each array into a collection. I don't have a good name off the top of my head, but something like this:

$data = $collection->toDeepCollection();

Then you can do the whitelisting in your own code:

$data->only(...);

or

$data->map->only(...);

To me, such a method would have broader appeal, and is not specific to the given context @sileence is currently working in (validated data).

@sileence
Copy link
Contributor Author

sileence commented Apr 23, 2020

@JosephSilber I thought about using ->collect(), but that method returns an instance of Collection instead of static. Collection != Validated. Is there a reason for that or could we change it so it returns static and then maybe add a "collectList" option?

I don't think we should convert all the arrays recursively to collections because there are times where it's useful to store an array of data. For example user_settings might be an array of data that will be stored in a JSON column:

[
    'name' => 'Duilio',
    'username' => 'sileence',
    'user_settings' => ['dark_mode' => '1'],
],

A recursive method will break that.

The most common scenario that I've had to deal with over an over is sending a simple list of data, like a list of orders, items, users, etc.

@JosephSilber
Copy link
Contributor

JosephSilber commented Apr 23, 2020 via email

@DarkGhostHunter
Copy link
Contributor

It would be also cool to have a limit on how deep you want the collection:

$data = $collection->toDeepCollection(2);

@sileence
Copy link
Contributor Author

sileence commented Apr 24, 2020

@JosephSilber I'm thinking:

$data->collect('settings')

$data->deepCollect('users', 2) or collectList('users')

But right now collect returns a new Collection instead of new static so I'll lose the main objective of this method, which is to get fragments of the Validated data.

Is there any reason why collect doesn't return new static?

@JosephSilber
Copy link
Contributor

https://laravel.com/docs/7.x/collections#method-collect

The collect method is primarily useful for converting lazy collections into standard Collection instances

The collect method is especially useful when you have an instance of Enumerable and need a non-lazy collection instance. Since collect() is part of the Enumerable contract, you can safely use it to get a Collection instance.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

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.

7 participants