Tharos / LeanMapper

Lean Mapper is a tiny ORM based on powerful Dibi database abstraction library for PHP.
MIT License
87 stars 35 forks source link

Entity: added method initAfterDefaults() #106

Closed janpecha closed 4 years ago

janpecha commented 7 years ago

Kompromis mezi PR #69 a bugem #85.

Co na to říkáte?

castamir commented 7 years ago

Uff, tohle si nechám projít hlavou ;)

mibk commented 7 years ago

Já osobně bych tam tohle tedy nepřidával. Nebo má někdo pocit, že #69 je potřeba řešit nějak často? Podle mě k tomu tedy často nedochází (já to nepotřeboval nikdy). Raději bych ponechal věci jednoduché tak, jak jsou teď.

janpecha commented 7 years ago

Casto to potreba neni, ale prijde mi to docela uzitecne a obcas by se to hodilo. Navic to je vec, ktera kod knihovny nijak vyrazne nezkomplikuje.

mibk commented 7 years ago

Mně přijde, že každá, byť malá, komplikace je na škodu. Jestli to Tobě teď ušetří 4 řádky napříč všemi projekty, nevyváží fakt, že to takhle zkomplikuje API poměrně neintuitivní novou metodou, se kterou nováček vůbec nebude vědět, jak naložit a k čemu vlastně slouží.

janpecha commented 7 years ago

Jasně, naprosto s tebou souhlasím, na druhou stranu:

mibk commented 7 years ago
  1. Teď si nejsem jist, jestli je initDefaults někde zdokumentováno, ale initAfterDefaults asi nikdy nebude. Tohle by mohlo být pro nováčky trochu zmatené.
  2. JJ, #69 i #85 jsem četl, ale vzhledem ke stáří mi přišlo, že to úplně nezbytná věc není.
  3. Pokud by přeci jen mělo být toto nějak vyřešeno, nemohla by metoda initDefaults být volána s argumentem $args, pokud jsem tedy něco nepřehlédl?

Stejně mi přijde, že #69 byl prostě omyl přírody a že by bylo mnohem jednodušší, kdyby se toto vůbec neřešilo. Obzvláště v tomto stádiu LM bych nepřidával nové API, které se "občas hodí".

Nebo nemáš nějaký konkrétní use case, na kterém by se dalo rozsoudit, jak moc je tato feature žádoucí a jak moc bolestivý je její workaround?

janpecha commented 7 years ago
  1. initDefaults aktuálně nikde zdokumentováno není, jediná zmínka v dokumentaci je v changelogu k verzi 2.0.0 (http://leanmapper.github.io/cs/changelog/#200-26-8-2013) a odkaz do fóra.
  2. nezbytná asi ne, ale možnost inicializovat property na základě předaných argumentů vnímám jako docela užitečnou
  3. předat $args do initDefaults by šlo, vidím u toho ale zásadní nedostatek - budeš muset pracovat s blíže nespecifikovanou a nezvalidovanou strukturou (array, Traversable, NULL), místo s konkrétními zvalidovanými property

Konkrétní use case bohužel aktuálně nemám. Dokonce jsem nikdy nepoužil ani initDefaults, ale na dalším projektu plánuju přistoupit k tvorbě entit trochu jiným způsobem a cítím, že tam by se oboje hodilo. Potvrzené to ale nemám.

K workaroundu - ono už při pohledu na constructor člověk vnímá, že to je něco, do čeho nechce moc šťourat, protože se to klidně v další verzi může změnit, musíš znát vnitřní implementaci (jaké argumenty constructor přijímá, co s nimi dělá), defacto to musíš hackovat a doufat, že se to v budoucnu nerozbije. Aktuálně bych to řešil asi takto:

public function __construct($arg = null)
{
  parent::__construct($arg);
  if (!($arg instanceof Row)) {
    // inicializace
  }
}

vs.

public function initAfterDefaults()
{
  // inicializace
}
mibk commented 7 years ago
  1. $args by se přeci dalo před vložením do initDefaults předzpracovat tak, aby to bylo vždy array. Mělo by to i tu výhodu, že by se dalo zjistit, které hodnoty uživatel vlastně inicializoval.

Nicméně se pořád nedokážu zbavit dojmu, že si maličko protiřečíš. Na jednu stranu řekneš, že to vnímáš jako "docela užitečné" a pak se přiznáš, že jsi nikdy nepoužil ani initDefaults (kterou jsem já osobně použil pouze pro inicializaci DateTime polí). Aktuálně pouze cítíš, že se Ti to bude hodit, z čehož cítím já, že to využiješ jednou, dvakrát v celém projektu.

Právě proto by mě zajímaly nějaké konkrétní use case, abychom to na nich mohli rozsoudit a případně pro ony use case vymyslet lepší a jednodušší řešení než přidávat nové API.

Také jsem spíše čekal, že jako workaround by posloužilo využít nějaké "factory" metody, která správně entitu inicializuje podle vstupních parametrů a entitu vrátí. Člověk se nemusí zuby nehty držet slovíčka new.

Začínám se taky trochu cítit, že dělám z komára velblouda, ale přijde mi, že takovými malými vylepšeními, které přece nemohou vadit, to začíná. Ono se to ale pak nakupí a rázem se z projektu stane moloch.

janpecha commented 7 years ago
  1. stále to má tu nevýhodu, že to bude nezvalidovaná množina hodnot - co když entita bude předpokládat, že dostane hodnotu created, která bude DateTime, ale v $args místo toho dostane bool? Musel bys to zbytečně ošetřovat na 2 místech (v initDefaults a v anotaci), IMHO lepší je nechat ty hodnoty nejprve probublat do $entity->assign(), kde se zavolají všechny settery, passThru, validace, atd. a ve výsledku pracovat už s konkrétními validními properties

Protiřečím? Ani ne :) Pointa je, že přestože Lean Mapper obsahuje spoustu funkcí, které jsem nikdy nevyužil (nebo jen velmi velmi zřídka), považuji je za užitečné a jsem rád, že v LM jsou.


Samozřejmě využití nějaké "factory" metody je také jedna z možností, to ale neřeší to, že entita by měla zodpovídat za svůj stav, takže by si i měla hlídat jaké hodnoty při inicializaci dostane a podle toho reagovat. To je jen a pouze její zodpovědnost, neměla by být přenesena na nikoho jiného.


Já tě chápu, přesto si nemyslím, že tady nějak reálně hrozí, že by se z LM stal někdy moloch. Moc si ani nedokážu představit, o co všechno by musel být rozšířen, aby nějak výrazně nakynul. Všechno podstatné už obsahuje a k úplné dokonalosti mu chybí jen pár drobností.


Prozatím bych tu naší diskusi ukončil, sami dva tady asi nic nevyřešíme, chtělo by to taky názor někoho dalšího :-)