[Proposal] generate class doc blocks with real time facades

[Evertt]

Right now the facades included in laravel, like Illuminate\Support\Facades\Route contain class doc blocks so that IDEs such as PhpStorm can autocomplete their methods. That is awesome and I would like the same for real time facades. You know, any class prefixed with the namespace Facades\.

I don't know how the doc blocks have been made for the normal facades, but my guess is that they've been generated by some tool, right? If so, then that same tool can be used to scan classes that are references as real-time facades, is that correct? If not then I'm confident such a tool can be made, maybe even mostly copied from Barry's ide-helper (if he's okay with that of course).

[m1guelpf]

@Evertt

maybe even mostly copied from Barry's ide-helper (if he's okay with that of course).

I'd say this is most a thing that should be included with Barry's ide-helper...

[Evertt]

@m1guelpf but why? You could say the same thing about the "normal" facades, but apparently people saw value in having those doc-blocked by default without the need of an extra dependency and extra action to perform to make that work.

I see no reason why we couldn't implement this in Laravel as well. We're already generating a file anyway, namely the facade file, so why not generate some doc blocks with them too?

[sisve]

The normal facades does not have docblocks covering the methods. (Ref: DB, Schema, Queue).

The ide-helper package uses reflection to resolve the actual instances behind the facades, and generates an _ide_helper.php file that phpstorm parses.

[Evertt]

@sisve Okay apparently not all, but these do:

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Support/Facades/App.php https://github.com/laravel/framework/blob/5.4/src/Illuminate/Support/Facades/Artisan.php https://github.com/laravel/framework/blob/5.4/src/Illuminate/Support/Facades/Auth.php https://github.com/laravel/framework/blob/5.4/src/Illuminate/Support/Facades/Route.php

So that signals to me that they aren't generated automatically, but that Taylor just wrote them manually for some facades. I'd say then using a tool to write those automatically is still preferred.

Okay so I agree that since such a tool would be a non-trivial undertaking, it wouldn't make sense to make that part of Laravel's core. However, couldn't we make such a tool in a separate repo (or ask Barry to adept his tool) and simply require it as a dependency of laravel?

edit

We could even make it so that the tool isn't automatically added as a dependency, but rather as a suggestion in composer.json. And possibly as a dev-dependency anyway, so that we can use it to generate the class doc blocks for the default laravel facades.

// composer.json

{
    "require-dev": {
        "barryvdh/laravel-ide-helper": "~2.3.2"
    },
    "suggest": {
        "barryvdh/laravel-ide-helper": "Required to automatically generate doc blocks for real-time facades."
    },
}

Then in the code we check whether the tool is available. If so, use it. If not, skip that part of the generation.

[alexbowers]

I'd love for all facades to have the docblocks setup.

@taylorotwell is there a reason this is not the case? Or is it just that you don't want to spend time doing it, but are willing to accept a PR that does this?

It'd be very helpful for IDE users for autocomplete, source jumping, and documentation purposes.

[m1guelpf]

@alexbowers I'd say that if you do it manually, you'll have to add a reminder somewhere, or new methods added will make them outdated

[alexbowers]

It probably wouldn't be too difficult to use something based on the ide helper to see whether any methods are missing and have that run as a script on each PR. Similar to Travis and StyleCI, there could be one to check for valid docblocks

[Evertt]

@alexbowers, my thought exactly!

[barryvdh]

I could probaby create a simple command that adds those methods. But most of the time you could just add @mixin \RealClass to import the methods dynamically. But that doesn't work for 'real time facades', because they're nog real facades. (But the IDE should be able to do that probably)

[alexbowers]

Huh never knew about @mixin is that supported by PHP storm?

If it is part of the standard it'd be great to get that done. I can perhaps look at it this evening depending what time I get back home from the conference

On 22 Jun 2017, at 07:57, Barry vd. Heuvel notifications@github.com wrote:

I could probaby create a simple command that adds those methods. But most of the time you could just add @mixin \RealClass to import the methods dynamically. But that doesn't work for 'real time facades', because they're nog real facades. (But the IDE should be able to do that probably)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[barryvdh]

It's not part of the standard.

[alexbowers]

That's a shame. Might be worth this change anyway for the places that do allow for that so block

On 22 Jun 2017, at 08:52, Barry vd. Heuvel notifications@github.com wrote:

It's not part of the standard.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[Evertt]

