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.

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] Add support for callables in model factories attributes #20692

Merged
merged 1 commit into from
Aug 23, 2017
Merged

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

merged 1 commit into from
Aug 23, 2017

Conversation

roberto-aguilar
Copy link
Contributor

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

@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
Copy link
Contributor

scrubmx commented Aug 23, 2017

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
Copy link
Contributor

derekmd commented Aug 23, 2017

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
Contributor

The suggested fix above indeed seems to fix the issue.

This pull request was closed.
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