nelmio / alice

Expressive fixtures generator
MIT License
2.5k stars 329 forks source link

Change Alice API #998

Open theofidry opened 5 years ago

theofidry commented 5 years ago

https://github.com/nelmio/alice/issues/601#issuecomment-481601779

I think it would be more beneficial for the user to propose a new API to write down fixtures, e.g.:

<?php

use Is\Bundle\PlanBundle\Entity\Event;
use Nelmio\Alice\Alice;

return [
  Event::class => [
    'foo1' => [
      'title' => Alice::faker()->sentence(3),
      'show' => Alice::reference('@show_*'),
      'startDateTime': Alice::faker()->dateTimeBetween('-1 month', '+4 month'),
      'isDraft' => false,
      'version' => Alice::optional(10, Alice::reference('@version_*')),
      '__calls': [
        'setRevenue (25%?)': [Alice::faker()->moneyBetween(10000, 300000)]
      ],
    ],
    'foo2' => new Foo(),
  ],
];

There is three immediate advantages there:

kgilden commented 5 years ago

Just throwing out a suggestion of creating a light-weight DSL to make it even easier. I've seen developers getting confused with setRevenue (25%?): vs setRevenue: (25%?). Something along the lines of the following, perhaps?

<?php

use Is\Bundle\PlanBundle\Entity\Event;
use Nelmio\Alice\Alice;

return [
    Alice::fixture(Event::class)
      ->pattern('foo_{0..99}')
      ->constructor([Alice::reference('@show_*')])
      ->props([
        'isDraft' => false,
        'startDateTime' => Alice::faker()->dateTimeBetween('-1 month', '+4 month'),
      ])
      ->calls([
          'setRevenue' => Alice::sometimesCalled(0.25, [Alice::faker()->moneyBetween(10000, 300000)]),
          'setFoo' => Alice::sometimesNull(0.5, true),
      ])
];

Very rough and there's definitely room for improvement.


Also while we're at it, would it be crazy to abstract away whether the arguments are for the constructor, props or need to call a setter? The system could follow a convention of "constructor args" > "setters" > "properties via reflection". Just to show in code what I mean.

    /**
     * Creates a new object with the given values. Values must be a list mapped
     * by property names. Constructor arguments must also be passed inside $values.
     *
     *     class Ex { private $foo; public function __construct($bar) {}}
     *
     *     // Creates an instance of Ex with $foo set to 23 and $bar 'whatever'.
     *     $factory = new ObjectFactory('Ex');
     *     $ex = $factory->create('Ex', array(
     *         'foo' => 23,
     *         'bar' => 'whatever',
     *     ));
     *
     * @param array $values
     *
     * @return object
     *
     * @throws \RuntimeException If a required constructor argument is missing
     * @throws \Excetpion        If a property was not found
     */
    public function create(array $values = [])
    {
        $class = new \ReflectionClass($this->fqcn);

        $constructorArgs = [];

        if (($constructor = $class->getConstructor())) {
            foreach ($constructor->getParameters() as $arg) {
                if (isset($values[$arg->getName()])) {
                    $constructorArgs[] = $values[$arg->getName()];
                    unset($values[$arg->getName()]);
                } elseif ($arg->isDefaultValueAvailable()) {
                    $constructorArgs[] = $arg->getDefaultValue();
                } else {
                    throw new \RuntimeException(sprintf('Missing required constructor argument "$%s" for "%s".', $arg->getName(), $this->fqcn));
                }
            }
        }

        $object = $class->newInstanceArgs($constructorArgs);
        $this->setProperties($object, $values);

        return $object;
    }

    /**
     * Sets the properties of an object using reflection.
     *
     * @param object $object
     * @param array  $properties
     *
     * @throws \Exception If any of the properties was not found in the object
     */
    private function setProperties($object, array $properties)
    {
        foreach ($properties as $name => $value) {
            try {
                PropertyAccess::createPropertyAccessor()->setValue($object, $name, $value);
            } catch (NoSuchPropertyException $e) {
                if ($this->setProperty($object, $name, $value)) {
                    continue;
                }

                // Try snake case instead of camel case (i.e. "foo_bar" vs "fooBar").
                if ($name === ($altName = Inflector::tableize($name))) {
                    throw new \Exception(sprintf('Could not set "%s" to "%s" on "%s" using property access nor reflection.', $name, $value, get_class($object)), null, $e);
                }

                if ($this->setProperty($object, $altName, $value)) {
                    continue;
                }

                throw new \Exception(sprintf('Could not set "%s" or "%s" to "%s" on "%s" using property access nor reflection.', $name, $altName, $value, get_class($object)), null, $e);
            }
        }
    }

    /**
     * Sets the property of an object using reflection.
     *
     * @param object $object
     * @param string $name
     * @param mixed  $value
     *
     * @return bool Whether the property was found and successfully changed
     *
     * @throws \Exception If the property was not found in the object
     */
    private function setProperty($object, $name, $value)
    {
        $class = new \ReflectionClass($object);

        while ($class && !$class->hasProperty($name)) {
            $class = $class->getParentClass();
        }

        if ($class) {
            $property = $class->getProperty($name);
            $property->setAccessible(true);
            $property->setValue($object, $value);

            return true;
        }

        return false;
    }
theofidry commented 5 years ago

For the first suggestion 👍.

Also while we're at it, would it be crazy to abstract away whether the arguments are for the constructor, props or need to call a setter? The system could follow a convention of "constructor args" > "setters" > "properties via reflection". Just to show in code what I mean.

I don't think it's a good idea because:

cf. https://github.com/nelmio/alice/blob/master/CONTRIBUTING.md#generator for more on that part

c33s commented 4 years ago

-1 for deprecating yaml, as it removes the option to let non developers write fixtures

to abstract away whether the arguments are for the constructor, props or need to call a setter?

this is exactly what i would need also see https://github.com/nelmio/alice/issues/840#issuecomment-671638594

theofidry commented 4 years ago

-1 for deprecating yaml, as it removes the option to let non developers write fixtures

I would argue that the current YAML is more oriented towards developers and falls short in that regard, hence a PHP alternative which would offer both a more robust and auto-discoverable API.

From what I can see in https://github.com/nelmio/alice/issues/840#issuecomment-671638594, even the current YAML is quite far from it. But although it would require some changes to the Fixtures objects, I think this could be achieved with custom data loader (see https://github.com/nelmio/alice/blob/master/CONTRIBUTING.md#architecture) and possibly bypassing completely the expression language which is the problematic part at the moment.

In other words: deprecating the current YAML as you know it does not mean you could not achieve https://github.com/nelmio/alice/issues/840#issuecomment-671638594

tacman commented 3 years ago

I quite like the YAML format, in fact, I think Alice Fixtures are useful even when not leveraging all the cool dynamic options.

In particular, sometimes I just want to dump a small set of fixtures in YAML, then reload them unto another machine or fork. Basically, use the YAML files as an archive. But it's maddening that there's no way to represent a simple strings (like a URL) that contain special symbols, like $, #, /, etc.

goetas commented 2 years ago

I would keep yaml. In our projects, fixtures are written by non php-devs, often with minimal dev experience. forcing them to learn PHP is a no go for us...