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

Support instanciation of object with named constructor #303

Merged
merged 3 commits into from
Jan 7, 2016
Merged

Support instanciation of object with named constructor #303

merged 3 commits into from
Jan 7, 2016

Conversation

gquemener
Copy link
Contributor

Currently having a private constructor will force instanciation using reflection (using instanciator ReflectionWithoutConstructor).

Current implementation only allows to use factory method when the class constructor is public.

This is not working when using the Named constructors (http://verraes.net/2014/06/named-constructors-in-php/)

This PR intends to provide the use case describing this scenario and open discussion about the best way to handle it.

So far, I've tried to manipulate the ReflectionWithConstructor and ReflectionWithoutConstructor canInstantiate method, but I can't make the whole test suite pass.

@@ -22,6 +22,10 @@ public function canInstantiate(Fixture $fixture)
{
$reflConstruct = new \ReflectionMethod($fixture->getClass(), '__construct');

if (!$reflConstruct->isPublic() && '__construct' !== $fixture->getConstructorMethod()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gquemener Let's just add this conditional bit to the line below - it's effectively the same thing, but this makes it look like there's much more change happening than there actually is. Make sense?

@gquemener
Copy link
Contributor Author

Yes it does @tshelburne, I was concerned about readability issues with a long conditionnal statement but it's not so bad in the end.

Tell me if you want me to squash the PR before merge.

tshelburne added a commit that referenced this pull request Jan 7, 2016
Support instanciation of object with named constructor
@tshelburne tshelburne merged commit c8cb341 into nelmio:master Jan 7, 2016
@tshelburne
Copy link
Collaborator

@gquemener Thanks!

@gquemener gquemener deleted the feature/named-constructor branch January 8, 2016 08:57
@gquemener
Copy link
Contributor Author

Is it possible to tag a 2.1.4 with this fix please @tshelburne?

@tshelburne
Copy link
Collaborator

@gquemener Bam! Done.

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.

2 participants