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

Closed
wants to merge 2 commits into
from

3 participants

@netiul

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.

@netiul

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.

@netiul netiul added a commit that referenced this pull request Jan 16, 2013
@netiul netiul fix for zendframework/zendframework#3439 9d600ef
@netiul

Added fix.

Okay, this addresses the same issue as zendframework/zendframework#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

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

@weierophinney weierophinney added a commit that referenced this pull request Jan 21, 2013
@weierophinney weierophinney [#3439] CS fixes
- Trailing whitespace
79f192a
@weierophinney weierophinney added a commit that referenced this pull request Jan 21, 2013
@weierophinney weierophinney Merge branch 'hotfix/3439' into develop
Forward port #3439
d4270ef
@weierophinney weierophinney added a commit that closed this pull request Jan 21, 2013
@weierophinney weierophinney Merge branch 'hotfix/3439'
Close #3439
Fixes #3409
b756cd7
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@netiul netiul fix for zendframework/zendframework#3439 c5d929f
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney [#3439] CS fixes
- Trailing whitespace
54bf7d5
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'hotfix/3439'
Close #3439
Fixes #3409
325909c
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'hotfix/3439' into develop
Forward port #3439
f6cdac0
@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#3439] CS fixes
- Trailing whitespace
37ea59a
@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/3439' ff8bdbb
@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/3439' into develop b3220c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment