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

Check bean return type against return type annotation #40

Merged
merged 1 commit into from
Jun 15, 2016

Conversation

dropdevcoding
Copy link
Contributor

Recently I ran into a problem where I used a bean as constructor argument while that argument was optional because the class would create a default object if none was given. Because the BeanFactory does neither throw an exception if the resulting object is null nor if the resulting object's type is different to the return type annotation of the bean it was really hard to figure out why my code didn't work.

So this PR includes exceptions being thrown if the cases mentioned above occur.

@@ -120,7 +128,15 @@ public static function generateMethod(
}

$body .= $innerpadding . '$instance = parent::' . $methodName . '(' . $methodParamTpl . ');' . "\n";
$body .= $innerpadding . '$this->initializeBean($instance, "' . $methodName . '");' . "\n";
$body .= $innerpadding . '$this->initializeBean($instance, "' . $methodName . '");' . "\n\n";
$body .= $innerpadding . 'if (!is_a($instance, \'' . $beanType . '\')) {' . "\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace the is_a() function call with an instanceof check? That might potentially be faster as it is not a method call.

In addition the check should be moved before the $this->initializeBean(). If the expected value is not returned, there is no need to run the initializer logic. Failing early makes sense to me in this case.

@dropdevcoding
Copy link
Contributor Author

Of course we could use instanceof check instead.

I tried to check the return type in front of initializeBean, but that won't work since FactoryBeans return the real instance AFTER it, because resolution is part of the BeanInitializer (and should possibly be moved to BeanMethod or a dedicated MethodGenerator).

@shochdoerfer
Copy link
Member

shochdoerfer commented May 28, 2016

Right, I missed the FactoryBean. Could you add a comment then why the code has the placed where it was placed? ;)

You may want to open another issue to discuss the placement of the FactoryBean call. I simply wanted one location where all the magic happens even though different things happen within the initializeBean() method.

@shochdoerfer
Copy link
Member

After a discussion with @dropdevcoding we will consider dropping support for the FactoryBean as it is currently not needed anymore due to the fact that the configuration code is already PHP and one can use any kind of complex logic to construct a Bean instance. FactoryBeans made sense back in the days of XML configuration but not today anymore.

@shochdoerfer shochdoerfer merged commit 8f0e5bf into master Jun 15, 2016
@shochdoerfer shochdoerfer deleted the enhancement/beanconfiguration branch June 15, 2016 18:19
@shochdoerfer shochdoerfer added this to the 0.4.0 milestone Jun 15, 2016
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.

None yet

2 participants