Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Removes core class aliases from the skeleton #40062

Merged
merged 6 commits into from
Dec 20, 2021

Conversation

nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Dec 15, 2021

This pull request allows removing core class aliases from the config/app.php from the skeleton in Laravel 9 — about 46 lines removed: laravel/laravel#5751.

The motivation for this pull request is the same #40034, those aliases are rarely (never?) modified at user-land. So, it's kind of safe to move them to the core, giving the skeleton a little bit more space to breathe.

Now, the approach of this pull request is simple, if this pull request gets merged, the core aliases are moved to a Facades::defaultAliases(), and the skeleton app.aliases calls that method to get the core aliases. Allowing the user to still fully disable this feature by removing the app.aliases configuration key.

Is worth mentioning that this pull request does not contain breaking changes. Also, if this pull request gets merged, I will remove this section in the docs too: https://laravel.com/docs/8.x/redis#the-redis-facade-alias.

@taylorotwell
Copy link
Member

One small caveat to this approach is if you want to define even one additional alias for their own classes you now need to copy this entire list into the skeleton.

You could get around that by making the laravel/laravel config like this in Laravel 9:

'aliases' => [
    ...Application::facadeAliases(),
    // ...
],

@nunomaduro
Copy link
Member Author

@taylorotwell @igorgaming Updated the description of the pull request in light of our recent discussion.

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user wishes to add more aliases or override existing ones

How would I go about replacing the default ones?

Mine look currently like this:

    'aliases' => [
        // Necessary due to use as `@mixin \Eloquent` in models due to ide-helper
        'Eloquent' => Illuminate\Database\Eloquent\Model::class,
    ],

And that is only out of necessity with ide-helper, actually.

I read the arguments about aliases being lazy and thus having no impact, but I can also tell from my experience, developers want to have specific control over this and want to make sure they're disabled so they're not getting accidentally used.

Thanks

$this->assertCount(39, AliasLoader::getInstance()->getAliases());
}

protected function getDefaultAliases()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this method should be part of class RegisterFacades so the array below does not have to be duplicated in tests?

@koenhoeijmakers
Copy link
Contributor

Wouldn't this make it way more inconvenient to disable all aliases?

Use case: i preferably remove all aliases so that our team properly uses the class definition so that every facade used also gives the expected intellisense

@taylorotwell
Copy link
Member

taylorotwell commented Dec 17, 2021

We should make the app.aliases configuration entry look like this:

'aliases' => [
    ...Facade::aliases(),
    // ...
],

Then, if someone wants to totally disable the built-in aliases they can easily do so. In addition, they can disable the built-in aliases while still retaining the ability to define their own.

Edit

Well, unpacking arrays with string keys only works with PHP 8.1+ so the configuration would have to look like:

'aliases' => Facade::defaultAliases() + [
    // ...
],

@taylorotwell taylorotwell marked this pull request as draft December 17, 2021 15:17
@nunomaduro nunomaduro marked this pull request as ready for review December 17, 2021 21:49
@nunomaduro
Copy link
Member Author

nunomaduro commented Dec 17, 2021

@taylorotwell Have applied the requested changes in both pull requests, and updated this PR description. Worth to mention that the result of the operation bellow won't override the default View alias. People would have to sum the Facade::defaultAliases() as last element.

Facade::defaultAliases() + [
    'View' => 'Nuno',
],

@BrandonSurowiec
Copy link
Contributor

Would it be weird to do:

'aliases' => Facade::defaultAliases([
    'View' => 'Nuno',
]),

Then the Facade::defaultAliases could merge the array on the inside. To override with the current setup, we'd array_merge or use the + operator:

'aliases' => array_merge(Facade::defaultAliases(), [
    'View' => 'Nuno',
]),
'aliases' => [
    'View' => 'Nuno',
] + Facade::defaultAliases(),

@mfn
Copy link
Contributor

mfn commented Dec 18, 2021

Would it be weird to do:

TBH, the moment a method is being called in the config file to provide the defaults (for backwards compat), started to look weird to me. Though I do in fact appreciate the suggestion because exactly of "backwards compat".

@taylorotwell taylorotwell merged commit 3ad64c5 into master Dec 20, 2021
@taylorotwell taylorotwell deleted the feat/remove-aliases branch December 20, 2021 19:44
taylorotwell added a commit that referenced this pull request Dec 20, 2021
* Allows to remove core class `app.aliases` from the skeleton

* Removes `Redis` alias

* Merges configurations

* Reverts changes on Register Facades

* Adds `Facade::defaultAliases`

* Apply fixes from StyleCI

Co-authored-by: Taylor Otwell <taylorotwell@users.noreply.github.com>
@RoodFruit
Copy link

@nunomaduro Heads-up, in the latest Laravel 9.0.0-beta.3 branch, Facade::defaultAliases() now returns a Collection. Meaning the array_merge() in app.php won't work anymore.

Easy fix of course by adding ->toArray().

    'aliases' => array_merge(Facade::defaultAliases()->toArray(), [
        // ...
    ]),

@driesvints
Copy link
Member

@RoodFruit this is already fixed on master.

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

Successfully merging this pull request may close these issues.

None yet

8 participants