Better comments in the code

[halaei]

There are places in Laravel code that I wish for better comments. To me better is not necessarily more, but it is most of the time. I try to list some cases here. Examples are from laravel/horizon unless stated otherwise.

1. Architectural documentation

when you run artisan horizon, an instance of a master supervisor is created. Then, the master supervisor runs some artisan horizon:supervise commands to make supervisor processes. Each supervisor process runs some artisan horizon:work commands to create some worker processes. In other words master supervisor creates, monitors, controls and kills some supervisors and each supervisor creates, monitors, controls and kills some workers.

The problem

You only can know about this very high-level architecture by reading the code. To me as a user it is not a problem at first. The trouble starts when the current state of the code doesn't meet my specific needs in a project. Now I have to fix horizon, and in order to fix it, the first thing I need to know is to figure out the high level architecture. Please note that the terms master and supervisor are used in laravel horizon docs but they are not defined.

2. Classes needs to have comments

For supervisor, we have 2 classes: Supervisor and SupervisorProcess. It would make my understanding of code faster if the classes have some comments mentioning their responsibilities and relations with other classes so that I can easily understand them:

/**
 * A supervisor process is an object created by the master supervisor that contains
 * one child process which has a single supervisor object.
 *
 * @see MasterSupervisor
 * @see Supervisor
 */
class SupervisorProcess extends WorkerProcess

/**
 * A supervisor object manages a set of process pools each containing many worker process objects.
 *
 * @see ProcessPool
 * @see WorkerProcess
 */
class Supervisor implements Pausable, Restartable, Terminable

3. Don't document the obvious

The following comments about what __destruct() doesn't add value. Empty spaces will do better:

    /**
     * Destruct the object.
     *
     * @return void
     */
    public function __destruct()
    {
        //
    }

In this example, a documentation better than nothing is a why documentation, which is in the following section.

4. More why documentations

An empty __destruct() is probably there to disable the parent destructor:

class BackgroundProcess extends Process
{
    /**
     * An empty __destruct() to prevent the parent destructor from killing the child process.
     *
     * @return void
     */
    public function __destruct()
    {
        //
    }
}

For another example, Horizon\RedisQueue extending from Illuminate\Queue\RedisQueue and overrides getRandomId() function. The comments on this method are just a copy paste from the parent.

    /**
     * Get a random ID string.
     *
     * @return string
     */
    protected function getRandomId()
    {
        return JobId::generate();
    }

But a better comment would be the reason behind overriding, which I can't figure out yet:

    /**
     * Returns an auto-increment integer id instead of random string.
     * Horizon needs job ids to be auto-incremental because **I DON'T KNOW WHY**.
     *
     * @return string
     */
    protected function getRandomId()
    {
        return JobId::generate();
    }

5. Type-hinting closure parameters

This is not something entirely missing in Laravel, but I wish for more.

        return collect($this->processes)->filter(function (WorkerProcess $process) {
            return $process->process->isRunning();
        });

6. Inline docblocks to type hint local variables

Apart from tests, I have seen zero inline phpdoc for local variables in Laravel sources. I have no specific example for this one, but sometimes I feel Laravel core developers are against inline dockblocks. Maybe they have some good reason for that, or I am totally wrong on this case.

7. Type hint array/collection elements

Basically, string[] is more useful than array. It is also nice to write Collection|ProcessPool[] instead of just Collection:

    /**
     * All of the active processes.
     *
     * @var WorkerProcess[]
     */
    public $processes = [];

    /**
     * All of the process pools being managed.
     *
     * @var \Illuminate\Support\Collection|ProcessPool[]
     */
    public $processPools;

7. Documenting magic properties

Maybe this is also not entirely missing, but it is missing in many model classes across the laravel projects. Here is an example in laravel/passport:

/**
 * @property int $id
 * @property int $user_id
 * @property int $client_id
 * @property string $name
 * @property string $scopes
 * @property bool $revoked
 * @property Carbon $created_at
 * @property Carbon $updated_at
 * @property Carbon $expires_at
 *
 * @property-read Passport $client
 * @property-read $user
 */
class Token extends Model

[mfn]

Maybe this is also not entirely missing, but it is missing in many model classes across the laravel projects. Here is an example in laravel/passport:

I've yet so see a better solution then https://github.com/barryvdh/laravel-ide-helper which however doesn't extend to libraries, only your application.

[halaei]

@mfn to me the only thing that matters is for @property lines to be there, no matter how. Also, I don't like it to be in a separated ide_helper file and don't like to have things that I don't use, like @method whereUserId($value).

[mfn]

I think you misunderstand how laravel-ide-helper works here, for the models it generates them inline exactly were you wrote them (but more correct, introspects the database, understands $casts, etc.).

[halaei]

