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

Create interface for User #20

Open
foxycode opened this issue Jan 18, 2017 · 17 comments
Open

Create interface for User #20

foxycode opened this issue Jan 18, 2017 · 17 comments

Comments

@foxycode
Copy link

  • bug report? no
  • feature request? yes
  • version: 3.0

Few times I wanted to use own User implementation, bt there is no simple way how to force Nette to use it. I'd like to have IUser same way as IUserStorage to be able to use my own implementation in framework.

@mrtnzlml
Copy link
Contributor

User is registered in DIC as a service with the name therefore it should be possible to replace it with your own implementation similarly like in this thread.

@foxycode
Copy link
Author

Problem is, there is User class used directly in Nette code. If not in code, definitelly in comments which isn't good for IDE.

@JanTvrdik
Copy link
Contributor

My tip: don't use the User class at all, operate directly on IUserStorage. It has almost the same API.

@foxycode
Copy link
Author

@JanTvrdik Thanks for the tip, but this doesn't solve issue in framework. User is somehow hardcoded and it would be great if it wasn't. I'm happy to make PR (or at least try it), but I want to know others opinions first.

@enumag
Copy link
Contributor

enumag commented Jan 18, 2017

@foxycode Actually the User class is problematic in quite a few use-cases so in advanced systems it's better to not use it at all. Rather than a PR it's better to just make your own library like I did.

@foxycode
Copy link
Author

@enumag Again, why not solve it directly in framework? Yes, I can make my own library. I actually already did.

I believe, if we put our heads together, we can solve it directly in framework. I train new programmers in Nette and it's confusing for them. I can't simply tell them to write own library :)

@enumag
Copy link
Contributor

enumag commented Jan 18, 2017

@foxycode It would become a new package separate from nette/security anyway (because of BC). In which case it doesn't matter if it is in nette namespace or not. Can't speak for @dg but I don't think he wants to add something completely new into nette.

@foxycode
Copy link
Author

@enumag I think we can afford BC breaks in 3.0 and this is only about adding interface. Possibly this wouldn't need BC at all. I'd like to know what @dg thinks too. Can you tell me which problems do you see with User?

@JanTvrdik
Copy link
Contributor

this is only about adding interface

Yes, but it is an interface of a class with bad name and bad design. The goal should be to remove (or somehow fix?) the class instead of making even more stuff dependent on in.

@foxycode
Copy link
Author

@JanTvrdik Ok, I agree with it :) I'd like to try, but need to know what others don't like about User class. Can you tell your opinion?

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Jan 18, 2017

Few random things that are bad about the User class.

  1. The name – User is usually an entity in application. The responsibility of this class is not to represent a user but to serve as some kind of manager of active user session.
  2. It has way to many responsibilities (that's why it cannot have a good name). It is better to use directly IAuthenticator, IUserStorage and some alternative of IAuthorizator.
  3. It relies on bad interface IAuthorizator (see this RFC on forum)

@dakujem
Copy link
Contributor

dakujem commented Mar 20, 2020

God, I just wanted to post the same as @foxycode.

I am upgrading a larger codebase from nette v 2.4 and it's just not possible, since we are using an extended version of User class, bud suddenly some methods are final 😖. What is even worse, Nette\Application\UI\Presenter class won't accept any other than Nette\Security\User object and the getUser method is (yet again) final.

So... how do we extend the functionality of User object?? Please, this is a framework. Why do we constantly need to hit private and final stuff? We want to extend and bend a framework. This is where Nette gets in way. I do not wish to copypaste 1400 lines of Presenter code just to be able to use my own User implementation.

In particular, in my case I have some custom logic in User::getIdentity method, why it is final, I have no idea.

@sommcz
Copy link

sommcz commented Oct 26, 2020

Hi, any updates on this topic? It's quite old, but still it is a issue when upgrading large codebase from v2

@dakur
Copy link
Contributor

dakur commented May 5, 2022

I've just came accross this. I want NOT to use Nette\Security\User, but I can't because if it's baked into Presenter, UserPanel etc.

While I agree that User class has bad design, the thing is, that it's still in Nette and it will be probably for some time. So I think one of these solutions should be applied until the whole class is rethought:

  • make Nette\Application\UI\Presenter#getUser() not final – that's the easiest one. You can not replace User, but at least you can ensure that no one is going to use it in presenters. But e.g. User panel in Tracy still gets information from User class.
  • make Nette\Security\User an interface – User keeps working, but making it an interface, there is place for custom implementations. I think it could be actually done without BC breaks.

@dg Could you please write some thoughts to this old issue? At least for me, a big blocker NOT to make PRs is the fact, that I'll never know if the whole stuff wouldn't be rejected. And I don't want to/can't waste my time. If you say "well, yes, let's do it this way, would someone do it?", I believe more people would contribute.

Edit: just realized that non final getUser() solves it for presenters only, which is not enough in my opinion. So I would add myself to the call for interface. 🙂

@dakujem
Copy link
Contributor

dakujem commented May 5, 2022

In case the interface is too much to ask or simply not desired, I suggest also removing the return type hint, so that the overriding methods can return anything, not descendants of the User object only.

// Presenter.php

    /**
     * @return Nette\Security\User
     */
    public function getUser() //: Nette\Security\User
    {
        if (!$this->user) {
            throw new Nette\InvalidStateException('Service User has not been set.');
        }

        return $this->user;
    }

This is a backwards-compatible <10 minute fix.

@jkuchar
Copy link

jkuchar commented May 6, 2022

Simple, stop using it at all. Only hard which is to how to remove it from DI, so nobody can wire it. I have approached it using this fake factory overriding original registration from nette/security di-extension.

services:
	security.user:
		factory: CampApp\App\Infrastructure\NoNetteUserFactory::throwErrorWhenAccessed()
		autowired: false
final class NoNetteUserFactory
{
	public static function throwErrorWhenAccessed(): User
	{
		throw new UsageException('Hey! Nette\User is disabled in this project (as interface does not match use-cases needed in this project). Use <YourClass> instead.');
	}
}

@dakujem
Copy link
Contributor

dakujem commented Jun 15, 2022

Wouldn't it then be better to completely remove it from Presenter, extension etc.? It gets in the way.
Why not just enable overriding, which is simpler and backwards-compatible?

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

No branches or pull requests

8 participants