barryvdh / laravel-ide-helper

IDE Helper for Laravel
MIT License
14.19k stars 1.16k forks source link

@mixin \Eloquent is hardcoded and facades are optionals #811

Open pablorsk opened 5 years ago

pablorsk commented 5 years ago

Problem

Each model receive a mixin with Eloquent facade, but sometimes facades are disabled (like Lumen) or just commented on Laravel app.php.

Proposed solution, add this to config.php:

    // default
    'eloquent_mixins' => [
        '\Eloquent'
    ]

Another option, for a case without facades

    'eloquent_mixins' => [
        \Illuminate\Database\Eloquent\Model::class,
        \Illuminate\Database\Eloquent\Builder:class
    ]

Then this option can change phpblockdoc on models

 * @mixin \Eloquent

Alternative solution for Lumen

Add a hack to AppServiceProvider.php:boot() like says @ihipop on https://github.com/barryvdh/laravel-ide-helper/issues/611#issuecomment-361163745

Alternative solution for Laravel (facades disabled)

namespace App\Providers;

use Illuminate\Foundation\AliasLoader;
use Illuminate\Support\ServiceProvider;

class AppServiceProvider extends ServiceProvider
{
    public function boot(): void
    {
        if ($this->app->environment() !== 'production') {
            AliasLoader::getInstance()->alias('Eloquent', \Illuminate\Database\Eloquent\Model::class);
        }
    }

    public function register(): void
    {
        if ($this->app->environment() !== 'production') {
            $this->app->register(\Barryvdh\LaravelIdeHelper\IdeHelperServiceProvider::class);
        }
    }
}

Related with #611 and #756 (finishing words)

mfn commented 5 years ago

Why should the package default to Facades at all, when they're optional but the actual classes which can be referenced are always there?

I mean, why make it configurable / more complex when we could just always refer to the destination classes anyway.

Seems to me that the decision to use Facades here just brings problems to the table anyway.

pablorsk commented 5 years ago

Hi @mfn,

For perfomance and better code reasons, we don't use aliases, like Lumen.

On this case, I think, we need change @mixin \Eloquent because is not available.

mfn commented 5 years ago

Yes, as I said, the aliases are optional so why should this package ever depend on it.

nelson6e65 commented 5 years ago

It's there a way to disable the insertion of @mixin \Eloquent in my model's DocBlock?

If I run php artisan ide-helper:models -W, * @mixin \Eloquent is added:

<?php

namespace App\Entities;

use Illuminate\Database\Eloquent\Model;

use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Relations\BelongsTo;

/**
 * 
 *
 * @property int $id
 * @property string|null $name
 * [...]
 * @mixin \Eloquent
 */
class Company extends Model
{
    // ...
}
stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this issue is still present on the latest version of this library on supported Laravel versions, please let us know by replying to this issue so we can investigate further. Thank you for your contribution! Apologies for any delayed response on our side.

barryvdh commented 4 years ago

It shouldn't be added by default, but only when the config is set to true: https://github.com/barryvdh/laravel-ide-helper/blob/4b459d3e828fedb0fa5455fe6d25af17130635cb/config/ide-helper.php#L87 Did you publish your config and made changes?

mfn commented 4 years ago

It shouldn't be added by default, but only when the config is set to true:

@barryvdh this is a different settings 😅

So in short, currently there's no way to prevent this.

Before adding a new config I would suggest to see if we can detect if the Facade alias exists and, if not, don't write the mixin there.

mfn commented 4 years ago

(adapted the labels as this is rather an enhancement)

ptejada commented 4 years ago

Sound a like reasonable enhancement to have. I am surprised there still no PR for it.