Yes, I know that. I have tried it before. It is great, except it used to add things that I didn't liked e.g. whereUserId() methods, which I could delete easily. I don't know what it does now. Anyway, I wish the comments to be in Laravel packages 🙂

[ludo237]

I honestly would prefer native comments instead of laravel-ide-helper

[klimov-paul]

Each class, trait or interface provided but public library such as framework should be supplied with PHPDoc describing its purpose and basic usage.

Take Symfony for example, which serves as a foundation for many Laravel features. Here is a PHPDoc of the Symfony\Component\Console\Application:

/**
 * An Application is the container for a collection of commands.
 *
 * It is the main entry point of a Console application.
 *
 * This class is optimized for a standard CLI environment.
 *
 * Usage:
 *
 *     $app = new Application('myapp', '1.0 (stable)');
 *     $app->add(new SimpleCommand());
 *     $app->run();
 *
 * @author Fabien Potencier <fabien@symfony.com>
 */
class Application
{
    // ...
}

You can clearly understand the purpose behind this class as a "container for a collection of commands" without necessity to dig up its source code. Here you can also see a basic code snippet showing its usage example.

Next here is a PHPDoc of the Application::__construct():

/**
 * @param string $name    The name of the application
 * @param string $version The version of the application
 */
public function __construct(string $name = 'UNKNOWN', string $version = 'UNKNOWN')
{
    // ...
}

It clearly indicates that $name is a name of application - not a name of its author or something else.

Although I would say Symfony PHPDoc is still rather poor. The best examples of PHPDoc usage I can recall does to Yii2. See a PHPDoc of yii\db\Connection class. It is almost a full-scale manual, indicating it uses PDO (providing link to its php.net page), showing configuration and usage examples.

With the class doc blocks you can get all the information, necessary for programming, without leaving IDE window and without necessity to browse over web documentation (which is also quite poor).

It is a pity Laravel ignores the benefits of PHPDoc.

[klimov-paul]

As for barryvdh/laravel-ide-helper - it is quite a nonsense I require a 3rd party library to get a proper support for IDE basic features. I understand that providing a IDE type-hint for some features can be problematic. For example:

$dbConnection = App::get('db.connection');
// it hard to make every IDE provide type hint for `$dbConnection` variable here

But when it comes to easy to implement things, like adding virtual methods and properties to PHPDoc of Eloquent or Facades classes, the team refusal to do so, leaves me in perplexity.

Their arguments sounds like following:

if you change the code, you may not ware of this and your list in PHPDoc is already out of date. the Facades to get out of sync very now and then

which is also pitiful.

Back in Yii development we have created a special deployment script, which is running before each release ensuring PHPDocs for virtual methods and attributes are up-to-date. You can see an example here: https://github.com/yiisoft/yii2/blob/master/build/controllers/PhpDocController.php

Many developers pay good money for the modern IDE such as PHPStorm expecting it will ease their work. But Laravel team states that support of the IDE basic features, like type-hinting, is not their concern. Is this a treatment, which Laravel community deserves?

[mattstauffer]

@klimov-paul Are you aware of how condescending and entitled you're coming across? Is this a language or culture issue, or are you just used to talking down to people?

I ask this honestly because my first response is that you're being rude and condescending and entitled and you can go back to Yii if that's how you want to treat people. However, I'm fully aware that I've thought that in the past and it was a language or a culture issue, so I'm trying to give space that maybe I'm just reading you wrong.

Could you clarify? Thanks!

[klimov-paul]

@mattstauffer, I do not want to offend anyone. I apologize, if I did. You are free to edit my comments in the way you consider appropriate.

[halaei]

Although my concerns in this issue are not all about IDE support, but sometimes I feel the benefit of "helping IDE so that it can help us" is a bit under estimated in Laravel.

[halaei]

Also I think some of the code style rules are too strict to be helpful, some of them are even wrong. Those @return void above every single constructor!

[klimov-paul]

Unfortunally, there is no clear standard for PHPDoc specification. Usually PHPDocumentor is considered to be a common trends provider as it is common application for API doc generation.

Such things like having @return void is quite a debatable, however since PHP itself allows type restriction void over the function return value, it can be justified:

/**
 * If PHP allows restriction of the return to `void`, why PHPDoc should not?
 *
 * @return void
 */
public function foo(): void
{
    ...
}

[halaei]

That can be debatable but here is my opinion. @ retuen void for constructor adds none to the understanding of the code. Why should someone directly calls constructor and uses its return value? That is not even in the background of anyone's mind so no need to comment and say don't expect a return value here. I am a fan on the way C++ treats constructors, they don't have return type, not even void.

[klimov-paul]

Also relates to https://github.com/laravel/framework/pull/28188 and https://github.com/laravel/framework/pull/24647