laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

Facades always returning a singleton #1088

Open JayBizzle opened 6 years ago

JayBizzle commented 6 years ago

Is there any reason or design decision that dictates that the use of a Facade always returns a singleton, and ignores the binding method specified in the ServiceProvider?

Patryk27 commented 6 years ago

Using facades does not ignore bindings done by the service providers - could you provide a code example?

sisve commented 6 years ago

The Facade::resolveFacadeInstance method will cache the resolved instance. That one instance is reused, like a singleton would be, even if the service is registered in a way that should not be a singleton.

https://github.com/laravel/framework/blob/5fa5a65e14f1683dba1eeee41431bf653711b0ff/src/Illuminate/Support/Facades/Facade.php#L155-L159

JayBizzle commented 6 years ago

@Patryk27 As @sisve has pointed out, no matter if in your service provider you use $this->app->singleton or $this->app->bind, if you use a Facade, a singleton will always be returned because the Facade class keeps it's own record of what has been instantiated.

My question is, was this a design decision or an oversight?

Patryk27 commented 6 years ago

This seems like a completely valid design decision to me - facades were meant to be a nice & easy implementation of the singleton pattern, where you do not usually expect the underlying class to be switched.

Could you elaborate a bit more on your specific use case? Dynamically changing the facade's implementation seems like a bad architectural decision IMHO.

JayBizzle commented 6 years ago

We didn't have any real complex use case. We just had a helper Class we used to generate some bits of HTML, and after years of using Laravel, this tripped us up just recently when we were trying to use that helper Class multiple time in the same request.

I guess I wanted to clarify my understanding of how all this worked, rather than suggesting the way Facades work should be changed.

As a quick fix, we just overrode the resolveFacadeInstance() method that for that particular Facade.

sisve commented 6 years ago

This seems like a completely valid design decision to me - facades were meant to be a nice & easy implementation of the singleton pattern, where you do not usually expect the underlying class to be switched.

@Patryk27 This feels like an explanation "after the facts". Nothing in the documentation tells us that facades are singletons, or the backing service registrations should be singletons.

It would make sense to update the documentation with this information, or have the facade resolving code verify that the instance is registered as a singleton.

Patryk27 commented 6 years ago

@sisve: well, to be honest, the first sentence in the documentation is Facades provide a "static" interface to classes that are available in the application's service container - it already says: static (like singleton), which does not seem a bit, hm, dynamic :-) It could be stated explicitly though.

@JayBizzle: seems like you've implemented the builder pattern with your facade changing its inner state (that's why calling it later yields unexpected result) - is that right? What I believe you should have is something like:

class MyFacadeImplementation {

  public function foo() {
    return new FooBuilder();
  }

}

That is: you should keep the facade's state to its minimum.

sisve commented 6 years ago

@Patryk27 I disagree with your interpretation of the word static. Static means that you can call them without an object instance; you do not have to resolve anything from the container yourself before using it.

The quotes are there to confuse and perhaps hint of magic in the __callStatic and that it isn't really a static interface after all. Or perhaps they are there to mess with people and hint at a connection to interfaces containing static methods.

The word "interface" in that sentence is also easy to misinterpret. I think it should be understood as "a way to access", not an actual php interface.

On the other hand, the word "Facade" could also be arguable wrong... https://en.wikipedia.org/wiki/Facade_pattern

JayBizzle commented 6 years ago

@Patryk27 Judging by your first response in this thread, you seemed to imply that you also thought Facades adhered to whatever binding method was used in the service provider. Am I correct?

Patryk27 commented 6 years ago

@JayBizzle: my facades usually do not use any explicit binding - getFacadeAccessor() returns just the class name, without need to bind it anywhere, e.g.:

class MyFacade extends ... {

  public function getFacadeAccessor(): string {
    return \App\Services\Somewhere\Something::class;
  }

}

Thus not - I did not mean that.

I'm not saying that implying such behavior would be incorrect either - like you can see, this conversation emerged, so documentation would benefit from specifying directly what's happening under the hood, so people would know rather than guess.

JayBizzle commented 6 years ago

@Patryk27 Ah, I see. Most tutorials I have seen always create a ServiceProvider along with the Facade and I have always assumed this to be a required part of creating a Facade. Now I see that it is not, I can see how the Facade and ServiceProvider are separate and therefore work independently. Even in earlier versions of the Laravel documentation it mentions having to create a ServiceProvider. It no longer does.

The current documentation says...

When a user references any static method on the Cache facade, Laravel resolves the cache binding from the service container and runs the requested method (in this case, get) against that object.

service container links to here, which would make me believe classes accessed via a Facade, would be resolved using the binding method specified in the ServiceProvider, when in fact, the Facade class maintains its own "service container".

I do however, feel the documentation could be improved in this area to clarify all this.

tjallingt commented 6 years ago

Just ran into this unexpected behaviour. I expected laravel to execute the closure registered with app->bind() everytime I statically accessed the facade but instead i received the same instance over and over. Since app->singleton() exists shouln't the facade just rely on the service provider to register the services correctly?

Since facades behave differently from resolve()/app() and regular dependency injection in this way i'd have expected a note in the docs but there doesn't seem to be any.

mjeffe commented 6 years ago

I ran into this same issue. I agree with @JayBizzle, that the documentation should say something about this. Another reason Laravel's implementation of real-time facades is misleading is that the classes __construct() runs, so it behaves as a singleton class, not a static class.

olivernybroe commented 5 years ago

Same issue for me. First result when searching on Google is this thread which helps a lot.

However I will also admit that I did not check the docs about it before googling it.

autaut03 commented 5 years ago

Could someone point out why Facades don't rely on default container? Shouldn't it just do something alongside the lines resolve($this->face)->{$method}(...$arguments)? This would mean that the container itself decides whether it's a singleton or not and makes the behaviour more predictable overall.

fourstacks commented 5 years ago

For anyone reaching this thread and looking for a solution to this issue then consider adding the following to your facade:

/**
 * In Packing.php facade class
 */
protected static function getFacadeAccessor()
{
    self::clearResolvedInstance(PackingService::class);

    return PackingService::class;
}

As noted above by @sisve if you look in Illuminate\Support\FacadesFacade you see that the resolveFacadeInstance method will resolve the underlying class from the container the first time it's called then return the same object in subsequent calls. If your underlying class sets any state or builds up other classes then you could well run into issues.

By clearing the resolved instance (as seen in the snippet above) before the accessor is returned we can ensure that a new instance of the underlying class is called each time. Hope that helps if anyone else ended up scratching their heads on this one!

Broutard commented 4 years ago

Maybe the @fourstacks solution could be added to the documentation?