Implement factory creation #52

weierophinney · 2018-02-08T23:12:15Z

This patch ~~is a work in progress, addressing~~ addresses #42.

Functionality for creating a factory by reflecting a class
Functionality for writing a factory class file for a given class name
symfony/console command factory:create <class>
Update handler:create to also generate a factory for the created handler; command should include a --no-factory switch to disable this
Update middleware:create to also generate a factory for the created middleware; command should include a --no-factory switch to disable this
Register class/factory pairs in config/autoload/dependencies.global.php

~~One question I have: should the command be factory:create or factory:create-for?~~

The final commands are:

expressive factory:create <for class name> [--no-register]
expressive handler:create <class name> [--no-factory] [--no-register]
expressive middleware:create <class name> [--no-factory] [--no-register]

Creates and returns the string contents for the factory class based on reflecting the given class. Factories: - use strict mode - are generic classes defining `__invoke`, which accepts a PSR-11 container - import the PSR-11 container and any classes the class constructor depends on, even if they are in the same namespace - pull class dependencies from the container

Composes a `FactoryClassGenerator` in order to produce a factory for the given class name, and then writes that class in a sibling file. If the class is not autoloadable, `Create` raises an exception. If unable to write the file to the filesystem, `Create` also raises an exception. The factory method, `createForClass`, returns the filename where the factory class was written.

geerteltink · 2018-02-09T05:30:52Z

One question I have: should the command be factory:create or factory:create-for?

factory:create sounds better and is shorter

svycka · 2018-02-09T09:40:42Z

It would be useful if the create middleware functionality could add a skeleton factory and register it in the module's ConfigProvider.
Where should the factory be saved? I've seen people saving it in the same dir and others in a special Factory dir.

Same directory. As this is what the skeleton does for HomePageFactory. If people put it elsewhere, they can not use this tool or move it.

maybe add config option for this? or ask for the first time or even more fun it could check if exists Factory directory then create there instead

emir · 2018-02-10T17:26:14Z

factory:create can be predictable and keeps consistency to other commands like handler:create, middleware:create

weierophinney · 2018-02-11T18:43:04Z

maybe add config option for this?

This hugely complicates the command, and makes errors during generation far more likely. The only way it would work reliably is if, in addition to the class name for which to generate the factory, we required both the class name for the new factory, and the path where the factory should be created. At that point, you're typing as much, if not more, than you would if you were to generate the factory and then perform a git mv operation.

`ConfigInjector` creates and/or updates a special autoloadable config file, zend-expressive-tooling-factories.global.php. Its `injectFactoryForClass()` method expects the factory class name and the class name to which it maps. It slurps up the existing configuration, if any, and then adds the new mapping, sorts the entries, and writes the file back to its location.

Modifies the CreateFactoryCommand to register the new factory for the class using the ConfigInjector by default. Users can opt to disable this feature via the `--no-register` option.

When `middleware:create` is called, it now generates a factory for the generated middleware, as a sibling class, using `factory:create`. Users can disable this by passing the `--no-factory` class. Users can also generate the factory, but not register it, by passing the `--no-register` option.

Updates the `handler:create` command to trigger the `factory:create` command with the name of the generated handler. Users may disable generation of the factory using the option `--no-factory`. Users may generate the factory, but omit registration of the handler/factory pair by passing the option `--no-register`.

When testing the `middleware:create` and `handler:create` commands, I received exceptions during factory generation saying that the middleware and/or handler class could not be found. Adding a `require $path` statement addresses the issue, but introduces new problems during testing, which I addressed by adding a private flag that, when disabled, skips the `require` statement.

geerteltink · 2018-02-12T16:29:59Z

src/Factory/ConfigInjector.php

+ */
+class ConfigInjector
+{
+    public const CONFIG_FILE = 'config/autoload/zend-expressive-tooling-factories.global.php';


Is there a reason why a new file is used and not config/autoload/dependencies.global.php?

Yes: to prevent issues when re-generating the file:

serialization of closures

resolution of constants

expressions

usage of import statements

It's far more complex to write to a file meant to be managed manually than to use a file that we can mark as under control of the tooling.

Makes sense. Thanx for the explanation.

michalbundyra

It looks like that we are also missing definition of const HELP_ARG_MODULE in CreateHandlerCommand and CreateFactoryCommand.
For reference please see other command and class final class CommandCommonOptions (line 32)

update: oh, we are not calling addDefaultOptionsAndArguments so we don't need to define that constant there.

michalbundyra · 2018-02-12T16:42:20Z

src/CreateHandler/CreateHandlerCommand.php

@@ -63,6 +89,22 @@ protected function execute(InputInterface $input, OutputInterface $output)
            $path
        ));

