Nette\Database: isset($row->column) vrací TRUE, když je hodnota NULL

spidy

Z nadpisu je asi problém jasný. Když mám v databázi nějakou hodnotu NULL a volám na ní isset(), vrátí mi TRUE. V PHP má ale funkce isset vracet FALSE, pokud proměnná není definovaná, nebo je její hodnota NULL. Je tedy nějaký zvláštní důvod, proč se v Nette\Database\Table\ActiveRow::__isset() testuje jen existence sloupečku?

Díky za odpověď

Aurielle

Někdy je toto chování žádoucí a naopak chování standardního isset() žádoucí není. Když nad něčím volám isset, tak chci jen vědět, zda daný klíč existuje. Mně osobně toto chování přijde vhodnější.

Ale s Nette\Database nepracuji a nevím, zda je to správně nebo špatně.

Elijen

Také jsem na to narazil. Na jednu stranu je to chování správné, protože sloupeček skutečně existuje, pouze je nastaven na NULL. Na druhou stranu je to odchylka od standardního chování isset v PHP, která může vést k chybám v kódu.

Osobně místo isset používám $col !== NULL případně !empty($col)

spidy

Je mi jasné, že někdy se hodí víc takováto funkčnost, ale přijde mi matoucí, když potom ta funkce dělá něco jiného, než je napsáno v php dokumentaci… Navíc zrovna v tomto případě většinou vím, které sloupečky existují :), takže nevidím důvod, proč by to tak mělo být.

Elijen

spidy napsal(a):

Je mi jasné, že někdy se hodí víc takováto funkčnost, ale přijde mi matoucí, když potom ta funkce dělá něco jiného, než je napsáno v php dokumentaci…

Shouhlasím.

Navíc zrovna v tomto případě většinou vím, které sloupečky existují :), takže nevidím důvod, proč by to tak mělo být.

Jde spíš o to, když uděláš překlep v názvu sloupce. Funkce isset by takový překlep „zkryla“ kdežto $row["tady_je_preklep"] !== NULL vrati FALSE pokud sloupec obsahuje NULL a vyhodi warning, pokud sloupec neexistuje.

spidy

Elijen napsal(a):

Jde spíš o to, když uděláš překlep v názvu sloupce. Funkce isset by takový překlep „zkryla“ kdežto $row["tady_je_preklep"] !== NULL vrati FALSE pokud sloupec obsahuje NULL a vyhodi warning, pokud sloupec neexistuje.

Nejsem si tím úplně jistý, ale nevyhodí ActiveRow (dědí od Nette\Object) výjimku při čtení nedeklarovaného atributu (tj. i neexistujícího sloupce)?

Elijen

spidy napsal(a):

Nejsem si tím úplně jistý, ale nevyhodí ActiveRow (dědí od Nette\Object) výjimku při čtení nedeklarovaného atributu (tj. i neexistujícího sloupce)?

Koukám, že se to nějak měnilo. V mé verzi (2.0-dev ze 2011–08–24) je v metode __get trigger_error("Unknown column $key", E_USER_WARNING); a v API dokumentaci, kterou linkujes, to neni.

Editoval Elijen (27. 12. 2011 17:02)

hrach

https://github.com/…tte/pull/457 ;)

_Martin_

Tenhle kód bohužel nefunguje s DiscoveredReflection. Pokud testovaná property neexistuje ve sloupečcích, zkusí se hledat cizí klíče v databázi pomocí reflexe. A ta, pokud se žádná nenajde, vyhodí výjimku.

Přemýšlel jsem o fixu, ale Nette vyhazuje PDOException, což prakticky znemožňuje její správné odchycení. Jaké by bylo čisté řešení, vlastní výjimka? A nemělo by být chování obou reflexí sjednoceno?

hrach

Tenhle kod naopak pracuje s discovered reflection (viz. testy). Ty totiz pomoci isset preci nebudes testovat, zda existuje dany sloupec, ale zda existuje dana hodnata sloupce.

Opravdu nevim v jakych sci-fi situacich to chces vyuzivat.

