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

Preparation for immutable objects #116

Closed
shadowhand opened this issue Aug 26, 2015 · 8 comments
Closed

Preparation for immutable objects #116

shadowhand opened this issue Aug 26, 2015 · 8 comments

Comments

@shadowhand
Copy link
Contributor

Right now it is not possible to modify an immutable object using prepare. For instance, I might want to do this (as a contrived example):

$injector->prepare('Zend\Diactoros\Response', function (&$response) {
    $response = $response->withHeader('X-API-Version', 'v2.1');
});

Attempting this will currently produce the following error:

Warning: Parameter 1 to {closure}() expected to be a reference, value given

Because reflection is used to execute the closure, it is not possible to use references.

What is the best way to handle immutable preparation?

@J7mbo
Copy link
Contributor

J7mbo commented Aug 26, 2015

Hmm, looks like you're trying to tell the injector to, when it encounters an instance of Zend\Diactoros\Response, inject an altered version of it (with the header). Have you considered instantation delegation for this (basically, a factory).

This way your implementation doesn't know about the injector.

$apiResponseFactory = function() use ($response) {
    return $response->withHeader('X-API-Version', 'v2.1');
};

$injector->delegate('Zend\Diactoros\Response', $apiResponseFactory);

Just a suggestion. This might not work for you as it'll need to be in the same context as where you get the $response object.

An alternative would be to grab the injector in the $apiResponseFactory and use that to change the already shared $response. Or maybe that's complete rubbish! Perhaps @rdlowrey can chime in here :-)

@rdlowrey
Copy link
Owner

This is a sane use-case. There are a couple of options to "fix" prepare() so that this works ... I think the easiest is just to have it check the return value of the prepare callable with an instanceof and if it matches the expected class/interface type have the Injector internally replace the old instance with the newly returned instance.

This would be safe for BC as the return value from prepares wasn't previously meaningful. Can anyone think of any drawbacks to that approach that I may be missing?

Using a delegate like @J7mbo suggested would work, but I try to avoid being forced to instantiate closures when possible in favor of string class method constructions just to make sure things load as lazily as possible (the closure may not always be needed for the particular application resource being loaded).

This would change the OP code to:

$injector->prepare('Zend\Diactoros\Response', function ($response) {
    return $response->withHeader('X-API-Version', 'v2.1');
});

@shadowhand
Copy link
Contributor Author

@rdlowrey I like that solution very much. Using delegate is not ideal because it forces me to handle the creation of the object or already have it created, which might have unintended consequences if the class is aliased somehow.

@rdlowrey
Copy link
Owner

Okay, great. I'll try to implement/test/tag that change today. Will post updates here as they happen.

@J7mbo
Copy link
Contributor

J7mbo commented Aug 26, 2015

The prepare() method change looks much better, I really like that 👍

@rdlowrey
Copy link
Owner

I've pushed the mentioned change up to master. @shadowhand if you can confirm that works as you expect (you can probably just verify by looking at the new test cases) I will go ahead and tag 1.1.0

@rdlowrey
Copy link
Owner

This capability is available as of 1.1.0

@kelunik
Copy link
Collaborator

kelunik commented Aug 27, 2015

👍

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

No branches or pull requests

4 participants