Refactoring Presenter::checkRequirements na dve metody

proč jsi to rovnou neposlal jako pull?

Pull request

HosipLan napsal(a):

Otočil bych podmínku

Ta podminka je tak udelana schvalne, aby ti checkRequirements mohlo vratit napr popis chyby (!== TRUE), ktery nasledne v ensureRequirements nacpes do vyjimky.

Jeste poznamka k navratovemu typu checkRequirements v pripade neuspechu. Mame prakticky dve moznosti:

1/ Ruznorode typy (@return bool|mixed, jak je v pullu)

false jako jednoduche hlaseni problemu, pokud staci

string e.g. Required role admin

array pro detailnejsi popis …

To pak zavisi na prislusne aplikaci, jak moc chce mit clovek popsane chyby.

2/ Pouze bool (@return bool):
Dovolime jen true/false, pak se ale tezko budou resit chybove hlasky (viz vyse)

Ono vytvoreni vyjimky neni AFAIK tak levna operace, protoze se musi zkopirovat cely stacktrace. Navic, jak uz nazev sam vypovida, je to vyjimka a tedy pro vyjimecne situace.

Problem to muze zpusobit pokud si napr budu napr testovat hodne metod, pak uz muze hodne vyjimek zabrat opravdu hodne casu.

Ve vysledku bych si udelal stejne checkRequirements refactoroval do jednoho authorizatoru spolecneho pro celou aplikaci, a checkRequirements by delegovalo volani na nej.
Pro jednodche pripady bych se tedy ptal primo presenteru (sama sebe); v pripade, ze bych se chtel ptat hodne, tak uz onoho autorizatoru.

Upravil jsem pull request, aby checkAuthorization vracelo pouze bool, je to tedy daleko cistejsi. Presmerovani na Sign:in bych teda ted delal v ensureRequirements. Je to „spravne“?

Jen pro přehled: ForbiddenRequestException tedy stojí defacto na pozici neexistující AuthorizationException. Je to tak?

David Grudl napsal(a):

Tohle je přesně důvod, proč jsem to do verze 2.0 neřešil. Neexistuje nic jako $application->signInPresenter;

Presmerovani na Sign:in bych urcite nedelal do Nette, ale bylo by v mojem BasePresenter::ensureRequrements. Muj BasePresenter by pak vypadal asi takto.

public function checkRequirements($element) {
	if (...) return false; // moje kontroly (actionX, signalX, renderX, ...)
	// napr:
	if (!$this->authorizator->isMethodAllowed($this->user, $this, $this->formatActionMethod(...)) return false;

	return true;
}

public function ensureRequirements($element) {
	$ret = $this->checkRequirements($element);

	if(!$ret) {
		if($this->user->loggedIn) throw new ForbiddenRequestException; // already loggen in, but not allowed
		else $this->redirect($this->loginLink); // not logged in -> go to login form
	}
}

Tam, kde se ocekava, ze uzivatel ma pristup (napr tryCall v Presenter::run), se vola ensureRequirements, ktere zajisti, aby byly requirementy splneny. To bud jde tak, ze se uzivatel prihlasi, nebo nejde a vyhodi se vyjimka a request skonci 403 chybou.

Kdyz chci zkontrolovat, zda nekam ma opravneni (napr kontroluju zda zobrazit nejaky odkaz), tak zavolam checkRequirements a ocekavam true/false a nebudu chytat vyjimky. Protoze jde o kontrolu a ta muze bezne rici „ne“, coz neni vyjimecny stav.

Editoval juzna.cz (27. 3. 2012 1:12)

@Vrtak to uz jsi ale odbehl od puvodniho tematu – moznost si overit splneni pozadavku (checkRequirements) bez side-efektu.

Proc vubec v presenteru resit odchytavani vyjimek a tim ho jeste vic komplikovat (kdyz uz ted si porad rikame, ze toho dela strasne moc)?