Feature: generated injector delegator #51

michalbundyra · 2019-01-09T13:33:03Z

In documentation we are suggesting to create in manually. I think we can have it in the library as it's generic code.

See: https://docs.zendframework.com/zend-di/cookbook/aot-guide/#5-add-aot-to-the-service-manager

Documentations needs update if it is going to be accepted.

Are you creating a new feature?
- Why is the new feature needed? What purpose does it serve?
- How will users use the new feature?
- Base your feature on the develop branch, and submit against that branch.
- Add only one feature per pull request; split multiple features over multiple pull requests
- Add tests for the new feature.
- Add documentation for the new feature.
- Add a CHANGELOG.md entry for the new feature.

As it could be used comonnly would be nice to have it out of the box.

tux-rampage

This makes sense. Thanks a lot for your effort. Just a view nitpicks that needs to be fixed before a merge.

I also think it makes sense to add it to the ConfigProvider

src/GeneratedInjectorDelegator.php

@tux-rampage

- per @tux-rampage

michalbundyra · 2019-01-09T21:56:27Z

@tux-rampage

I also think it makes sense to add it to the ConfigProvider

I am not sure about it, what if someone already has it added there (the custom one) or if someone what to use another?

tux-rampage · 2019-01-10T08:38:55Z

@tux-rampage

I also think it makes sense to add it to the ConfigProvider

I am not sure about it, what if someone already has it added there (the custom one) or if someone what to use another?

Good point. Could you update the AoT guide?

michalbundyra · 2019-01-10T10:19:38Z

@tux-rampage

Could you update the AoT guide?

Docs is updated now.

tux-rampage

I found some additional minor things that we may need to address.

Thanks for your effort to improve this component 👍

src/GeneratedInjectorDelegator.php

This namespace is default for generated resources.

src/GeneratedInjectorDelegator.php

test/GeneratedInjectorDelegatorTest.php

tux-rampage

Hi @webimpress

sorry for the delay on this. Just one thing with the exception you throw. After that I think we're good to go.

src/GeneratedInjectorDelegator.php

test/GeneratedInjectorDelegatorTest.php

This will improve readability and makes the unit easier to test. It will also offer a typed information to consumers to get rid it.

tux-rampage · 2019-12-16T10:37:52Z

@michalbundyra As discussed in slack, I've addressed this one. Can you pleas re-check?

michalbundyra · 2019-12-16T10:55:29Z

src/GeneratedInjectorDelegator.php

