neočekávané chování setDefaults

michal.lohnisky

Dobrý den,

používám nejnovější Nette dev z GitHubu.

Ve svém projektu používám addDynamic. Jsme ve stavu, kdy je formulář již odeslaný, je připojený k presenteru. V konstruktoru formuláře používám následující řádky:

dump($values);
dump($form->values);
$form->setDefaults($values);
dump($form->values);

Tímto dostanu následující výstup:

array (2)
items => array (2)
| _0 => array (0)
| 2598 => array (10)
| | id => 2598

Nette\ArrayHash (4)
items => Nette\ArrayHash (2)
| _0 => Nette\ArrayHash (8)
| | id => "1"
| _1 => Nette\ArrayHash (8)
| | id => "2"

Nette\ArrayHash (4)
items => Nette\ArrayHash (3)
| _0 => Nette\ArrayHash (8)
| | id => "1"
| _1 => Nette\ArrayHash (8)
| | id => "2"
| 2598 => Nette\ArrayHash (8)
| | id => ""

Tedy z databáze tam chci vložit 2 záznamy, _0 a 2598, v POST mi přišly také 2 záznamy, _0 a _1. Funkce setDefaults mi však do formuláře přidá i řádek s indexem 2598, ale prázdný (protože id není disabled). Já bych očekával, že mi setDefaults nepřidá řádek s indexem 2598 vůbec a že hodnoty formuláře budou přesně odpovídat hodnotám POST. Problém je v tom, že mi pak formulář hlásí, že jsem nevyplnil povinné pole id.

Je to tedy očekávané chování? Jak bych měl postupoval, aby mi vše fungovalo?

thorewi

Jinymi slovy ocekavas, ze prvni i druhe $form->values ti vrati to stejne, je to tak? Tak bych to ocekaval i ja. Mohl by to nekdo objasnit prosim? (@enumag ?)

enumag

Není mi bohužel jasné proč k tomu dochází. Kromě toho @dg mou původní implementaci totálně překopal, mohl bys vyzkoušet kód z mého PR zda způsobuje tentýž problém?

EDIT: addDynamic přidává IControl nebo Container?

EDIT2: Přepdokládám, že Container. Myslím, že chyba je v kódu replikátoru – není s tím kompatibilní a třetí parametr ignoruje. Tzn. ten commit způsobil nepředpokládabý BC break. Navíc mám podezření, že moje původní implementace to dělat nebude. Prosím o ověření než to nahlásím na GitHubu.

Editoval enumag (26. 4. 2013 17:38)

michal.lohnisky

Jo, s tvojí původní verzí funguje špatně addDynamic, ale zase setDefaults funguje správně.

enumag

Můžeš rozvést „funguje špatně addDynamic“? ;-)

michal.lohnisky

Zkrátka se mi nenaplní hodnotami, přesněji teď nevím.

enumag

Ok, zkus pls zjistit proč. Možná je i v mém kódu nějaký BC break o kterém nevím.

michal.lohnisky

Dobře. K tomu kódu replikátoru: asi mám jinou verzi, ve které je funkce takto:

public function setValues($values, $erase = FALSE, $onlyDisabled = false)
{
	foreach ($values as $name => $value) {
		if ((is_array($value) || $value instanceof \Traversable) && !$this->getComponent($name, FALSE)) {
			$this->createOne($name);
		}
	}

	return parent::setValues($values, $erase, $onlyDisabled);
}

EDIT: Jen mě napadá, že jsem to možná upravil já, protože mi PHP hlásilo STRICT při přechodu na nejnovější Nette dev z GitHubu.

EDIT2: Takže Replicator nejspíš vytvoří novou položku i v případě, že následující setValues nenastaví žádné hodnoty.

EDIT3: A proto bych asi směřoval tento problém na Replicator.

Editoval michal.lohnisky (26. 4. 2013 18:02)

thorewi

jj to

, $onlyDisabled = false

sis tam pridal ty…

michal.lohnisky

Napsal jsem Hosiplanovi.

enumag

@michal.lohnisky: To sis tam nejspíš přidal protože v kódu replikátoru nic takového nevidím. Tohle se ale pravděpodobně bude řešit na straně Nette protože takhle je tam BC break o kterém se nevědělo. A pokud se to v Nette bude měnit, bylo by fajn udělat to úplně bez BC breaku, tzn. bylo by dobré zjistit jaký problém způsobuje moje implementace a proč. Zkus pls otestovat mou verzi toho commitu s replikátorem z GitHubu a případně zjistit kde je problém.

Editoval enumag (26. 4. 2013 18:09)

michal.lohnisky

Aha, jasně, dobře, už mi to došlo.

thorewi

jj uz jsem to michalovi vysvetlil on uz ted sel z kancelare domu, tak se na to mrkne v pondeli…

enumag

Ok, díky. :-)

