New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC - příznak m:hasMany(#inversed) #123

Closed
janpecha opened this Issue Apr 19, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@janpecha
Collaborator

janpecha commented Apr 19, 2018

Pokud v současné době potřebujeme vyjádřit obrácenou M:N vazbu (např. tag => knihy pro tabulku book_tag z quick startu), musíme uvést v příznaku m:hasMany název tabulky, jinak bude Lean Mapper dohledávat vazbu ve špatné tabulce (pro quick start v tabulce tag_book).

Napadlo mě zavést pro příznak m:hasMany nový modifikátor #inversed (podobně jako existuje modifikátor #union), který by způsobil, že v reflexi se prohodí zdrojová a cílová tabulka. Teoreticky by to nemělo způsobit žádný BC break.

Co říkáte?

@janpecha janpecha added the RFC label Apr 19, 2018

@janpecha janpecha added this to the Version 3.3.0 milestone Apr 19, 2018

@castamir

This comment has been minimized.

Show comment
Hide comment
@castamir

castamir Apr 19, 2018

Collaborator

není to jen otázkou mapperu?

Collaborator

castamir commented Apr 19, 2018

není to jen otázkou mapperu?

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Apr 20, 2018

Collaborator

To sice částečně je, ale mapper počítá s tím, že do něj budou předány správné údaje. V současné chvíli to probíhá takto:

  1. z názvu zdrojové entity se pomocí IMapper::getTable() získá název zdrojové tabulky (viz)
  2. z názvu typu u property se pomocí IMapper::getTable() získá název cílové tabulky (viz)
  3. název zdrojové a cílové tabulky se předá do IMapper::getRelationshipTable() a ta vrátí název spojovací tabulky. Výchozí implementace pracuje tak, že tyto názvy jen spojí pomocí podtržítka (viz)
  4. pro volání $book->tags to správně vrátí tabulku book_tag, ale pro $tag->books to vrátí špatný název tag_book
  5. v teoretické rovině je problém v metodě IMapper::getRelationshipTable() - měla by vrátit správnou tabulku, ale nevrací, prakticky je to však složitější:
    • mapper nemůže korekci udělat automaticky, protože nemá informaci o tom, která tabulka je primární a která sekundární, spoléhá se na to, že mu budou předány správné parametry
    • uživatel by si tedy musel upravovat mapper a pro každou takovou spojovací tabulku dopisovat podmínku if ($sourceTable === 'tag' && $targetTable === 'book') { return 'book_tag'; }
    • to je trochu nepraktické (hlavně při větším množství takových vazeb) a osobně v takové chvíli raději dopíšu název té tabulky přímo do příznaku v entitě Tag ($books m:hasMany(:book_tag))
    • a tuhle situaci by právě řešil ten modifikátor #inversed, který by naznačil, že se jedná o obrácenou hasMany vazbu
Collaborator

janpecha commented Apr 20, 2018

To sice částečně je, ale mapper počítá s tím, že do něj budou předány správné údaje. V současné chvíli to probíhá takto:

  1. z názvu zdrojové entity se pomocí IMapper::getTable() získá název zdrojové tabulky (viz)
  2. z názvu typu u property se pomocí IMapper::getTable() získá název cílové tabulky (viz)
  3. název zdrojové a cílové tabulky se předá do IMapper::getRelationshipTable() a ta vrátí název spojovací tabulky. Výchozí implementace pracuje tak, že tyto názvy jen spojí pomocí podtržítka (viz)
  4. pro volání $book->tags to správně vrátí tabulku book_tag, ale pro $tag->books to vrátí špatný název tag_book
  5. v teoretické rovině je problém v metodě IMapper::getRelationshipTable() - měla by vrátit správnou tabulku, ale nevrací, prakticky je to však složitější:
    • mapper nemůže korekci udělat automaticky, protože nemá informaci o tom, která tabulka je primární a která sekundární, spoléhá se na to, že mu budou předány správné parametry
    • uživatel by si tedy musel upravovat mapper a pro každou takovou spojovací tabulku dopisovat podmínku if ($sourceTable === 'tag' && $targetTable === 'book') { return 'book_tag'; }
    • to je trochu nepraktické (hlavně při větším množství takových vazeb) a osobně v takové chvíli raději dopíšu název té tabulky přímo do příznaku v entitě Tag ($books m:hasMany(:book_tag))
    • a tuhle situaci by právě řešil ten modifikátor #inversed, který by naznačil, že se jedná o obrácenou hasMany vazbu
@castamir

This comment has been minimized.

Show comment
Hide comment
@castamir

castamir Apr 20, 2018

Collaborator

omlouvam se, jak uz 3. rok nedelam v PHP a tim padem asi s LM, tak mam problem se do toho dostat a dat ti relevantni zpetnou vazbu

Collaborator

castamir commented Apr 20, 2018

omlouvam se, jak uz 3. rok nedelam v PHP a tim padem asi s LM, tak mam problem se do toho dostat a dat ti relevantni zpetnou vazbu

@mibk

This comment has been minimized.

Show comment
Hide comment
@mibk

mibk Apr 23, 2018

Contributor

Na jednu stranu proti tomu asi nic nemám, ale na druhou stranu Lean Mapper tady už nějakou chvíli je a zatím jsme se bez toho obešli, tak necítím nějaký silný důvod, proč to do knihovny přidávat. Počet uživatelů klesá, takže to reálně nebude snad ani nikdo používat. Jen to bude další API, které se bude muset udržovat, ideálně k němu psát dokumentace. Prostě celkově je to dost práce na to, že tahle situace nastane v jednom projektu tak jednou až dvakrát. Já jsem zastánce spíše nechat věci v co nejjednodušší podobě. Pokud přidávat nějaký zlepšovák, mělo by to řešit nějaký závažnější problém. To platí obzvláště u umírajících knihoven.

Contributor

mibk commented Apr 23, 2018

Na jednu stranu proti tomu asi nic nemám, ale na druhou stranu Lean Mapper tady už nějakou chvíli je a zatím jsme se bez toho obešli, tak necítím nějaký silný důvod, proč to do knihovny přidávat. Počet uživatelů klesá, takže to reálně nebude snad ani nikdo používat. Jen to bude další API, které se bude muset udržovat, ideálně k němu psát dokumentace. Prostě celkově je to dost práce na to, že tahle situace nastane v jednom projektu tak jednou až dvakrát. Já jsem zastánce spíše nechat věci v co nejjednodušší podobě. Pokud přidávat nějaký zlepšovák, mělo by to řešit nějaký závažnější problém. To platí obzvláště u umírajících knihoven.

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Apr 23, 2018

Collaborator

Lean Mapper tady už nějakou chvíli je a zatím jsme se bez toho obešli

To je sice pravda, ale taky to neznamená, že je to tak správně. Obejít se dá totiž bez spousty věcí. Cílem tohoto drobného vylepšení je primárně to, aby aplikace obsahovaly co nejméně hardcodovaných názvů a hlavní díl práce vždy odvedl mapper. Osobně bych to hodně ocenil a hodilo by se mi to skoro u každé aplikace. Mimochodem z hodně podobného soudku je i změna navrhovaná v #77.

Pokud přidávat nějaký zlepšovák, mělo by to řešit nějaký závažnější problém. To platí obzvláště u umírajících knihoven.

Tady se asi neshodneme v pohledu, jestli se pokoušet takové "umírající" knihovny posouvat dál, nebo zcela rezignovat na jejich vývoj. I takové knihovny totiž mají uživatele, kteří nová vylepšení uvítají. Potíž je také s definicí "závažnějšího problému". Podle mě tato úprava řeší závažnější problém - za prvé čistotu kódu a za druhé ochranu uživatelů před "chybami" (viz příklad níže).


Napadl mě k tomu jeden příklad - částečně vychází ze situace, kterou jsem nedávno řešil. Řekněme, že potřebujeme prefixovat názvy všech tabulek. Prefix se může měnit - pro každou konkrétní instalaci může být jiný. Upravíme tedy mapper tak, aby k názvům tabulek doplňoval prefix.

V aplikaci pak máme knihy a tagy a potřebujeme volat jak $book->tags, tak i $tag->books. Pro property $tags m:hasMany v entitě Book to není problém - mapper správně vrátí prefix_book_tag. Horší je to pro entitu Tag a její property $books m:hasMany(:book_tag) - v takovém případě se mapper vůbec nedostane ke slovu a nemůže tedy do názvu tabulky doplnit prefix a uživatel v podstatě nemá možnost, jak z toho ven. Řešením by bylo právě $books m:hasMany(#inversed).


Jinak díky oběma za názory, dost mi pomohli.

Collaborator

janpecha commented Apr 23, 2018

Lean Mapper tady už nějakou chvíli je a zatím jsme se bez toho obešli

To je sice pravda, ale taky to neznamená, že je to tak správně. Obejít se dá totiž bez spousty věcí. Cílem tohoto drobného vylepšení je primárně to, aby aplikace obsahovaly co nejméně hardcodovaných názvů a hlavní díl práce vždy odvedl mapper. Osobně bych to hodně ocenil a hodilo by se mi to skoro u každé aplikace. Mimochodem z hodně podobného soudku je i změna navrhovaná v #77.

Pokud přidávat nějaký zlepšovák, mělo by to řešit nějaký závažnější problém. To platí obzvláště u umírajících knihoven.

Tady se asi neshodneme v pohledu, jestli se pokoušet takové "umírající" knihovny posouvat dál, nebo zcela rezignovat na jejich vývoj. I takové knihovny totiž mají uživatele, kteří nová vylepšení uvítají. Potíž je také s definicí "závažnějšího problému". Podle mě tato úprava řeší závažnější problém - za prvé čistotu kódu a za druhé ochranu uživatelů před "chybami" (viz příklad níže).


Napadl mě k tomu jeden příklad - částečně vychází ze situace, kterou jsem nedávno řešil. Řekněme, že potřebujeme prefixovat názvy všech tabulek. Prefix se může měnit - pro každou konkrétní instalaci může být jiný. Upravíme tedy mapper tak, aby k názvům tabulek doplňoval prefix.

V aplikaci pak máme knihy a tagy a potřebujeme volat jak $book->tags, tak i $tag->books. Pro property $tags m:hasMany v entitě Book to není problém - mapper správně vrátí prefix_book_tag. Horší je to pro entitu Tag a její property $books m:hasMany(:book_tag) - v takovém případě se mapper vůbec nedostane ke slovu a nemůže tedy do názvu tabulky doplnit prefix a uživatel v podstatě nemá možnost, jak z toho ven. Řešením by bylo právě $books m:hasMany(#inversed).


Jinak díky oběma za názory, dost mi pomohli.

@mibk

This comment has been minimized.

Show comment
Hide comment
@mibk

mibk Apr 23, 2018

Contributor

Chápu, že i uživatelé umírajících knihoven uvítají nová vylepšení, Tebe ale beru jako spoluautora a nikdo jiný si na nic podobného nestěžoval. Beru to z pragmatického pohledu, že jakákoli nová vlastnost, která se do jakékoli knihovny zakomponuje, může zvýšit počet potenciálních bugů a nechtěných BC breaků. V situaci, kdy už tato knihovna přišla o 2 hlavní vývojáře, bych prostě automaticky volil spíše strategii starání se o knihovnu ve formě opravy případných nově objevených bugů.

Ale jak jsem řekl na začátku, vlastně proti tomu nic nemám. Jen je škoda, že jsme tohle neřešili na úplném začátku vývoje. Asi bychom tuhle issue mohli rozdělit do dvou bodů: jestli chceme mít možnost "prohození zdrojové a cílové tabulky", a pokud ano, jakým způsobem to chceme implementovat. Je třeba divné, že můžu v jednom zápisu vazby použít #inversed a zároveň i tabulku pojmenovat: m:hasMany(:tag_books#inversed), čímž je příznak ignorován. Bohužel mě nenapadlo nic lepšího (leda tedy pojmenovat tu vazební tabulku něco jako ~: m:hasMany(:~), jelikož ~ se v mnoha jazycích používá jako bitwise complement a mohl by tedy evokovat "obrácení". S tím ale také nejsem spokojen a navíc by to vlastně i byl BC break pro někoho, kdo tu tabulku takto opravdu pojmenoval :-D)


Co se týče #77, jestli tomu správně rozumím, tak toto jsem navrhoval v poznámce pod čarou již 4 a půl roku zpátky a nikdo mi na to nic moc neřekl: https://forum.dibiphp.com/cs/14592-lean-mapper-tenke-orm-nad-dibi?p=18#p115003.

Contributor

mibk commented Apr 23, 2018

Chápu, že i uživatelé umírajících knihoven uvítají nová vylepšení, Tebe ale beru jako spoluautora a nikdo jiný si na nic podobného nestěžoval. Beru to z pragmatického pohledu, že jakákoli nová vlastnost, která se do jakékoli knihovny zakomponuje, může zvýšit počet potenciálních bugů a nechtěných BC breaků. V situaci, kdy už tato knihovna přišla o 2 hlavní vývojáře, bych prostě automaticky volil spíše strategii starání se o knihovnu ve formě opravy případných nově objevených bugů.

Ale jak jsem řekl na začátku, vlastně proti tomu nic nemám. Jen je škoda, že jsme tohle neřešili na úplném začátku vývoje. Asi bychom tuhle issue mohli rozdělit do dvou bodů: jestli chceme mít možnost "prohození zdrojové a cílové tabulky", a pokud ano, jakým způsobem to chceme implementovat. Je třeba divné, že můžu v jednom zápisu vazby použít #inversed a zároveň i tabulku pojmenovat: m:hasMany(:tag_books#inversed), čímž je příznak ignorován. Bohužel mě nenapadlo nic lepšího (leda tedy pojmenovat tu vazební tabulku něco jako ~: m:hasMany(:~), jelikož ~ se v mnoha jazycích používá jako bitwise complement a mohl by tedy evokovat "obrácení". S tím ale také nejsem spokojen a navíc by to vlastně i byl BC break pro někoho, kdo tu tabulku takto opravdu pojmenoval :-D)


Co se týče #77, jestli tomu správně rozumím, tak toto jsem navrhoval v poznámce pod čarou již 4 a půl roku zpátky a nikdo mi na to nic moc neřekl: https://forum.dibiphp.com/cs/14592-lean-mapper-tenke-orm-nad-dibi?p=18#p115003.

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Apr 24, 2018

Collaborator

To je pravda, použití m:hasMany(:tag_books#inversed) by se muselo ošetřit, aby vyhazovalo nějakou LogicException.


#77 - že se něco takového objevilo už na fóru mi úplně uniklo - to je právě nevýhoda jednoho dlouhého vlákna, kde se řeší naráz příliš věcí :-/ Osobně bych to už tehdy docela uvítal. Prozatím jsem přidal do #77 odkaz na tu diskusi na fóru.

Collaborator

janpecha commented Apr 24, 2018

To je pravda, použití m:hasMany(:tag_books#inversed) by se muselo ošetřit, aby vyhazovalo nějakou LogicException.


#77 - že se něco takového objevilo už na fóru mi úplně uniklo - to je právě nevýhoda jednoho dlouhého vlákna, kde se řeší naráz příliš věcí :-/ Osobně bych to už tehdy docela uvítal. Prozatím jsem přidal do #77 odkaz na tu diskusi na fóru.

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha May 25, 2018

Collaborator

Připravil jsem pro tuhle změnu PR #125. Pokud je v anotaci uveden zároveň název tabulky (m:hasMany(:tag_books#inversed)) nebo je #inversed uvedeno u jiného typu vazby, vyhazujeme výjimku.

Collaborator

janpecha commented May 25, 2018

Připravil jsem pro tuhle změnu PR #125. Pokud je v anotaci uveden zároveň název tabulky (m:hasMany(:tag_books#inversed)) nebo je #inversed uvedeno u jiného typu vazby, vyhazujeme výjimku.

janpecha added a commit that referenced this issue Jun 13, 2018

Merge pull request #125 from inlm/pr/hasMany-inversed
Added support for #inversed flag in m:hasMany (for #123)
@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Jun 13, 2018

Collaborator

Merged #125

Collaborator

janpecha commented Jun 13, 2018

Merged #125

@janpecha janpecha closed this Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment