Should ScopeManager include a method `deactivate`?

[jcchavezs]

Background

When implementing the in process context propagation there is a need to notify the scope manager that a scope is closed. Implementations usually exposes a method called deactivate to do so:

final class Scope implements OpenTracingScope
{
    ...
    public function __construct(ScopeManager $scopeManager, OpenTracingSpan $span, $finishSpanOnClose)
    {
        $this->scopeManager = $scopeManager;
        $this->span = $span;
        $this->finishSpanOnClose = $finishSpanOnClose;
    }
    ...
    public function close()
    {
        if ($this->finishSpanOnClose) {
            $this->span->finish();
        }

        $this->scopeManager->deactivate($this);
    }
    ...
}

Problem

The problem is that since ScopeManager::deactivate is not part of the ScopeManager interface then it makes it impossible to use a noop instead of the real implementations for example in tests.

Proposal

Including ScopeManager::deactivate would allow us to include that method in a Noop implementation following the dependency inversion principle.

Thoughts?

@tedsuo @yurishkuro @bhs @beberlei @palazzem @carlosalberto

[jcchavezs]

Ping @ellisv

[ellisv]

:-1:

There is no need to expose deactivate() in the interface in my opinion. You do not create Scope objects yourself but your ScopeManager does so. So there is no need for dependency inversion there.

So I imagine you are trying to do something like this

class MyScopeManager implements ScopeManager
{
    // ...

    public function activate(Span $span, $finishOnClose)
    {
        return new MyScope($this, $span, $finishOnClose);
    }

    // ...
}

class MyScope implements Scope
{
    // Notice a typehint of ScopeManager interface instead of actual implementation
    public function __construct(ScopeManager $scopeManager, Span $span, $finishOnClose)
    {
        // ...
    }

    // ...

    public function close()
    {
        // You are lacking ScopeManager::deactive() here as you typehint on the interface
    }
}

But in that case you do not need to abstract typehinting on any ScopeManager implementation but rather you should typehint on actual ScopeManager implementation as Scope is created by ScopeManager itself only. So:

class MyScopeManager implements ScopeManager
{
    // ...

    public function activate(Span $span, $finishOnClose)
    {
        return new MyScope($this, $span, $finishOnClose);
    }

    // ...
}

class MyScope implements Scope
{
    // No need for dependency inversion for $scopeManager as this Scope is only created by
    // ScopeManager only
    public function __construct(MyScopeManager $scopeManager, Span $span, $finishOnClose)
    {
        // ...
    }

    // ...

    public function close()
    {
        // Now you typehinted on actual implementation so you can use any new methods
        // you introduced in implementation
    }
}

Or am I failing to see the case why you want to have deactivate() on the interface? Could you then provide with examples?

[jcchavezs]

I gave a second thought to this and arrived to the same conclusion as you, @ellisv. Closing this.