+        if (! $input->getOption('no-factory')) {
+            $this->requireHandlerBeforeGeneratingFactory && require $path;


This is unclear, not easy readable. I know how it works, but it looks like 'js optimized code'.

What's more I don't like to include the class that way. Can't we use autoloader? If not, maybe we should use BetterReflection to read the class by reflection without autoloading the class?

If there is no other way than require please change it for more readable if statement

It's a testing problem, plain and simple. require works fine in actual use, but in testing, we get an error due to the fact that no resolvable class file is created.

I can update this to be an if statement; it just feels like overkill for what will be the general use case.

And no, the autoloader does not work in this situation. I'm not entirely sure why. But I tried a number of approaches, and the only way I could get it to be able to reflect the generated class was to require it first. I am reluctant to add BetterReflection when we can accomplish what we need without the extra dependency.

michalbundyra · 2018-02-12T16:43:28Z

src/CreateMiddleware/CreateMiddlewareCommand.php

@@ -63,6 +88,22 @@ protected function execute(InputInterface $input, OutputInterface $output)
            $path
        ));

+        if (! $input->getOption('no-factory')) {
+            $this->requireMiddlewareBeforeGeneratingFactory && require $path;


The same as noted above.

michalbundyra · 2018-02-12T16:46:34Z

src/Factory/ConfigInjector.php

+            throw ConfigFileNotWritableException::forFile($this->configFile);
+        }
+
+        $config = file_exists($this->configFile) ? include($this->configFile) : [];


I think usually we are not using parenthesis with include. We have this check in upcoming zend-coding-standards

I had done this because originally I was calculating the filename inline; I'll remove the parens in an upcoming commit.

michalbundyra · 2018-02-12T16:49:02Z

src/Factory/ConfigInjector.php

+
+    private function configIsWritable() : bool
+    {
+        if (! file_exists($this->configFile) && ! is_writable(dirname($this->configFile))) {


I think we can simplify this function, according to the php docs:

Returns TRUE if the filename exists and is writable.
http://php.net/is_writable

So it's not enough just return is_writable($this->configFile) ?

(it checks existence)

Noted; thanks!

Actually, this only applies to the following conditional, not the one flagged. Note that this one is testing explicitly that the parent directory is writable when the file does not exist.

The one following we can rewrite to ! is_writable($this->configFile), as we're then testing that it both exists and is writable.

Okay, interestingly: tests FAIL when I change this! I'm not sure if this is a difference with how vfsStream handles the is_writable() call, or if the documented behavior of is_writable may be incorrect. Regardless, it needs to stay as-is.

Ok, I can check it later, for now it's fine for me. 👍

michalbundyra · 2018-02-12T16:55:25Z

src/Factory/FactoryAlreadyExistsException.php

+
+use RuntimeException;
+
+class FactoryAlreadyExistsException extends RuntimeException


I would extract all Exceptions to namespace, as we have in different project.
I can see we don't have it here in commands, but we can refactor it later on as well.
Here we have more exceptions so it will be clearer to have them in separate directory/namespace (each for command ofc)

I'd rather not. Most of these commands are:

A command class

One or more utility classes to which they delegate

One or more exceptions

The exceptions are specific to the specific command domain, and having a separate namespace is overkill, even when there are multiple different types. Consdiering the majority have a single exception type, forcing each to have a subnamespace is imposing hierarchy just for the sake of it.

michalbundyra · 2018-02-12T17:04:40Z

src/Factory/FactoryClassGenerator.php

+            $constructorParameters,
+            function (ReflectionParameter $argument) {
+                if ($argument->isOptional()) {
+                    return false;


Do I understand correctly - optional params are not going to be initialized in the factory?

Correct. This is also how zend-servicemanager handles optional arguments in its ReflectionBasedAbstractFactory and related factory generator.

michalbundyra · 2018-02-12T17:06:53Z

test/CreateHandler/CreateHandlerCommandTest.php

+
+        return $application;
+
+        $this->command->setApplication($application->reveal());


unreachable statement

Oh! That's not good! I'll remove the return statement!

Actually, need to remove the setApplication() statements, as they happen in the test cases that call this method.

michalbundyra · 2018-02-12T17:08:02Z

test/CreateHandler/CreateHandlerCommandTest.php

@@ -43,6 +54,32 @@ private function reflectExecuteMethod()
        return $r;
    }

+    private function mockApplication()


As we are using PHP 7.1 I would suggest to add return type (Application?)

Sure; do we need to add RTH within a test case, though?

We don't need to, but it would be nice, especially for IDEs and writing new test.
I would add docblock with Application|ObjectProphecy as it is more accurate, and maybe we should go this way, not RTH.

This is... confusing. We can't specify a RTH of Application, as we actually return a prophecy. But specifying ObjectProphecy as the RTH makes it less clear what we're returning. I'll document it via a docblock annotation instead.

michalbundyra · 2018-02-12T17:08:58Z

test/CreateMiddleware/CreateMiddlewareCommandTest.php

+
+        return $application;
+
+        $this->command->setApplication($application->reveal());


unreachable statement

Will fix; thanks!

michalbundyra · 2018-02-12T17:12:48Z

test/Factory/FactoryClassGeneratorTest.php

+        $className = InvokableObject::class;
+        $factory = file_get_contents(__DIR__ . '/TestAsset/factories/InvokableObject.php');
+
+        self::assertEquals($factory, $this->generator->createFactory($className));


Just for consistency, everywhere in this PR you are using $this-> ... and maybe we can make a final decision what we are gonna do: self:: or $this-> (or static:: 🤣 )

I'll fix that; not sure why I used self:: here; likely copy-pasta from the original in zend-servicemanager.

- Adds a docblock with the RTH specified as `ObjectProphecy|Application` - Removes `$this->command->setApplication()` calls within the `mockApplication()` methods; these were unreachable, and unwanted.

- Do not test if a `ReflectionClass` was produced; an exception is thrown if one is not. - Use `! $constructorParameters` instead of `empty($constructorParameters)` - An exception class was not imported, due to bad copy-pasta; created a new one with a named constructor, and used that instead.

michalbundyra · 2018-02-12T18:37:42Z

src/Factory/FactoryClassGenerator.php

+        return substr($className, strrpos($className, '\\') + 1);
+    }
+
+    private function getConstructorParameters(string $className) : array


Missing @throws annotation with the function (UnidentifiedTypeException)

michalbundyra · 2018-02-12T19:09:03Z

src/Factory/ConfigInjector.php

+    /**
+     * Sorts entries by key, using natcase
+     */
+    private function sort(array $config) : array


This is basically ksort with SORT_NATURAL flag :) So I would suggest to use internal function instead.

…lass Uses `substr` + `strrpos` to calculate it.

… implementation.

Close #52 Fixes #42

weierophinney added 5 commits February 8, 2018 14:42

Adds symfony/console command class CreateFactoryCommand

1726258

Registers factory:create command with CLI tool

9ba75e6

Also raise an exception if the factory file to generate already exists

398e4de

weierophinney added this to the 1.0.0 milestone Feb 8, 2018

weierophinney added the enhancement label Feb 8, 2018

geerteltink mentioned this pull request Feb 9, 2018

Update CreateMiddleware to add a factory and register it #42

Closed

weierophinney added 6 commits February 12, 2018 09:06

Updates CreateFactoryCommand to register factory

829883e

Modifies the CreateFactoryCommand to register the new factory for the class using the ConfigInjector by default. Users can opt to disable this feature via the `--no-register` option.

Adds CHANGELOG entry for zendframework#52

023beef

geerteltink reviewed Feb 12, 2018

View reviewed changes

michalbundyra reviewed Feb 12, 2018

View reviewed changes

weierophinney added 5 commits February 12, 2018 11:38

Refactor flag && require statements to conditional blocks

e8073b2

Remove parens around include argument

063f3e9

Fix mockApplication method implementations

8aa64ac

- Adds a docblock with the RTH specified as `ObjectProphecy|Application` - Removes `$this->command->setApplication()` calls within the `mockApplication()` methods; these were unreachable, and unwanted.

s/self::/$this->/ for consistency in test cases

0071037

michalbundyra reviewed Feb 12, 2018

View reviewed changes

weierophinney added 3 commits February 12, 2018 13:42

Fixes how FactoryClassGenerator determines namespace from submitted c…

91e4f05

…lass Uses `substr` + `strrpos` to calculate it.

Simplifies ConfigInjection::configIsWritable() logic

204737f

Adds a missing @throws annotation

c8058bc

Use ksort() with the SORT_NATURAL sort flag, instead of homegrown…

1f61691

… implementation.

weierophinney force-pushed the feature/factory-creation branch from ccf16fb to 1f61691 Compare February 12, 2018 20:06

weierophinney merged commit 1f61691 into zendframework:release-1.0.0 Feb 12, 2018

weierophinney added a commit that referenced this pull request Feb 12, 2018

Merge branch 'feature/factory-creation' into release-1.0.0

1d9bdf0

Close #52 Fixes #42

weierophinney deleted the feature/factory-creation branch February 12, 2018 20:13


		use RuntimeException;

		class FactoryAlreadyExistsException extends RuntimeException


		return $application;

		$this->command->setApplication($application->reveal());

Implement factory creation #52

Implement factory creation #52

Conversation

weierophinney commented Feb 8, 2018 • edited Loading

geerteltink commented Feb 9, 2018

svycka commented Feb 9, 2018

emir commented Feb 10, 2018

weierophinney commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbundyra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Feb 8, 2018 •

edited

Loading

michalbundyra left a comment •

edited

Loading