@barryvdh and @alexbowers but @mixin doesn't work for facades. Since on facades you call all methods statically and when you add @mixin Real\Class above a facade then PhpStorm says all methods should be called non-statically.

[Evertt]

@barryvdh

I could probaby create a simple command that adds those methods.

You don't mean a console command, right? What we would need is to be able to implement in laravel the following code.

namespace Illuminate\Foundation;

class AliasLoader
{
    function createDocBlocksForRealTimeFacade($originalClass)
    {
        if (!class_exists('Barryvdh\LaravelIdeHelper\Analyzer')) return;

        $analyzer = new \Barryvdh\LaravelIdeHelper\Analyzer($originalClass);

        foreach($analyzer->methods() as $method) {
            $this->addMethodDocBlockToFacade('static', $method->returnType, $method->signature);
            // This would output something like @method static \Some\Type getSomething(string $value)
        }
    }
}

Could you write a class that does something like that?

[barryvdh]

At runtime? Wouldn't do that.

[alexbowers]

I agree runtime wouldn't be good.

This should be a command that can be ran once per version for example to ensure the tags are correct on each facade class

On 26 Jun 2017, at 08:55, Barry vd. Heuvel notifications@github.com wrote:

At runtime? Wouldn't do that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[barryvdh]

I think there are 2 different things here:

• phpdocs for the internal Laravel classes, which a command could fix to add them before each release (eg. a pre-commit hook to add them every time)
  • phpdocs for 'realtime facades', which aren't even classes. For this, a command could be created which would have to scan the project for those facades and add them to the ide-helper file. But that's an issue for ide-helper.

[alexbowers]

Yeah, in my view this discussion is only relevant for the first point and a discussion needs to start in your helper module for the second point

On 26 Jun 2017, at 09:03, Barry vd. Heuvel notifications@github.com wrote:

I think there are 2 different things here:

phpdocs for the internal Laravel classes, which a command could fix to add them before each release (eg. a pre-commit hook to add them every time) phpdocs for 'realtime facades', which aren't even classes. For this, a command could be created which would have to scan the project for those facades and add them to the ide-helper file. But that's an issue for ide-helper. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[Evertt]

At runtime? Wouldn't do that.

Why not? It's only run once per real-time facade and then they're cached forever.

[barryvdh]

Because the www group wouldn't have access to the files usually. And I would be a bit hesitant with code altering code like that

[barryvdh]

But as said, that's a ide-helper issue, not for Laravel. Because the realtime classes don't even exist in the code.

[alexbowers]

@barryvdh What would be the best way for this to be done for the existing facades in Laravel?

You mentioned a pre-commit hook. Is there a way to ensure this is tracked and utilised?

[barryvdh]

Uhh not sure. You could just try to remember a command once in a while, but otherwise there are thing s like this: https://github.com/phpro/grumphp That adds hooks for tasks you can run on pre-commit hooks

[Evertt]

@barryvdh

Because the www group wouldn't have access to the files usually. And I would be a bit hesitant with code altering code like that

I'm afraid we're having a misunderstanding here. I am talking about altering the code that Taylor already made which generates facades real-time whenever a namespace is prefixed with Facades\. That code already exists and works. All I'm asking is to add a little Reflection code to it so that the generated facade will have some extra doc-blocks. This is done only once, since facades are then cached in the storage/framework/cache/ folder so it's not a performance issue to worry about. And if you think you have to worry about the www group then that would be the same problem for ALL real-time facades, regardless of adding doc blocks to them.

Do you understand what I'm saying?

[barryvdh]

Ohhh the realtime facades are actually created as real files in the storage dir, which you are indexing?

So you would need to add some reflection here: https://github.com/laravel/framework/blob/7255de376e6834911d2672b12dc6d6a4c18dda70/src/Illuminate/Foundation/AliasLoader.php#L94-L131

I guess in that case, it is kind of similar to regular facades indeed.

[barryvdh]

But it would still be possible with just ide-helper, by scanning that folder + changing the phpdocs (with a command). I doubt Taylor would approve this in the core, but otherwise willing to help out.

[Evertt]

@barryvdh Yaay for clarity and understanding. So yeah I would find it acceptable to have it as an added feature to the ide-helper. I was hoping that we could have a tool which automatically writes doc-blocks for the regular facades using a before-commit-hook or something like that and it sounded like everyone agreed that would be a good idea. And if that is the case then you could kill two birds with one stone if you'd then use that same tool to automatically generate doc-blocks for real-time facades.