Support references to arbitrary container entries for factory definitions #321

Merged
merged 5 commits into from Sep 15, 2015

Conversation

Projects
None yet
2 participants
@jdreesen
Contributor

jdreesen commented Sep 12, 2015

Currently, references to container entries in factory definitions are only possible, if they reference a class that actually exist with this exact FQN.

This PR enables you to to reference arbitrary container entries in factory definitions (as long as they are callables, of course), for example:

  • factory('my.closure')
  • factory('my.invocable.object')
  • factory(['my.class', 'method'])

Todo:

  • write tests
  • update documentation

Closes #272

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 12, 2015

Member

👍

However is the change (current state of the PR) necessary? is_callable('A::b') is true (see https://3v4l.org/HgXfa)

Member

mnapoli commented Sep 12, 2015

👍

However is the change (current state of the PR) necessary? is_callable('A::b') is true (see https://3v4l.org/HgXfa)

@mnapoli mnapoli added the enhancement label Sep 12, 2015

@mnapoli mnapoli added this to the 5.2 milestone Sep 12, 2015

@jdreesen

This comment has been minimized.

Show comment
Hide comment
@jdreesen

jdreesen Sep 12, 2015

Contributor

Oh, I didn't know that this is possible! You are right, is_callable('A::b') is true for valid PHP callables (if class A actually exists).

But this change uses the container for callable resolution, too, which enables you to use any container entry name for the first part and thus allows something like: factory('foo.bar.baz::create')

Contributor

jdreesen commented Sep 12, 2015

Oh, I didn't know that this is possible! You are right, is_callable('A::b') is true for valid PHP callables (if class A actually exists).

But this change uses the container for callable resolution, too, which enables you to use any container entry name for the first part and thus allows something like: factory('foo.bar.baz::create')

@jdreesen

This comment has been minimized.

Show comment
Hide comment
@jdreesen

jdreesen Sep 12, 2015

Contributor

I just looked at the documentation where it says Please note: you can set any container entry name in the array, e.g. factory(['foo_bar_baz', 'create']), allowing you to configure foo_bar_baz and its dependencies like any other object.

This is actually not possible at the moment and would only be possible with the changes in this PR, too.

Contributor

jdreesen commented Sep 12, 2015

I just looked at the documentation where it says Please note: you can set any container entry name in the array, e.g. factory(['foo_bar_baz', 'create']), allowing you to configure foo_bar_baz and its dependencies like any other object.

This is actually not possible at the moment and would only be possible with the changes in this PR, too.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 12, 2015

Member

Mhh yeah that's true, 2 good points! I can't believe I didn't add tests in #308

Member

mnapoli commented Sep 12, 2015

Mhh yeah that's true, 2 good points! I can't believe I didn't add tests in #308

@jdreesen

This comment has been minimized.

Show comment
Hide comment
@jdreesen

jdreesen Sep 13, 2015

Contributor

Added some unit tests that cover factory definitions that reference container entries.

Tests for Class::method callables are failing with dependencies=lowest, of course... should we raise the composer version for php-di/invoker to ^1.1?

Contributor

jdreesen commented Sep 13, 2015

Added some unit tests that cover factory definitions that reference container entries.

Tests for Class::method callables are failing with dependencies=lowest, of course... should we raise the composer version for php-di/invoker to ^1.1?

@jdreesen jdreesen changed the title from [WIP] Support 'Class::method` callables for factory definitions to [WIP] Support references to container entries for factory definitions Sep 13, 2015

@jdreesen jdreesen changed the title from [WIP] Support references to container entries for factory definitions to [WIP] Support references to arbitrary container entries for factory definitions Sep 13, 2015

@jdreesen

This comment has been minimized.

Show comment
Hide comment
@jdreesen

jdreesen Sep 13, 2015

Contributor

Changed name and description of this PR to what it actually does.

Contributor

jdreesen commented Sep 13, 2015

Changed name and description of this PR to what it actually does.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 13, 2015

Member

should we raise the composer version for php-di/invoker to ^1.1?

makes sense to me, yep.

Member

mnapoli commented Sep 13, 2015

should we raise the composer version for php-di/invoker to ^1.1?

makes sense to me, yep.

@jdreesen

This comment has been minimized.

Show comment
Hide comment
@jdreesen

jdreesen Sep 15, 2015

Contributor

Added some documentation (please tell me if you have suggestions for improvements on this :).

Contributor

jdreesen commented Sep 15, 2015

Added some documentation (please tell me if you have suggestions for improvements on this :).

@jdreesen jdreesen changed the title from [WIP] Support references to arbitrary container entries for factory definitions to Support references to arbitrary container entries for factory definitions Sep 15, 2015

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 15, 2015

Member

This is the cleanest pull request ever! Tests, docs, changelog! That rocks :)

Member

mnapoli commented Sep 15, 2015

This is the cleanest pull request ever! Tests, docs, changelog! That rocks :)

mnapoli added a commit that referenced this pull request Sep 15, 2015

Merge pull request #321 from jdreesen/paamayim-nekudotayim
Support references to arbitrary container entries for factory definitions

@mnapoli mnapoli merged commit c9967ab into PHP-DI:master Sep 15, 2015

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.691%
Details

@jdreesen jdreesen deleted the jdreesen:paamayim-nekudotayim branch Sep 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment