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

Usage of facade aliases causes exceptions when aliases are forbidden in project. #2723

Open
swayok opened this issue Oct 13, 2023 · 2 comments
Assignees
Labels
Pending Awaiting author response

Comments

@swayok
Copy link

swayok commented Oct 13, 2023

Describe the bug
In blade templates you use aliases of facades like {{ Auth::check() }} that cause exceptions like Class "Auth" not found when project discourages usage of aliases.

To Reproduce
Steps to reproduce the behavior:

  1. In config/app.php remove everything from aliases: 'aliases' => []
  2. Open /admin
  3. Get exception Class "Auth" not found

Expected behavior
No exception.

Additional context
In most cases aliases can be replaced by app(class) or fully qualified name of the facade. This way platform will be more independent from the project's configs and restrictions.

@swayok swayok added the Errors label Oct 13, 2023
@swayok swayok changed the title Usage of facade aliases causes exceptions when aliases are forbidden it project. Usage of facade aliases causes exceptions when aliases are forbidden in project. Oct 13, 2023
@tabuna tabuna removed the Errors label Oct 13, 2023
@tabuna
Copy link
Member

tabuna commented Oct 17, 2023

Hi @swayok

Facades, such as Auth, are an important part of the Laravel framework, and it is recommended to include them to ensure a consistent user experience and guarantee functionality.

If you unintentionally removed these facades and are experiencing their absence, it is recommended to check your project's configuration file to ensure it aligns with the framework's recommendations. You can review the configuration file at this link: link to configuration file. It is recommended to keep the facades enabled.

However, if you deliberately removed the facade registration in accordance with your organization's policies or personal preferences, you can dynamically register the facades, for example, within a middleware, to make them work only in your Orchid application.

The current use of facades does not pose any problems for me or for the majority of users.

However, abandoning the use of facades would impose limitations on my role as a maintainer. Are there any significant reasons to refrain from using facades in this package? This will help me better understand the context and make an informed decision.

@tabuna tabuna added the Pending Awaiting author response label Oct 17, 2023
@swayok
Copy link
Author

swayok commented Oct 17, 2023

HI. I did not mean to remove usage of facedes. The problem is usage of aliases of facades that Laravel declares in config/app.php -> aliases. This is literally a magic flavored with magic which is very bad from the design point of view. The less magic you have in project - the more it is maintainable and compatible with other projects. In the case of a library it is very important to use less magic so the library is compatible with more projects. Most people do not remove aliases and use them but it is better to use facades directly, avoiding aliases.

I've forked a platform and managed to run it without facade aliases - all you need to change is replace \Auth:: by auth()-> helper (it was actually designed for use in templates and you can see this in Laravel docs). Laravel won't remove this helper because it is in core, so it is safe to use it instead of app('auth')-> which would be the the most direct option.

Also I've seen a problem with arguments typization. For example: you have Orchid\Access\Impersonation->loginAs(User $user) instead of Orchid\Access\Impersonation->loginAs(Autheticatable $user). The main point here is usage of the most abstract entity (class/interface) so that developer can pass any instance of class that implements interface or extends base class. I don't like mixing users and admins in same table (separation of concerns is very important here) so, I needed to pass Admin model but method does not allow this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending Awaiting author response
Development

No branches or pull requests

2 participants