Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Individual shared setting does not override the shareByDefault setting of the ServiceManager #3439

Closed
wants to merge 2 commits into from
Closed

Individual shared setting does not override the shareByDefault setting of the ServiceManager #3439

wants to merge 2 commits into from

Conversation

zluiten
Copy link
Contributor

@zluiten zluiten commented Jan 15, 2013

This PR currently contains just a failing test.
Edit: Added possible fix.

Fixing is not as easy as it looks.

Making the tests succeed can be achieved by changing the following 3 lines https://github.com/zendframework/zf2/blob/master/library/Zend/ServiceManager/ServiceManager.php#L461

if ($this->shareByDefault() && (!isset($this->shared[$cName]) || $this->shared[$cName] === true)) {
    $this->instances[$cName] = $instance;
}

into

if (
    ($this->shareByDefault() && !isset($this->shared[$cName]))
    || (isset($this->shared[$cName]) && $this->shared[$cName] === true)
) {
    $this->instances[$cName] = $instance;
}

But another problem lies in how the shared setting of the indivual services are set: as the third parameter of setInvokableClass($name, $invokableClass, $shared = true) and setFactory($name, $factory, $shared = true) which sets the _individual_ shared setting always to yes by default which is a problem when shareByDefault is set to false.

Any ideas are welcome.

@zluiten
Copy link
Contributor Author

zluiten commented Jan 15, 2013

I guess a solution could be to change the default value of $shared in the signature of setInvokableClass and setFactory: if $shared == null then get the ServiceManager's default value.

@tux-rampage tux-rampage mentioned this pull request Jan 16, 2013
Closed
@zluiten zluiten mentioned this pull request Jan 16, 2013
Closed
@zluiten
Copy link
Contributor Author

zluiten commented Jan 16, 2013

Added fix.

Okay, this addresses the same issue as #3409, except that this also covers the problem that setInvokableClass, setFactory and setService don't respect the shareByDefault value when shared parameter is not provided (it's default true). Therefore I'm just leaving it open for now, but I'm fine if it will be fixed in the other PR ofcourse.

@tux-rampage
Copy link
Contributor

You mean if $shared is omitted for these methods?
Sounds reasonable to me. But I guess this would require a RFC?
I'm fine with adding this in the current or a separate PR.

@weierophinney How should this be adressed? Open a separate Issue?

Edit: Sorry, I just saw you already added a fix for this to your PR

@ghost ghost assigned weierophinney Jan 21, 2013
weierophinney added a commit that referenced this pull request Jan 21, 2013
@weierophinney
[#3439] CS fixes
79f192a 
- Trailing whitespace
weierophinney added a commit that referenced this pull request Jan 21, 2013
@weierophinney
Merge branch 'hotfix/3439' into develop
d4270ef 
Forward port #3439
weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney
[zendframework/zendframework#3439] CS fixes
37ea59a 
- Trailing whitespace
weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney
Merge branch 'hotfix/3439'
ff8bdbb 
Close zendframework/zendframework#3439
Fixes zendframework/zendframework#3409
weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney
Merge branch 'hotfix/3439' into develop
b3220c5 
Forward port zendframework/zendframework#3439
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@weierophinney weierophinney

Labels
None yet
Projects
None yet
Milestone
2.0.7
Development

Successfully merging this pull request may close these issues.
3 participants
@zluiten @tux-rampage @weierophinney