_Martin_

No to budu, takhle přeci funkce isset na normálních objektech funguje:

class Trida
{
	public $dostupnaAJeNull;
	public $dostupnaANeniNull = 'text';
	private $nedostupnaAJeNull;
	private $nedostupnaANeniNull = 'text';
	// $neexistujici
}

$class = new Trida();
NDebugger::dump(isset($class->dostupnaAJeNull)); // FALSE
NDebugger::dump(isset($class->dostupnaANeniNull)); // TRUE
NDebugger::dump(isset($class->nedostupnaJeNull)); // FALSE
NDebugger::dump(isset($class->nedostupnaNeniNull)); // FALSE
NDebugger::dump(isset($class->neexistujici)); // FALSE

Je matoucí, pokud se najednou u jednoho druhu objektů chová jinak. Navíc se liší chování i podle použité reflexe.

O sci-fi situaci nejde, zpracující funkce pracuje s více druhy objektů (tzn. různé tabulky) s podobným rozhraním, jenže podle třídy nelze rozlišovat, když je vždy ActiveRow. Tak rozlišuje podle existence sloupečku.

Melmen

Doporučuju přečíst článek od Jakuba Vrány, popisuje tam přesně tvůj „problém“.

_Martin_

Krásný článek, ale nemůžu s ním souhlasit bez výhrad. Konstrukt isset má svoje chování v dokumentaci jasně vysvětlené: „Determine if a variable is set and is not NULL.“. Z toho důvodu preferuji, aby se při implementaci magických metod zohlednilo standardní chování.

hrach

Souhlasim ze aktualni stav neni dobry. Jako jedine rozumne reseni mi pripada pridat metodu hasColumn, ktera plni to, co ty pozadujes. Duvody:

přístup $row->key naznamená $row->column z principu ActiveRow, v tuto chvíli je nutné, aby metoda __isset dělala to stejné, tedy se snažila také načíst závislou tabulku.
jako první triviální řešení by se zdálo to, že v rámci issetu bude blog try-catch, který odchytne vyjimku při nenalezení vazebního sloupce pro daný key a v catchi to vrátí false
dříve uvedené řešení ale nararáží na to, že při takovémto volání issetu se právě jednou při daném http requestu provede dotaz do db pro oveření validnosti cache klíčů. toto považuji za špatné řešení;
možným řešením předchozí odrážky by byla změna cachování, ale to mi přijde nejvýš v důsledku pro programátora nešťastné.
samotnou příčinou mi totiž připadá neuvědomění si konceptu zmíněného v odrážce, že isset($row->user_id) neznámená otazku na existenci sloupce user_id, ale otázku na „zjištění sloupce, nebo vazby na cizí tabulku“.

co ty na to?

spidy

Co takhle netestovat, jestli sloupec existuje, ale jestli byl selectnutý? Např:

$user_id = $db->table('users')->select('user_id')->fetch();
$vsechno = $db->table('users')->select('*')->fetch();
$id = $db->table('users')->select('id')->fetch();
dump(isset($user_id->user_id)); // TRUE, pokud user_id není NULL
dump(isset($vsechno->user_id)); // TRUE, pokud user_id není NULL
dump(isset($id->user_id)); // FALSE

S metodou hasColumn souhlasím.

hrach

@spidy: jednim slovem: blbost ;) nedokazi si predstavit realny usecasy, kdy by to mohlo rozumne fungovat.

paranoiq

@spidy: součástí chování ActiveRow je i to, že dokáže vydat i data, které nemá selectnutá od začátku. to co navrhuješ by bylo matoucí

myslím, že u isset() by se měl sloupec načíst (pokud není) a ověřit hodnota. tak aby bylo chování konzistentní s isset() nad polem

hrach

Hluboce jsem se nad daným tématem zamyslel a nakonec jsem změnil názor. Tu jsou moje argumenty:

