Skip to content

[5.4] Add support for callables in model factories attributes#20692

Merged
taylorotwell merged 1 commit into
laravel:5.4from
roberto-aguilar:feature/model-factory-callables
Aug 23, 2017
Merged

[5.4] Add support for callables in model factories attributes#20692
taylorotwell merged 1 commit into
laravel:5.4from
roberto-aguilar:feature/model-factory-callables

Conversation

@roberto-aguilar

@roberto-aguilar roberto-aguilar commented Aug 23, 2017

Copy link
Copy Markdown
Contributor

@scrubmx and i were tinkering with the model factories and we discovered that even though there is no support for the usage of any type of callables in the factory definition, this was easy to implement and opens the possibility to interesting patterns. We also found a precedent in #18264 of this behavior so we wanted to give it a shot.

If accepted, this will allow:

Simple callbacks:

$factory->define(App\User::class, function () {
    return [
        'lorem' => function () {
            return 'ipsum';
        },
    ];
});

Object method calls:

$factory->define(App\User::class, function () {
    return [
        'lorem' => [new App\Ipsum, 'create'],
    ];
});

Static class method calls:

$factory->define(App\User::class, function () {
    return [
        'lorem' => 'App\Ipsum::create',
    ];
});

or classes implementing the __invoke magic method:

namespace App;

class Ipsum
{
    public function __invoke()
    {
        return 'ipsum';
    }
}

// And then, in the model factories definitions:

$factory->define(App\User::class, function () {
    return [
        'lorem' => new App\Ipsum,
    ];
});

This will allow to use callables like [$factory, 'create'], Factory::create
or classes implementing the __invoke magic method.
@scrubmx

scrubmx commented Aug 23, 2017

Copy link
Copy Markdown
Contributor

The exact use case we have for this is a json column that we have to fill with data and we want to be able to dynamically return the json data based on other attributes.

Also, the logic can sometimes get very complex and would be nice to have dedicated classes

Example:

$factory->define(App\User::class, function ($faker, $overrides) {
  return [
    'email' => $faker->unique()->safeEmail,
    'type' => $faker->randomElement(['borrower', 'lender']),
    'options' => new App\UserOptionsFactory,
  ];
});
class App\UserOptionsFactory 
{
  public function __invoke($attributes) 
  {
    // Switch or other complex logic
    switch($attributes['type']) {
      case 'borrower': return $this->borrowerOptions();
      case 'lender': return $this->lenderOptions();
    }
  }

  public function borrowerOptions() 
  {
    // Return the appropriate borrower options
  }

  public function lenderOptions() 
  {
    // Return the appropriate lender options
  }
}

@taylorotwell taylorotwell merged commit 91765f5 into laravel:5.4 Aug 23, 2017
@roberto-aguilar roberto-aguilar deleted the feature/model-factory-callables branch August 23, 2017 13:09
@derekmd

derekmd commented Aug 23, 2017

Copy link
Copy Markdown
Contributor

This change will affect factories for enum/constant values that also have an equivalent global function name:

e.g.,

$item = factory(TransactionItem::class)->make([
    'name' => TransactionName::Info, // 'INFO'
    'type' => TransactionType::Collect, // 'COLLECT'
]);
is_callable('INFO');
// true

is_callable('COLLECT');
// true

Then the model attributes end up being filled with the results of those global functions accepting the factory()->make() array payload:

dd($item->getAttributes());

[
     name: null,
     type: Illuminate\Support\Collection {#4742
       all: [
         "name" => null,
         "type" => Illuminate\Support\Collection {#4742},
       ],
     },
]

When it should be:

[
     name: 'INFO',
     type: 'COLLECT',
]

Fix:

if (is_callable($attribute) && !is_string($attribute)) {

This would allow __invoke classes to be used.

@Crinsane

Copy link
Copy Markdown
Contributor

The suggested fix above indeed seems to fix the issue.

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.

5 participants