Politely extensible Nette\Http\User #298

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
@fprochazka
Contributor

fprochazka commented Jun 20, 2011

Make Nette\Http\User easily extensible. Nowadays the whole class has to be duplicated, just to be able to change one method.

@hrach

This comment has been minimized.

Show comment
Hide comment
@hrach

hrach Jun 20, 2011

Contributor

Jj, přijde mi to jako lepší řešení než nějaké megalomanské věci :)

Contributor

hrach commented Jun 20, 2011

Jj, přijde mi to jako lepší řešení než nějaké megalomanské věci :)

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Jun 20, 2011

Contributor

UserStorage může být další krok, ale rád bych zamezil dalším takovýmhle: https://twitter.com/#!/HosipLan/status/82498296697978880

Contributor

fprochazka commented Jun 20, 2011

UserStorage může být další krok, ale rád bych zamezil dalším takovýmhle: https://twitter.com/#!/HosipLan/status/82498296697978880

@dg

This comment has been minimized.

Show comment
Hide comment
@dg

dg Jun 21, 2011

Member

A proč jsi potřeboval tu metodu přepisovat?

Member

dg commented Jun 21, 2011

A proč jsi potřeboval tu metodu přepisovat?

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Jun 22, 2011

Contributor

Protože potřebuji okamžitě po načtení ze session dostat uživatele zpátky do správy EntityManageru (Doctrine). Což můžu udělat tak, že ho do něj mergnu, nebo implementuji identitě interface Serializable, do session budu ukládat pouze ID a uživatele vždy načtu z databáze. V současnosti to dělám takto: https://gist.github.com/1039260

Contributor

fprochazka commented Jun 22, 2011

Protože potřebuji okamžitě po načtení ze session dostat uživatele zpátky do správy EntityManageru (Doctrine). Což můžu udělat tak, že ho do něj mergnu, nebo implementuji identitě interface Serializable, do session budu ukládat pouze ID a uživatele vždy načtu z databáze. V současnosti to dělám takto: https://gist.github.com/1039260

@Vrtak-CZ

This comment has been minimized.

Show comment
Hide comment
@Vrtak-CZ

Vrtak-CZ Jun 22, 2011

Contributor

Mám stejný problém s tím rozdílem že načítám celou identitu z DB.

Contributor

Vrtak-CZ commented Jun 22, 2011

Mám stejný problém s tím rozdílem že načítám celou identitu z DB.

@honzajavorek

This comment has been minimized.

Show comment
Hide comment
@honzajavorek

honzajavorek Jun 24, 2011

Contributor

$context by IMHO neměl být private. Potřeboval jsem teď jen rozšířit User o nějakou další funkčnost navázanou na databázi a na facebook API a přesně $context jsem tam potřeboval. Musel jsem si ho tam zbytečně znova přidat jako protected.

Contributor

honzajavorek commented Jun 24, 2011

$context by IMHO neměl být private. Potřeboval jsem teď jen rozšířit User o nějakou další funkčnost navázanou na databázi a na facebook API a přesně $context jsem tam potřeboval. Musel jsem si ho tam zbytečně znova přidat jako protected.

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Jun 24, 2011

Contributor

Což se dá obejít takto

private $context;
public function __construct(Nette\DI\Container $context)
{
    parent::__construct($context);

    $this->context = $context;
}

ale příjde mi jednodušší mít tam protected getter

Contributor

fprochazka commented Jun 24, 2011

Což se dá obejít takto

private $context;
public function __construct(Nette\DI\Container $context)
{
    parent::__construct($context);

    $this->context = $context;
}

ale příjde mi jednodušší mít tam protected getter

@honzajavorek

This comment has been minimized.

Show comment
Hide comment
@honzajavorek

honzajavorek Jun 24, 2011

Contributor

@hosiplan Ano, přesně takto jsem to udělal, ale nevidím jediný důvod, proč by to nemohlo být prostě protected.

Contributor

honzajavorek commented Jun 24, 2011

@hosiplan Ano, přesně takto jsem to udělal, ale nevidím jediný důvod, proč by to nemohlo být prostě protected.

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Jun 24, 2011

Contributor

Protože to není čisté, "správně" by mělo být všechno private a mít gettery, popřípadě settery. Taky s tím David má občas nějaké konkrétní plány a ponechání většiny vlastností jako private mu částečně zaručuje, že na tom nebudou lidé stavět nějaké hacky a bude moct kdykoliv v budoucnu změnit implementaci.

Contributor

fprochazka commented Jun 24, 2011

Protože to není čisté, "správně" by mělo být všechno private a mít gettery, popřípadě settery. Taky s tím David má občas nějaké konkrétní plány a ponechání většiny vlastností jako private mu částečně zaručuje, že na tom nebudou lidé stavět nějaké hacky a bude moct kdykoliv v budoucnu změnit implementaci.

@honzajavorek

This comment has been minimized.

Show comment
Hide comment
@honzajavorek

honzajavorek Jun 24, 2011

Contributor

K té první části se vyjadřovat nebudu, o tom co je "správné" se dá diskutovat dlouho. Že s tím má David plány chápu, ale já s tím mám plány taky :) . Protected getter by byl dobré řešení.

Contributor

honzajavorek commented Jun 24, 2011

K té první části se vyjadřovat nebudu, o tom co je "správné" se dá diskutovat dlouho. Že s tím má David plány chápu, ale já s tím mám plány taky :) . Protected getter by byl dobré řešení.

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Jun 24, 2011

