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

Handle already-initialized components #15

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

EtienneBruines
Copy link
Contributor

@EtienneBruines EtienneBruines commented Sep 5, 2019

Sometimes the components within the configuration may already be configured.

This change adds support for following config syntax:

return [
    'components' => [
        'client' => new \GuzzleHttp\Client(),
    ],
]

src/ServiceMap.php Outdated Show resolved Hide resolved
Copy link
Contributor

@akondas akondas left a comment

Choose a reason for hiding this comment

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

Generally looks goods, maybe you can try to add a test?

@EtienneBruines
Copy link
Contributor Author

Hello @akondas, thank you for your code review. It is much appreciated.

As per your suggestion, I changed !is_array to is_object. Additionally, I added a check to verify that if it's not an object, it should at least be an array - otherwise the code will crash in a weird manner anyways. Now a more useful error message is thrown.

Two tests were added:

  • testThrowExceptionWhenComponentHasInvalidValue to verify the exception is thrown when the component value was invalid (e.g. a numeric value)
  • testItLoadsServicesAndComponents (updated) to verify that the additional component customInitializedComponent is recognized.

I wasn't quite sure what the getComponentIdentityClassById method was supposed to do, so I didn't change it. I had to add a special condition for getComponentClassById to make it perform as expected.

If you have any points of improvements, don't hesitate to mention them 😄

@akondas akondas added this to the 0.6.0 milestone Sep 10, 2019
@akondas
Copy link
Contributor

akondas commented Sep 10, 2019

Great, thanks for tests 👍

@akondas akondas merged commit 6b006ff into proget-hq:master Sep 10, 2019
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