[#3625] Do not query abstract factories for registered invokables #3640

Merged
merged 2 commits into from Feb 4, 2013

Conversation

Projects
None yet
3 participants
Owner

weierophinney commented Jan 31, 2013

  • Prior to this patch, if a route has been registered as an invokable,
    but the DiAbstractServiceFactory was also present, using the FQCN to
    retrieve the route would result in DI being invoked. In the case of
    the various shipped ZF routes, this was undesirable, as the factory
    method for these classes was intended for instantiation; usage if DI
    caused failures.
  • This patch overrides the setInvokableClass() method of the router
    plugin manager such that it also calls setAlias() to alias the FQCN to
    the service name; this ensures that usage of the FQCN for a class
    already registered as an invokable will work properly.

FIxes #3625

+ *
+ * @param string $name
+ * @param string $invokableClass
+ * @param null|bool $shared
@Freeaqingme

Freeaqingme Feb 1, 2013

Member

Shouldn't this be marked as optional, with the type 'null' removed?

@weierophinney

weierophinney Feb 1, 2013

Owner

This existed previously (the only new part with my commit was adding the docblock). Basically, null has a different semantic meaning than true or false, and needs to be called out.

+ try {
+ $route = $routes->get($shortName, $options);
+ $this->assertInstanceOf($routeName, $route);
+ } catch (\Exception $e) {
@Freeaqingme

Freeaqingme Feb 1, 2013

Member

Wouldn't simply not catching the exception have the same result?

@weierophinney

weierophinney Feb 1, 2013

Owner

No. I needed to be able to see the error messages when running the tests to understand what exception was thrown and why. There were actually different exceptions thrown at different times, and having the catch block allowed me to know when I was on the right track.

weierophinney added some commits Jan 31, 2013

[#3625] Do not query abstract factories for registered invokables
- Prior to this patch, if a route has been registered as an invokable,
  but the DiAbstractServiceFactory was also present, using the FQCN to
  retrieve the route would result in DI being invoked. In the case of
  the various shipped ZF routes, this was undesirable, as the factory
  method for these classes was intended for instantiation; usage if DI
  caused failures.
- This patch overrides the setInvokableClass() method of the router
  plugin manager such that it also calls setAlias() to alias the FQCN to
  the service name; this ensures that usage of the FQCN for a class
  already registered as an invokable will work properly.
[#3625] Added additional test for short names
- Added test demonstrating routes may be pulled by short names. Ran this
  test both before and after the setInvokableClass() change; passed in
  both cases.
Owner

weierophinney commented Feb 1, 2013

Error is 5.5 only, which is allowed; 5.5 simply didn't run.

@ezimuel ezimuel merged commit 4ca56a3 into zendframework:master Feb 4, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment