Nette\Object a řešení problému s method gettery

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

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?

  1. 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.
  2. 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 handlery signInFormSucceeded() 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.
  3. 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.

před 4 lety

Lukes
Člen | 58
+
0
-

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)

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

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.

před 4 lety

Pavel Kouřil
Člen | 128
+
0
-

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í.

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

Žá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ší.

před 4 lety

Lukes
Člen | 58
+
0
-

Osobně je používám na předávání callbacků metodám jako parametr metody.

<?php

    $this->angularPost($this->save, [$paramSave], $this->loadData, [$paramLoad]);

?>

Abych furt nemusel opisovat zpracování AngularJS požadavku.

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

@Lukes ty metody save() a loadData(), mají nějaké povinné parametry?

před 4 lety

Lukes
Člen | 58
+
0
-

@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)

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

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í.

před 4 lety

hrach
Člen | 1815
+
0
-

Rad bych to vyzkousel na projektu. Mam aktualne

"nette/nette":              "~2.3.4",
"nette/utils":              "2.3.*@dev",

nejaky tip, jak to upravit, aby to includlo dev utils?

před 4 lety

David Matějka
Moderator | 5875
+
+1
-

@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",

před 4 lety

Honza Marek
Člen | 1674
+
+9
-

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.

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.

před 4 lety

Pavel Kouřil
Člen | 128
+
+6
-

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í. :/

před 4 lety

amik
Člen | 124
+
+3
-

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…

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

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?

před 4 lety

castamir
Člen | 631
+
0
-

@DavidGrudl Jsem pro A), ale osobně to považuju za BC break, protože budu nejspíš muset na některé místa dopsat ty zavináče, abych s aplikací mohl pracovat i na lokálu. Je to pro mne úplně stejně otravná změna (byť asi nutná) jako pro tebe ty anotace od @amik.

před 4 lety

Pavel Kravčík
Člen | 969
+
0
-

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?

před 4 lety

David Grudl
Nette Core | 6849
+
+2
-

@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.

před 4 lety

Pavel Kouřil
Člen | 128
+
+1
-

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)

před 4 lety

Jan Endel
Člen | 1022
+
+3
-

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)

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

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.

před 4 lety

Honza Marek
Člen | 1674
+
+2
-

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á.

před 4 lety

Honza Marek
Člen | 1674
+
0
-

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.

před 4 lety

David Grudl
Nette Core | 6849
+
+1
-

@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é.

před 4 lety

Lukes
Člen | 58
+
0
-

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

před 4 lety

David Grudl
Nette Core | 6849
+
+2
-

@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.

před 4 lety

Lukes
Člen | 58
+
0
-

@DavidGrudl No tak jako i to je možnost samozřejmě, ale v kodu se mi to líbí méně, ale to už je asi můj problém.

před 4 lety

hrach
Člen | 1815
+
-2
-

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.

před 4 lety

Jan Tvrdík
Nette guru | 2553
+
+4
-

@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…).

před 4 lety

Honza Marek
Člen | 1674
+
0
-

<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>

před 4 lety

Jan Tvrdík
Nette guru | 2553
+
+22
-

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).

před 4 lety

Jan Endel
Člen | 1022
+
0
-

Jan Tvrdík napsal(a):

Ve 2.3 bych manuálně zakázal, aby $user->isLoggedIn vracelo callback

a neuděláš tím větší BC break než je teďka? Co chudáci aplikace co na $user->isLoggedIn callback spoléhají? Není takových potenciálně nebezpečných metod mnohem víc?

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

@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'].

před 4 lety

David Grudl
Nette Core | 6849
+
0
-

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.

před 4 lety

Eda
Člen | 212
+
+2
-

K původnímu dotazu, jak kdo tuto fíčuru používá:

  1. Dnes ji používám jen na „process“ metody k formulářům.
  2. 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.

před 4 lety

Pavel Kouřil
Člen | 128
+
0
-

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)

před 4 lety

ali
Člen | 304
+
0
-

@DavidGrudl po nahozeni nette/utils -dev ani jeden warning po 5 dnech provozu..

Muj zpusob uziti callbacku u formularu:

$form->onSuccess[] = [$this, "formSuccess"];