michal.lohnisky

Takže co jsem zjistil:

pokud jsme před řádkem

foreach ($this->getComponents() as $name => $control) {

ve funkci setDefaults() v souboru Nette/Forms/Container.php a v $this je „Kdyby\Forms\Containers\Replicator“, tak příkaz

$this->getComponents()

vrátí prázdné pole, protože v Replikátoru doopravdy ještě žádné komponenty nejsou. Ty se tam mají přidat pomocí setValues()). Foreach se tedy neprovede, tím se nenastaví žádné hodnoty a Replicator zůstane prázdný, bez hodnot.

enumag

Aha, takže je to BC break v každym případě, protože před tím pull requestem metoda setDefaults volala setValues, kde se přidaly ty komponenty. Má implementace to naruší a nemůže tu fungovat, Davidova zase přidává parametr což pokazí dědičnost. Nevidím z toho cestu ven bez BC breaku…

thorewi

No myslim ze ted je tam tolik bc breaku ze uz je to jedno.chtelo by to tedy nazor davida a vyresit to jednim nebo druhym zpusobem.

EDIT: nechci aby to znelo ze si stezuju, jsem pro kazdy pokrok i za cenu BC breaku, tak se pojdme domluvit na nejakem reseni :)

Editoval thorewi (29. 4. 2013 17:38)

enumag

@thorewi: Ke stejnému závěru jsem došel také. Napsal jsem to Davidovi na GH už včera, tak uvidíme co řekne. :)

thorewi

@enumag: jo ja jsem mu psal v pondeli taky email, tak snad bude mit chvilku…

David Grudl

Zmíněné chování v úvodním postu je správné (byť sis musel opravit E_STRICT v replicatoru).

Ale je to samozřejmě nešťastné chování, takže řešením bude commit #1e3606 (částečně) revertnout a

vrátit se k jednoduchému chápání setDefaults jakožto metody, která nastavuje hodnoty jen poprvé
doporučit v dokumentaci, že společně s každým setDisabled() je třeba nastavit i setValue()
změnu v Nette\Forms\Controls\BaseControl.php bych možná ponechal

Filip Procházka

Ten strict už je opravený.

enumag

@David Grudl: To bude trochu zvláštní chování když setDefaults nebude dělat totéž jako foreach ... setDefault. A podmíněné nastavování setValue ručně u fieldů které někdy jsou disabled a někdy ne mne tady docela pálí. SetDefaults nevolám v createComponent, ale až v action.

Editoval enumag (2. 5. 2013 14:16)

David Grudl

A víš o jakémkoliv jiném řešení? (Shodujeme se snad v tom, že Michalem popisované chování je v hlediska záměru správné.)

Filip Procházka

Imho by se setDefaults zjednodušeně mělo chovat takto

public function setDefaults($values)
{
	if (!$this->isSubmitted()) {
		$this->setValues($values);
	}
}

Ty čárymáry s disabled v tom dělají jenom chaos.

enumag

@David Grudl: Moje původní implementace to imho řeší celkem dobře, jen by se muselo upravit chování replikátoru aby nepředpokládal, že setDefaults volá setValues, což je triviální.

@Filip Procházka: Souhlasím, že ty čárymáry s disabled moc hezké nejsou, jen se mi nelíbí když ve formuláři po chybném odeslání u disabled políček zmizí hodnota kterou jsem tam pomocí setDefaults přece dával.

David Grudl

enumag napsal(a):

@David Grudl: Moje původní implementace to imho řeší celkem dobře…

Proč by setDefaults neměla přidat řádek s indexem 2598?

michal.lohnisky

IMHO by to mělo fungovat tak, že pokud mezi prvky Replicatoru není ani jeden disabled, neměl by se vytvořit ani řádek s indexem 2598. Já jsem například v situaci, kdy může uživatel smazat některou položku Replicatoru a já to zjistím tak, že při zpracování formuláře nedostanu danou položku nazpět. Sice to v mém případě mohu detekovat tak, že u dané položky není vyplněno id, ale podle mě jsou situace, kdy by byla i prázdná hodnota validní a nedalo by se to podle toho zjistit. Proto si myslím, že absenci některého řádku Replicatoru bych měl zjistit už tím, že tam není celý řádek.

enumag

David Grudl napsal(a):

enumag napsal(a):

@David Grudl: Moje původní implementace to imho řeší celkem dobře…

Proč by setDefaults neměla přidat řádek s indexem 2598?

Na to ti schválně neodpovím, je to totiž irelevantní. Ať už zvolíš libovolnou implementaci, @Filip Procházka si chování replikátoru stejně napíše tak jak on uzná za vhodné.

thorewi

muzu se laicky zeptat, jaky BC break se vlastne resi? to ze by se musel upravit replikator a ze by prestali fungovat nadstavby nad replikatorem nebo? nejak se nechytam…