nelíbí se mi dříve mnou navrhované zavedení metody hasColumn, je naprosto neintuitivní, nové api, máme tu isset()… tak proč?
načítání referenční „entity“ v issetu mi přišlo hloupé. Protože pokud jsme issetovali neexistující sloupec, znovu se kvuli validaci cizich klicu provedl na ne dotaz; nova data nenasel, a stejne to skoncilo exceptinou, ze dana vazba pro dany sloupec neexistuje.
vyse zminena odrazka by sla vyresit try-catchem v isset metode, ale to je stejne prasarna, pac (jak bylo vyse zmineno) by pri kazdem issetu na neexistujici se invalidovala cache cizich klicu. FUJ.
nakonec jsem si uvedomil, ze existuje mnohem cistsi reseni – v issetu proste kontrolovat jen nactene sloupec. takze nyni dle predpokladani funguje:

isset($book->id); // TRUE
isset($book->neexistujici_sloupec); // FALSE

dalsim krokem, co potrebujeme testovat je, kdyz odkazujici sloupec je null. drive slo pouzit isset($book->translator). Toto chovani bylo tedy ted zmeneno, protoze jde efektivne nahradit:

// $book->author_id = 1;
// $book->translator_id = NULL;
isset($book->author); // FALSE; - toto je onen BC BREAK
isset($book->translator); // FALSE

// nove testujte pomoci:
isset($book->author_id); // TRUE
isset($book->translator_id); // FALSE

Pokud mate konzistentni db, je tento pristup bezpecny. Pokud ne, stale muzete delat pohodove:

if ($book->translator != NULL)

zaver: bohuzel se jedna o BC break, na druhou stranu fixuje jeden zasadni bug/nedostatek bez nejake nelogicke a nekonzistentni zmeny API.

poznamka: zatim vse, co sem tu napsal je predmetem pullrequestu, uvidime, zda david prijme ;)

_Martin_

Pěkné =) Ještě přemýšlím, zda by nešlo udělat, aby se při požadavku na isset($book->author) v případě FALSE zkusil dohledat skutečný sloupeček (tj. autor_id) a v případě nalezení by se test provedl na něm: isset($book->author_id).

Podobně to bývalo před tvou úpravou, ale tam se narážel na problém té výjimky a invalidace keše, což vedlo ke zbytečným dotazům. Ta keš by se nemusela invalidovat sama. Akorát nevím, jak sjednotit chování metody getBelongsToReference napříč reflexemi. Protože u Conventional vrátí nějaké sloupečky vždy a pak se musí ověřit, zda existují. Naopak Discovered neexistenci řeší rovnou výjimkou.

Je otázkou, zda by šlo jednoduše udělat ověření i u Conventional, nebo zda raději brát tuto metodu jako „možné návrhy vazeb“ a poté k ní tak z venku přistupovat.

hrach

@_Martin_:

ne, to by neslo. prave kvuli te invalidaci cache. a podstrkavat tam nekde podminky, ze jenom tentokrat se to ma neinvalidovat je magie, ktera je ted v nette zakazana ;)
co se tyka sjednoceni, tak v podstate si myslim, ze to je proti principu; na prvni pohled bych mozna na to chtel spolehat, ale na druhy pohled je to proste presne to, co vyvojar potrebuje. Potrebuje vedet o chybach hned. Proste spoleha na cizi klice a nema je definovane? Hned se to dozvi. Kdyz pouziva konvence, tak se to dozvi az v sql. Pokud vyvojar kombinuje oba pristupy, myslim, ze nic nebrani tomu, aby nejak dane reflection spojil ;)

edke

Moj priklad:

// $row->expire = NULL;
isset($row->expire); // FALSE
$row->offsetExists('expire'); // FALSE

Prvy priklad je ok, co ten druhy ? V druhom pripade testujem, ci existuje taky stlpec, preco sa berie do uvahy aj jeho aktualna hodnota NULL ? Mna v tom pripade nezaujima hodnota.

hrach

@edke: ActiveRow poskytuje stejne api jak pro array access tak pro property access. V tomto smeru je cilene zachovane api. Krom toho, offsetExists() se rozhodne nepouziva verejne.