Nette\Object a řešení problému s method gettery
- David Grudl
- Nette Core | 8215
Problém jsem popisoval
před více než 2 lety. Nette\Object umí pracovat s metodami podobně jako
JavaScript, Ruby nebo Python:
class TestClass extends Nette\Object
{
public function adder($a, $b)
{
return $a + $b;
}
}
$obj = new TestClass;
$method = $obj->adder;
echo $method(2, 3); // 5
Říkejme tomu method gettery. O téhle letité fíčuře se moc neví, protože není jako mnoho jiných zdokumentovaná, nicméně používala se v sandboxu tímto způsobem, takže někteří o ní ví a používají ji.
Pak se ale ukázalo, že má jednu velmi zrádnou vlastnost. Podívejte se na tento kód:
if ($user->isLoggedIn) {
echo 'sensitive data';
}
Problém je, že vypíše sensitive data
vždy, nezávisle na
tom, jestli je uživatel přihlášen. V kódu totiž chybí závorky za
isLoggedIn
, tedy místo volání funkce se volá method getter a
vrátí callback k metodě a ten je vždy truthy.
Což je malér. Co s tím?
Protože mě samotného znervózňuje, že mohu mít takovou nějakou chybu ve svém kódu, chci to řešit už ve verzi 2.3, a ne čekat rok na 3.0. Tudíž četné názory „neřeš to“ neberu.
Zároveň nechci dělat BC break v setinkové verzi.
Chtěl bych dosáhnout toho, aby vše fungovalo beze změn, jen v případě zapomenutých závorek Nette vygenerovalo varování. Ale je hodně těžké to rozlišit, tj. rozeznat, kdy opravdu používám method getter, a kdy jsem jen zapomněl na závorky.
Zároveň budu raději, když Nette vypíše falešné varování, které lze
snadno potlačit zavináčem, nebo lépe přepsat na klasický
callback, tj. $cb = [$user, 'isLoggedIn']
, než aby nevarovalo,
když by mělo.
Jaké jsou možnosti?
- Napadlo mě, že varování by se mohlo vypisovat pro všechny gettery, tj.
funkce začínající předponou
get|is|has
bez povinných parametrů. Je totiž mnohem větší pravděpodobnost, že zápis$user->getIdentity
je případem zapomenutých závorek, než že bych chtěl použít method getter. - Další možností je okruh rozšířit na všechny funkce bez povinných
parametrů, tj. zrušit podmínku na prefix get|is. Jelikož neobvyklejší
způsob užití je zmíněné
$form->onSuccess[] = $this->signInFormSucceeded
a event handlerysignInFormSucceeded()
parametry mívají, nemělo by k falešným poplachům docházet. Ale možná se method gettery používají i v jiným způsobem, nevím. - Zazněl taky nápad generovat varování pokaždé, když method getter
není používán nad
$this
, tj.$this->signInFormSucceeded
projde, ale$obj->signInFormSucceeded
vygeneruje varování. Tohle opět vychází z předpokladu, že se method gettery používají jen způsobem, který byl uvedený v sandboxu.
Variantu A) mám připravenou https://github.com/…tils/pull/87, ale raději bych se přiklonil k širší variantě B) (první commit v #87). Varianta C) mi přijde příliš omezující a generovala by varování pro potenciálně běžné užití method getterů.
Opakuji, že tohle není BC break, změna v nejhorším (což by nastat nemělo) způsobí, že se vygeneruje falešný warning, který snadno potlačíte, a v nejlepším vygeneruje warning, díky kterému objevíte závažnou chybu v kódu.
Pokud používáte method gettery jinak, dejte vědět, ať to můžeme co nejlépe doladit. Zajímá mě zejména to, zda je používáte pro metody, které nemají žádné povinné parametry, a pro metody, které začínají prefixem get, is, případně has.
Aktualizace: Jako jediná průchozí se ukázala varianta A,
pull request jsem mergnul do 2.3@dev a masteru, tak jej prosím
vyzkoušejte.* Můžete taky prohledat svůj kód na regexp
/->(is|get|has)([A-Z]\w*)?[^\w(]/
a zjistit, jestli v něm
zápis, který by mohl generovat varování, nemáte.
- Lukes
- Silver Partner | 68
Nebylo by lepší udělat nějaký nástroj do laděnky (panelu dolu), kde se budou výskyty těchto konstrukcí ukazovat a nebo možnost spustit nějaký test. Potlačovat varovaní v kódu zavináčem mi přijde jako něco co by se mělo používat až jako poslední možnost.
A pokud přikročíš k tomu, že to opravdu bude vyhazovat varovaní, tak by bylo dobré mít možnost tuto kontrolu vypnout.
Editoval Lukes (29. 9. 2015 18:42)
- David Grudl
- Nette Core | 8215
Ano, používání zavináčů je poslední možnost, která předpokládám nebude potřeba.
Doplnění: zavináče jsem neměl zmiňovat, pochopitelně správné je method getter přepsat na klasický PHP callback.
- Pavel Kouřil
- Člen | 128
Osobně method gettery používám pouze pro zpracování formulářů. Používám je převážně tímto způsobem (zjednodušený kód, ale snad to z něj půjde pochopit).
class FooFormFactory
{
public function create() { return new Form(); }
public function addFormSubmitted(Form $form) { }
public function editFormSubmitted(Form $form) { }
}
class FooPresenter
{
public $fooFormFactory;
public function createComponentBarForm()
{
$form = $this->fooFormFactory();
$form->onSuccess[] = $this->fooFormFactory->addFormSubmitted;
}
}
Nicméně osobně si myslím, že by neměl být jakýkoliv BC break. Navíc, vzhledem k tomu, jak je automatické testování presenterů „otravné“ (a hlavně odesílání formulářů, apod.), tak kdyby někdo udělal composer update, spustil testy a myslel si, že je vše OK, byl by velmi nemile překvapen při deploynutí updatlé verze aplikace. :/
Nejpřijatelnější řešení je podle mě opt-in nastavení, které by tyto chyby začalo házet – klidně na všechny method gettery. V sandboxu můžeš hodit toto nastavení na true (nové projekty se této potenciální chybě vyhnou), ale existující projekty by to nijak neovlivnilo – pokud si sami nezvolí.
- David Grudl
- Nette Core | 8215
Žádný BC break nenastane, vše bude fungovat jako doposud. V nejhorší možné variantě se může stát, že se vygeneruje falešné varování. Což je IMHO akceptovatelná daň za odhalení chyby.
opt-in nastavení, které by tyto chyby začalo házet – klidně na všechny method gettery.
Což bychom všichni, kdo method gettery používáme, pochopitelně nechali vypnuté, a všichni ostatní by to taky nechali vypnuté, protože o tom nebudou vědět. To nic neřeší.
- Lukes
- Silver Partner | 68
@DavidGrudl No angularPost používám jako generickou funkci pro zpracování toho požadavku a metody save() a loadData() můžou být prakticky jakékoliv (většinou z modelu). Čili může se stát, že jsou bez parametrů, ale i s nepovinnými ale i povinnými.
Editoval Lukes (29. 9. 2015 21:04)
- David Grudl
- Nette Core | 8215
Díky za reakce, varianty B) a C) jsou tím pádem ze hry a zůstává varianta A).
Tu by to teď chtělo otestovat, takže zkuste prosím na svých projektech použít dev verzi nette/utils:
composer require nette/utils 2.3.*@dev
Věřím, že to žádné falešné varování generovat nebude. Pokud by někoho napadla lepší varianta, sem s ní.
- David Matějka
- Moderator | 6445
@hrach mel by jit alias
"nette/utils": "2.3.*@dev as 2.3.4",
tak to mi nefungovalo, ale funguje
"nette/utils": "2.3.x-dev as 2.3.4",
- Honza Marek
- Člen | 1664
Pak se ale ukázalo, že má jednu velmi zrádnou vlastnost. Podívejte se na tento kód:
if ($user->isLoggedIn) { echo 'sensitive data'; }
Problém je, že vypíše
sensitive data
vždy, nezávisle na tom, jestli je uživatel přihlášen. V kódu totiž chybí závorky zaisLoggedIn
, tedy místo volání funkce se volá method getter a vrátí callback k metodě a ten je vždy truthy.
Ano, přesně takhle to funguje i v javascriptu. Považuju za normální, že když napíšeš něco jiného než chceš, tak se to chová jinak.
- Pavel Kouřil
- Člen | 128
David Grudl napsal(a):
Žádný BC break nenastane, vše bude fungovat jako doposud. V nejhorší možné variantě se může stát, že se vygeneruje falešné varování. Což je IMHO akceptovatelná daň za odhalení chyby.
opt-in nastavení, které by tyto chyby začalo házet – klidně na všechny method gettery.
Což bychom všichni, kdo method gettery používáme, pochopitelně nechali vypnuté, a všichni ostatní by to taky nechali vypnuté, protože o tom nebudou vědět. To nic neřeší.
BC break nenastane? Pokud mi ve verzi 2.3.x aplikace chybu neháže, nicméně ve verzi 2.3.x+1 chybu začne házet, jedná se o BC break.
Opt-in nastavení by mělo výhodu v tom, že by člověk mohl na 2.3.x+1 updatnout (např. kvůli důležitému fixu) bez strachu z toho, že mu aplikace začne házet falešná varování.
To, že by jej nikdo nepoužíval není podle mě tak uplně pravda – v momentě, co by se dalo do sandboxu, by toto nastavení používal každý nový projekt. Osobně bych jej klidně zapnul v momentě, co bych měl čas aplikace projít a zjistit, zda někde tuto chybu vyvolávat nebudou – ale do té doby je pro mě lepší nette neupdatovat, což považuju za velmi nešťastné řešení. :/
- amik
- Člen | 118
Přijde mi škoda tuhle užitečnou featuru odebírat nebo nějak ořezávat… co takhle to řešit opačně, že u security-critical metod by se toto chování explicitně dalo zakázat? něco jako
/** @disableMethodGetter */
public function isLoogedIn() { ... }
Připadá mi, že obecně případů, kdy to ohrozí přímo bezpečnost, je velmi málo (metody dotazující se na ACL vyžadují parametry), resp. mě ani nenapadá jiný příklad, než isLoggedIn…
- David Grudl
- Nette Core | 8215
Překvapuje mě vaše sebejistota, že nikde v šabloně nemáte
<div n:if="$user->isLoggedIn">
nebo cokoliv podobného.
A bohorovnost, s jakou se stavíte k tomu, že by to třeba mohlo pomoci
někomu jinému. Bohužel existují lidé, kteří na rozdíl od
@HonzaMarek nebo @PavelKouřil občas dělají v kódu překlepy,
a patřím mezi ně. Tak to prostě berte jako něco, co vám nemá překážet
a my horší programátoři to oceníme. Jo, jsem ironický :-/
@amik to si tahle máme všichni otagovat veškerý svůj kód? A podle čeho ty metody budeme vybírat? Jak mám rozlišovat ty, kde na návratové hodnotě opravdu záleží, od těch, kde je to víceméně jedno?
- Pavel Kravčík
- Člen | 1193
Takže, když si teď přidám „2.3.5“ místo „2.3“ do composeru, tak si s tím nemusím lámat hlavu?
- David Grudl
- Nette Core | 8215
@castamir zkus prohledat svůj kód na pattern
/->(is|get|has)([A-Z]\w*)?[^\w(]/
a rychle zjistíš, jestli
někde budeš potřebovat psát zavináč klasický PHP callback.
@PavelKravčík byl bych raději, kdybys to vyzkoušel, dokud je to v masteru, a pomohl to doladit.
- Pavel Kouřil
- Člen | 128
Ano, co se týče specifického $user->isLoggedIn
, tam jsem
si téměř jistý tím, že se mě to netýká – neboť používám
$user->loggedIn
(a vůbec, velmi často používám magické
properties).
Stejně tak nemám nic proti tomuto erroru v potenciální 2.4 (pokud by byla) nebo 3.0 – klidně i na všechny method gettery. Jediné, co mi nepřijde vyloženě OK, je prostě ten potenciální BC break a nemožnost pro některé lidi „bezpečně“ updatovat, když se objeví fix něčeho závažného.
PS: Přijde mi zbytečné diskusi zasírat poznámkami jako „horší programátoři“ nebo „X a Y nědělají překlepy“, protože to nikam nevede, a blbě se na to odpovídá, pokud člověk má zaškrknutý ten „iamnice“ checkbox. Radši bych řešil něco konstruktivního – tzn. co podle tebe BC break je a co podle tebe BC break tedy není. :)
PS2: Každopádně během zítřka zkusím hodit na projekty tedy tu testovací verzi a pak napíšu reálný dopad.
Editoval Pavel Kouřil (30. 9. 2015 11:40)
- Jan Endel
- Člen | 1016
Pavel Kouřil napsal(a):
Ano, co se týče specifického
$user->isLoggedIn
, tam jsem si téměř jistý tím, že se mě to netýká – neboť používám$user->loggedIn
(a vůbec, velmi často používám magické properties).
Btw, taky jsem dost používal magic properties, dokud jsem nezjistil, že
jsou klidně i 100× pomalejší, než přímé volání metody, podle mě je
tohle perfektní nápad a jsem za variantu A).
@DavidGrudl keep going!
P.S. díky za regulár, ověřil jsem si, že tím damejidlo netrpí a sakra nechci aby kdy trpělo.
Editoval Jan Endel (30. 9. 2015 11:55)
- David Grudl
- Nette Core | 8215
Pavle, to, že používáš $user->loggedIn
, znamená, že
jsi si 100% jistý, že jsi nikdy ani omylem v nějakém kódu nemohl napsat
$user->isLoggedIn
? Dáš za to ruku do ohně? Odpřísáhneš to
na život svého blízkého?
Pokud ne, tak se tě to týká.
A týká se tě to teď, ne až za rok, kdy bude příští verze.
ad BC break: to, že mi aplikace začne logovat warningy, ale její funkčnost zůstane nedotčena, nepovažuji za BC break. Navíc se tu od začátku snažím, abychom to odladili tak, aby vůbec žádné warningy nevyhazovala. Takže díky za testování, pokud se nějaký warning objeví, budeme to řešit dál.
- Honza Marek
- Člen | 1664
David Grudl napsal(a):
Je totiž mnohem větší pravděpodobnost, že zápis
$user->getIdentity
je případem zapomenutých závorek, než že bych chtěl použít method getter.
Problém je, že validní použití prostě nepoznáš podle názvu. A zavináč používat nechceš nikdy, protože to je vícenásobný mistr světa na ukrývání chyb v kódu.
// tohle je například validní použití method getteru
$identityFactory = $user->getIdentity;
$shouldShowSomethingDecider = $user->isAuthenticated; // blbej název, vim, ale snad jde poznat co jsem myslel
Překlepy samozřejmě můžou nastat kdykoliv. Ale smysl bojovat proti nim to má tehdy, pokud to nepřináší nežádoucí vedlejší účinky. A pokud k těm chybám dochází častěji než k jiným. Máš to Davide podložené jinak než že se ti to jednou stalo?
Můj názor je ten, že pokud v javascriptu, ruby ani pythonu nemají to potřebu řešit, zřejmě se o nějak závažný problém nejedná.
- Honza Marek
- Člen | 1664
Jinak obecně je tohle trochu kontroverzní funkce. Je to hezký, ale
i když se to běžně vyskytuje v jiných jazycích, tak pokud k tomu
přijde člověk, co to nezná, tak ten kód nepřečte. Asi bych spíš použil
[$user, 'getIdentity']
, protože to neni o moc delší a je to
standardní php way.
- David Grudl
- Nette Core | 8215
@HonzaMarek problém je v tom, že třída Object, jejímž prapůvodním a dodnes IMHO primárním účelem je boj proti překlepům (zejména zápis do neexistující property), najednou zvyšuje riziko chyby oproti tomu, když by použita nebyla.
To je rozhodující (a dostačující) důvod s tím něco dělat.
Že neexistuje optimální řešení je snad jasné. Takže se skutečně hodně snažím najít alespoň to nejlepší možné. A divím se, proč namísto spolupráce se mnou bojujete nebo vysvětlujete zjevné.
- Lukes
- Silver Partner | 68
Osobně se mi líbí varianta s tím tagem, ale udělal bych iverzní funkcionalitu než chce @amik. Čili bych vyhazoval varovaní na všechno a pokud to chci potlačit, tak to otaguji. Je mi jasné, že můžete namítat, že můžu udělat chybu při volání již takle otagované funkce, ale už je to explicitní úprava a vím, že si na to musím dát pozor. Nakonec funkcionalita by byla stejná jako u toho zavináče, ale není to taková „prasárna“, kde to může potlačit i nějaké další chyby. Nehledě na to, že ten zavináč použít můžete i tak, ale prosím mě do toho nenuťte :-D
- David Grudl
- Nette Core | 8215
@Lukes a není prostě jednodušší použít
[$user, 'isAuthenticated']
, než dopisovat anotaci, jen abych mohl
zapsat $user->isAuthenticated
?
Jde o to, aby existující kód fungoval bez warningů i bez zásahů. Pokud už bych zasahovat musel, tak bych method getter oželel.
- hrach
- Člen | 1838
Překvapuje mě vaše sebejistota, že nikde v šabloně nemáte <div n:if=„$user->isLoggedIn“> nebo cokoliv podobného.
Jsem si vcelku hodne jistej, protoze: mame IActor, kterej implementuje Guest a User. Bud to neprojde typehintem, nebo to velmi brzo skonci na tom, ze guest nema id. Tedy nebezpeci, ze se navstevnik dostane k necemu, k cemu nema, je naprosto minimalni.
- Jan Tvrdík
- Nette guru | 2595
@Lukes Jedna z podstatných výhod
[$user, 'isAuthenticated']
je, že ti pak funguje statická
analýza v PhpStormu (jako přejmenování, find usages…).
- Honza Marek
- Člen | 1664
<possible-offtopic>U Nette\Objectu by bylo fajn, kdyby se rozdělil na Object chránící proti překlepům a Object+, kterej by přinášel všechny nadstavbové vychytávky.</possible-offtopic>
- Jan Tvrdík
- Nette guru | 2595
Ještě k tomuhle – vzhledem k tomu ta funkcionalista je více než
3 roky stará, 2 a půl roku se o problému ví a nikdo si aktivně až do
teď nestěžoval, tak na to reálně lidi asi moc nenarážejí. Ve 2.3 bych
manuálně zakázal, aby $user->isLoggedIn
vracelo callback a ve
3.0 bych celou method-getter funkcionalitu zrušil (v rámci komplexnější
úpravy Object
třídy, ke které asi dojde).
- David Grudl
- Nette Core | 8215
@JanEndel něco jiného je klasický getter
$user->loggedIn
, který vrací totéž co volání metody
$user->isLoggedIn()
, a method getter
$user->isLoggedIn
, který vrací callback, obdobu
[$user, 'isLoggedIn']
.
- David Grudl
- Nette Core | 8215
Nikdo si aktivně nestěžoval…
Narazil jsem na to sám, tuhle mi o tom říkal třeba
@FilipProcházka, příklady jsou i na fóru, například
if (!$this->getUser()->isLoggedIn)
https://forum.nette.org/…eslani-formu#….
stačí zakázat isLoggedIn
A co třeba if ($this->isAjax)
https://forum.nette.org/…zpet-a-vpred#…,
byť tehdy šlo o překlep? Na některé věci, jako například
$control->getValue
https://forum.nette.org/…tni-validace#…,
se přijde rychle, ačkoliv chybová hláška musí být hodně matoucí
zejména pro začátečníjky, ale co je zrada i pro ostřílené borce, je
třeba foreach ($formular->getErrors as $input)
https://forum.nette.org/…zit-validaci#…,
protože iterování nad closure je korektní operace a důsledkem pak je, že
se nevypisují chyby.
- Eda
- Backer | 220
K původnímu dotazu, jak kdo tuto fíčuru používá:
- Dnes ji používám jen na „process“ metody k formulářům.
- Dřív jsem ji používal i pro předávání callbacků, které loadovaly
data, do cachovaných latte šablon. Tzn. komponenta měla bezparametrickou
metodu např.
getOperators
a tu jsem pak předal do šablony:
$template->operatorsLoader = $this->getOperators;
A v šabloně jsem pak volal:
{foreach $operatorsLoader() as $operator}
...
{/foreach}
Smysl to mělo takový, že při načtení výstupu z cache se pak již drahá funkce s loadem dat nemusela vůbec vykonat. V tomto případě by se mne navrhované řešení týkalo a nově by mi to vyhazovalo warning. Nicméně, jak už jsem psal, už to takto nepoužívám.
Jinak: Je to pěkná fíčurka a byl bych nerad, kdyby z Nette zmizela úplně. Používám to odjakživa a čte se mi to o dost líp, než nějaké pole.
- Pavel Kouřil
- Člen | 128
Jinak, zkusil jsem tedy tu 2.3.*@dev
na 2 projektech a na
žádné problémy v mých use-casech jsem nenarazil.
EDIT: Možná jsem měl prve ověřit, že se mi opravdu ta @dev stáhla … Zkusím to znovu zítra, snad již na správné verzi.
Editoval Pavel Kouřil (2. 10. 2015 21:50)