Skip to content
This repository was archived by the owner on Jul 16, 2021. It is now read-only.

[Proposal] Remove facades in Laravel 6.0 #1508

Closed
xbit opened this issue Feb 4, 2019 · 15 comments
Closed

[Proposal] Remove facades in Laravel 6.0 #1508

xbit opened this issue Feb 4, 2019 · 15 comments

Comments

@xbit
Copy link

xbit commented Feb 4, 2019

Laravel 6.0 would be an excellent opportunity to remove facades because breaking changes are expected.

As Taylor has stated before and many others, it can lead to bad architectural habits and encourages developers especially beginners to write bad code.

Type hinting dependencies is super simple, and I don't see a reason why facades should continue to exist in the framework. I believe that many would agree. I understand that it can be difficult to remove them in a minor version, but Laravel 6.0 would be an excellent opportunity to do that.

I guess that the major argument against this would be breaking changes, but it takes time for a framework to grow and mature so this is in my opinion a good step in the right direction.

Edit: It may also be possible to move them into a separate package, but I'm not sure if this is trivial.

@mfn
Copy link

mfn commented Feb 4, 2019

I don't see much value by removing them or what the removal really "fixes", as there's nothing broken per se.

By and large they're optional and although I avoid them, I use a very few selected ones because there are times when they're "just very convenient".

And, to me, Laravel is very much about convenience. It sometimes is convenient to have cached singleton instance available everywhere.

The documentation features an entire chapter on them, including some hints when to avoid it.

I think inexperienced developers are challenged more by other things, writing god-like controller actions and having no sense of separation of concerns, etc. Removing Facades won't fix the knowledge of beginners.

It's not like, here comes my favorite topic 😜, global helper functions Laravel provides. They are forced, can't opt-out and a PITA if you have a clash with existing (mostly legacy) software and you can't change either. Total bummer. In contrast, Facades have no such problem.

@xbit
Copy link
Author

xbit commented Feb 5, 2019

I totally agree with you about the global helper functions. What are the use cases in which you find facades "just very convenient" and an alternative method wouldn't work (or wouldn't provide a similar level of convenience). Can you provide an example?

IMHO, in most cases, they do more harm than good.

@mfn
Copy link

mfn commented Feb 5, 2019

and an alternative method wouldn't work (or wouldn't provide a similar level of convenience).

I think it's important to emphasize what you wrote in parenthesis. Because obviously we all know Facades are just shortcuts so technically, "wouldn't work" can never apply.

I just checked the my "reference codebase" how I'd like to call it :p

  • Logging, definitely
  • Quickly accessing the \DB service, very useful
  • Validator
  • App::environment pops up a lot
  • Auth::user() is super useful
  • Event::

Also from 3rd party packages I use:

  • GraphQL::, very very useful especially when declaring the whole type system programmatically
  • Agent:: (user agent checking, useful mostly in templates)

Both lists aren't even complete, I should have stopped at the first point because to me, without question, they definitely have a place (to stay) 😄

@xbit
Copy link
Author

xbit commented Feb 5, 2019

What's the advantage here? You can use type hinting.

Route::get('/', function (Application $app, AuthManager $auth, Dispatcher $event) {
        $app->environment()
        $auth->user()
        $event->...
});

You can also do exactly the same if you need a quick way to access these instances:

app('auth')->user()
app('events')->...

I think it's important to emphasize what you wrote in parenthesis

I said "(or wouldn't provide a similar level of convenience)"
The above provides a similar level of convenience

Edit: in fact, it appears that you're using them too much :)

@ludo237
Copy link

ludo237 commented Feb 5, 2019

It such an optional way of coding that it doesn't really matter if they exists or not in my opinion.

I've just checked on an enterprise code base in my company (an internal CMR built using Laravel) and the /app folder does not rely on facades at all (across almost a thousands of files not ten)

But tests, o gosh tests, stuff like Storage::shouldReceived()... or UploadedFile::fake() are gold and they should remain.

@thannaske
Copy link

What's the advantage here? You can use type hinting.

Yeah. I use controller actions where I use type-hinting to import some of my own Repository classes and I end up with around three or four arguments per function. It would drive me nuts if I would be required to type-hint everything I want to use in the function. Imagine a controller function like this

public function store(Item $item, Request $request, User $user, Application $application, Dispatcher $dispatcher, Cache $cache) { ... }

Sorry but this isn't convenient at all. As a framework-developer I want to have the freedom to choose when I use facades and when not. When I was a Laravel beginner Facades where one of the main reasons why I choose Laravel over CakePHP or some other PHP framework as I did not have to worry about dependency injection or other concepts I did not understand at this moment.

@xbit
Copy link
Author

xbit commented Feb 5, 2019

@thannaske No you don't have to do that. If you have common dependencies, you can do this.

  class HomeController extends Controller
  {
       private $app;
       private $dispatcher;
       private $cache;

      public function __construct(Application $application, Dispatcher $dispatcher, Cache $cache)
      {
           $this->app = $application;
           $this->dispatcher = $dispatcher;
           $this->cache = $cache;
       }

    public function store(Item $item) {
        $this->app
        $this->dispatcher
        $item
          ....
    }
 }

I did not have to worry about dependency injection or other concepts I did not understand at this moment.

the framework should guide to do it the right way but not encourage to tightly couple everything.

@mfn
Copy link

mfn commented Feb 5, 2019

If you have common dependencies

Yeah, at which point you can as well just write app('cache') and soon we're back at Cache:: :-)

@thannaske
Copy link

If you have common dependencies, you can do this.

May be "better style" but I still don't like that way of composing controllers. If you don't want to use facades just remove them from your configuration and everybody is happy.

@martinbean
Copy link

martinbean commented Feb 5, 2019

You can use type hinting.

@xbit You’re right. You can. Facades’ existence doesn’t stop us type-hinting and injecting dependencies if we prefer to 🙂

@michaeldyrynda
Copy link

I think you'll find Taylor's point of view on facades has changed quite a bit since that article you linked to... from 2014...

They're testable, you don't have to use them (you can inject the underlying classes if you want to 👨🏻‍🚀), and they're so damn convenient.

Let developers figure out if / when they want to use them. Chances are, if they're not aware that tightly coupling to the framework might be a problem, the app won't reach a size and complexity that it'll be an issue anyway.

@xbit
Copy link
Author

xbit commented Feb 6, 2019

Let developers figure out if / when they want to use them. Chances are, if they're not aware that tightly coupling to the framework might be a problem, the app won't reach a size and complexity that it'll be an issue anyway.

I think this is a valid point. I still believe they should be removed. I don't think there is anything useful left to add to this discussion, so there's no point in keeping it open.

@xbit xbit closed this as completed Feb 6, 2019
@barryvdh
Copy link

barryvdh commented Mar 4, 2019

I wouldn't remove Facades, but I would consider removing the dynamic class_alias call, which doesn't add much values anymore, because everything is already namespaced.

@winex01
Copy link

winex01 commented May 24, 2019

laravel is freedom. :)

@TomasVotruba
Copy link

TomasVotruba commented Dec 8, 2020

@barryvdh Hi Barry, to your last comment...

I wrote a post about it and how to do it in short time :) ↓
Laravel Facades to Constructor Injection: Replace Facade Aliases with Full Classes in 2 hours

I'd be greatful for feedback on what to improve or do better 🙇

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

No branches or pull requests

9 participants