+    {
+        $config = $container->has('config') ? $container->get('config') : [];
+        $aotConfig = $config['dependencies']['auto']['aot'] ?? [];
+        $namespace = empty($aotConfig['namespace']) ? 'Zend\Di\Generated' : $aotConfig['namespace'];


Maybe better "empty or non-string" here?

Suggested change

$namespace = empty($aotConfig['namespace']) ? 'Zend\Di\Generated' : $aotConfig['namespace'];

$namespace = empty($aotConfig['namespace']) || ! is_string($aotConfig['namespace'])

? 'Zend\Di\Generated'

: $aotConfig['namespace'];

Like this?

yes, please

test would be with an array, for example, as it can't be casted to string

I was just going to make this change and saw it would render the next 3 lines useless:

$config = $container->has('config') ? $container->get('config') : []; $aotConfig = $config['dependencies']['auto']['aot'] ?? []; $namespace = empty($aotConfig['namespace']) ? 'Zend\Di\Generated' : $aotConfig['namespace']; if (! is_string($namespace)) { throw new InvalidServiceConfigException('Provided namespace is not a string.'); }

I'd argue to leave it as is. This will be explicit to the user that there is an unusable misconfiguration instead of hiding it behind a default value that may cause failures which may become hard to debug. At least it's not obvious what's the issue that cause incorrect behavior.

I'll change the test from an int fixture to an array though.

michalbundyra · 2019-12-16T10:57:10Z

src/Exception/InvalidServiceConfigException.php

+
+use Psr\Container\ContainerExceptionInterface;
+
+class InvalidServiceConfigException extends LogicException implements ContainerExceptionInterface


not sure if LogicException as a parent is the correct one

Maybe better InvalidArgumentException?

InvalidArgumentException actually is a LogicException.
But in this case I'd argue that this is not cased by a function argument but a (service)configuration value. This could also be seen a RuntimeException.

I chose LogicException since it's the developer's job to configure the services correctly, and if they don't provide a namespace-string, they made a logical mistake.

https://www.php.net/manual/en/class.logicexception.php
Exception that represents error in the program logic. This kind of exception should lead directly to a fix in your code.

But I'm happy if you provide a better fit.

Ok, sounds good to me.

I just wonder if we should have then another exception before if the namespace is provided but is not null and not a string? Maybe we shouldn't ignore invalid value. What do you think?

Isn't this already the case or am I missing something? 🤔

michalbundyra · 2019-12-16T10:59:09Z

test/GeneratedInjectorDelegatorTest.php

@@ -27,7 +28,7 @@ public function testProvidedNamespaceIsNotAString()

        $delegator = new GeneratedInjectorDelegator();

-        $this->expectException(ContainerExceptionInterface::class);
+        $this->expectException(InvalidServiceConfigException::class);


it was quite important check, as container (per specification) can throw only ContainerExceptionInterface instances.

would be nice to have it somewhere

Is a unit test really the correct scope? We could assert that InvalidServiceConfigException implements ContainerExceptionInterface. So that whenever this changes this test fails:

Suggested change

$this->expectException(InvalidServiceConfigException::class);

self::assertInstanceOf(ContainerInterface::class, new InvalidServiceConfigException());

$this->expectException(InvalidServiceConfigException::class);

Yeah, probably it should be separate test case, but I am happy to have it here for time being.

I guess a separate test would be out of scope for now. I'm not entirely sure if this is the correct place or way to cover this. Maybe we find something more appropriate in the future. 😉

Until then I added the assert as suggested.

michalbundyra · 2019-12-16T11:04:42Z

docs/book/cookbook/aot-guide.md

-```
-
-After this we need to add configuration to the `ConfigProvider` class we created
+First, we'll use a delegator `Zend\Di\GeneratedInjectorDelegator` to decorate


I think we need to note that GeneratedInjectorDelegator is available since version X.Y.Z.

Docs are updated. If somebody has a more elegant phrasing or suggesstion, I'd appreciate to hear it.

tux-rampage · 2019-12-16T11:17:47Z

Static analysis is failing: https://travis-ci.org/zendframework/zend-di/jobs/625613032#L425
This was introduced in #56

… exception

Added GeneratedInjectorDelegator

fee64f7

As it could be used comonnly would be nice to have it out of the box.

tux-rampage self-assigned this Jan 9, 2019

tux-rampage added the enhancement label Jan 9, 2019

tux-rampage added this to the 3.2.0 milestone Jan 9, 2019

tux-rampage suggested changes Jan 9, 2019

View reviewed changes

src/GeneratedInjectorDelegator.php Outdated Show resolved Hide resolved

src/GeneratedInjectorDelegator.php Outdated Show resolved Hide resolved

Drop interface and useless parameter in delegator

3b7919c

- per @tux-rampage

Updated docs to use new decorator

2ba10fd

tux-rampage suggested changes Jan 15, 2019

View reviewed changes

src/GeneratedInjectorDelegator.php Show resolved Hide resolved

src/GeneratedInjectorDelegator.php Outdated Show resolved Hide resolved

michalbundyra added 3 commits January 15, 2019 09:57

If namespace is not configured/empty use default one: Zend\Di\Generated

eff2026

This namespace is default for generated resources.

Added import internal PHP function: class_exists

9d39117

Check if provided namespace is a string

884fb36

michalbundyra commented Jan 15, 2019

View reviewed changes

src/GeneratedInjectorDelegator.php Outdated Show resolved Hide resolved

michalbundyra commented Jan 15, 2019

View reviewed changes

test/GeneratedInjectorDelegatorTest.php Outdated Show resolved Hide resolved

tux-rampage suggested changes Feb 18, 2019

View reviewed changes

src/GeneratedInjectorDelegator.php Outdated Show resolved Hide resolved

test/GeneratedInjectorDelegatorTest.php Outdated Show resolved Hide resolved

tux-rampage added 2 commits December 16, 2019 11:33

Change anonymous to named exception class

da3001e

This will improve readability and makes the unit easier to test. It will also offer a typed information to consumers to get rid it.

Remove unused class imports

5bbf19e

tux-rampage approved these changes Dec 16, 2019

View reviewed changes

michalbundyra commented Dec 16, 2019

View reviewed changes

tux-rampage added 4 commits December 16, 2019 13:50

Add changelog entry

46d0e67

Add information for the delegator factory before 3.2

658bcb1

Add test to ensure InvalidServiceConfigException implements container…

41b3ebf

… exception

Change test fixture from int to array

e1e0432

Merge branch 'develop' into feature/generated-injector-delegator

453343b

michalbundyra merged commit a7092d6 into zendframework:develop Dec 31, 2019

michalbundyra deleted the feature/generated-injector-delegator branch December 31, 2019 05:22

tux-rampage mentioned this pull request Jun 26, 2020

Custom logger laminas/laminas-di#11

Merged

tux-rampage mentioned this pull request Aug 28, 2020

Fix handling of aot namespace config laminas/laminas-di#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: generated injector delegator #51

Feature: generated injector delegator #51

michalbundyra commented Jan 9, 2019 •

edited

Loading

tux-rampage left a comment

michalbundyra commented Jan 9, 2019

tux-rampage commented Jan 10, 2019

michalbundyra commented Jan 10, 2019

tux-rampage left a comment

tux-rampage left a comment

tux-rampage commented Dec 16, 2019

michalbundyra Dec 16, 2019

tux-rampage Dec 16, 2019 •

edited

Loading

michalbundyra Dec 16, 2019

tux-rampage Dec 17, 2019

michalbundyra Dec 16, 2019

tux-rampage Dec 16, 2019

michalbundyra Dec 16, 2019

tux-rampage Dec 17, 2019

michalbundyra Dec 16, 2019

tux-rampage Dec 16, 2019

michalbundyra Dec 16, 2019

tux-rampage Dec 17, 2019

michalbundyra Dec 16, 2019

tux-rampage Dec 16, 2019

tux-rampage Dec 17, 2019

tux-rampage commented Dec 16, 2019


		use Psr\Container\ContainerExceptionInterface;

		class InvalidServiceConfigException extends LogicException implements ContainerExceptionInterface

	$this->expectException(InvalidServiceConfigException::class);
	self::assertInstanceOf(ContainerInterface::class, new InvalidServiceConfigException());
	$this->expectException(InvalidServiceConfigException::class);

Feature: generated injector delegator #51

Feature: generated injector delegator #51

Conversation

michalbundyra commented Jan 9, 2019 • edited Loading

tux-rampage left a comment

Choose a reason for hiding this comment

michalbundyra commented Jan 9, 2019

tux-rampage commented Jan 10, 2019

michalbundyra commented Jan 10, 2019

tux-rampage left a comment

Choose a reason for hiding this comment

tux-rampage left a comment

Choose a reason for hiding this comment

tux-rampage commented Dec 16, 2019

Choose a reason for hiding this comment

tux-rampage Dec 16, 2019 • 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

tux-rampage commented Dec 16, 2019

michalbundyra commented Jan 9, 2019 •

edited

Loading

tux-rampage Dec 16, 2019 •

edited

Loading