Contributor

Proto tam taky byly ty uvozovky, že :)

Contributor

fprochazka commented Jun 24, 2011

Proto tam taky byly ty uvozovky, že :)

@PetrP

This comment has been minimized.

Show comment
Hide comment
@PetrP

PetrP Sep 19, 2011

Contributor

Nějaká šance že se tohle hne?

Contributor

PetrP commented Sep 19, 2011

Nějaká šance že se tohle hne?

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Sep 19, 2011

Contributor

Jestli se David objeví na Webexpu tak si ho podáme :)

Contributor

fprochazka commented Sep 19, 2011

Jestli se David objeví na Webexpu tak si ho podáme :)

@PetrP

This comment has been minimized.

Show comment
Hide comment
@PetrP

PetrP Sep 19, 2011

Contributor

Jinak ted jsem si to implementoval takto bez zasahu do nette :-)

class MyUser extends Nette\Http\User
{
    private $context;

    public function __construct(Nette\DI\Container $context)
    {
            parent::__construct($context);
            $this->context = $context;
    }

    protected function getSessionSection($need)
    {
        $session = parent::getSessionSection($need);
        if ($session AND isset($session->identity) AND !$session->identity->isAttached())
        {
            $session->identity->attach($this->context->model->users);
        }
        return $session;
    }
}

Identitu řeším trošku jinak než jí máš ty ale šlo by takto implementovat i to tvoje.

Contributor

PetrP commented Sep 19, 2011

Jinak ted jsem si to implementoval takto bez zasahu do nette :-)

class MyUser extends Nette\Http\User
{
    private $context;

    public function __construct(Nette\DI\Container $context)
    {
            parent::__construct($context);
            $this->context = $context;
    }

    protected function getSessionSection($need)
    {
        $session = parent::getSessionSection($need);
        if ($session AND isset($session->identity) AND !$session->identity->isAttached())
        {
            $session->identity->attach($this->context->model->users);
        }
        return $session;
    }
}

Identitu řeším trošku jinak než jí máš ty ale šlo by takto implementovat i to tvoje.

@Vrtak-CZ

This comment has been minimized.

Show comment
Hide comment
@Vrtak-CZ

Vrtak-CZ Oct 7, 2011

Contributor

Any news?

Contributor

Vrtak-CZ commented Oct 7, 2011

Any news?

@juzna

This comment has been minimized.

Show comment
Hide comment
@juzna

juzna Dec 4, 2011

Contributor

Ding ding, this is a reminder for this pull request to be merged.

Contributor

juzna commented Dec 4, 2011

Ding ding, this is a reminder for this pull request to be merged.

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Dec 5, 2011

Contributor

Well now I understand why @dg don't want to merge this. He knows the whole user-authentication-identity part of Nette is not that great and he doesn't want us to build our apps on something, he will eventualy need to rewrite.

And you know what is the best part? I've changed my point of view, because it took so long for this to just hang here, and now I have no idea why I've needed the entity to be loaded from repository every single time.

Contributor

fprochazka commented Dec 5, 2011

Well now I understand why @dg don't want to merge this. He knows the whole user-authentication-identity part of Nette is not that great and he doesn't want us to build our apps on something, he will eventualy need to rewrite.

And you know what is the best part? I've changed my point of view, because it took so long for this to just hang here, and now I have no idea why I've needed the entity to be loaded from repository every single time.

@hujer

This comment has been minimized.

Show comment
Hide comment
@hujer

hujer Dec 5, 2011

Zajimava schizofrenie cz vs. en :) Ale tohle me zrovna stve taky. Nacitam uzivatele z databaze pokazde znova, z jednoducheho duvodu: Kdyz mu dam ban, projevi se pri dalsim requestu a nemusim resit ruseni jeho session.

hujer commented Dec 5, 2011

Zajimava schizofrenie cz vs. en :) Ale tohle me zrovna stve taky. Nacitam uzivatele z databaze pokazde znova, z jednoducheho duvodu: Kdyz mu dam ban, projevi se pri dalsim requestu a nemusim resit ruseni jeho session.

@juzna

This comment has been minimized.

Show comment
Hide comment
@juzna

juzna Dec 5, 2011

Contributor

@hosiplan, how did you solve this problem eventually?

Contributor

juzna commented Dec 5, 2011

@hosiplan, how did you solve this problem eventually?

@fprochazka

This comment has been minimized.

Show comment
Hide comment
@fprochazka

fprochazka Dec 5, 2011

Contributor

@hujer: hlavně že kód mám anglicky! :))

@juzna I didn't

Contributor

fprochazka commented Dec 5, 2011

@hujer: hlavně že kód mám anglicky! :))

@juzna I didn't

@PetrP

This comment has been minimized.

Show comment
Hide comment
@PetrP

PetrP Dec 8, 2011

Contributor

@hosiplan ok tak pár let počkáme, ne?

Contributor

PetrP commented Dec 8, 2011

@hosiplan ok tak pár let počkáme, ne?

@dg

This comment has been minimized.

Show comment
Hide comment
@dg

dg Dec 13, 2011

Member
  1. context is User internal implementation. Use DI for other services.

  2. overwrite getSessionSection().

Member

dg commented Dec 13, 2011

  1. context is User internal implementation. Use DI for other services.

  2. overwrite getSessionSection().

@dg dg closed this Dec 13, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment