Skip to content
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

Změna výchozí hodnoty vazby m:hasOne #77

Closed
castamir opened this issue Oct 10, 2014 · 8 comments
Closed

Změna výchozí hodnoty vazby m:hasOne #77

castamir opened this issue Oct 10, 2014 · 8 comments
Assignees
Milestone

Comments

@castamir
Copy link
Collaborator

V podstatě od zveřejnění LM si nepamatuju situaci, kdy by se mi lišil název property od prvního parametru této vazby. Všechny property s takovou vazbou mají u mě zbytečně dlouhou a opakující se konstrukci:

/**
 * @property BaseEntity $ancestor m:hasOne(ancestor)
 * @property BaseEntity $descendant m:hasOne(descendant)
 */

Bylo by možné si nastavit výchozí hodnoty u takové vazby? V tomto případě rovnu názvu samotné property?


Ještě dodám, že by v takovém případě bylo potřeba propašovat mapper do parseru tak, aby se název property korektně přeložil z např. myColumn na my_column v závislosti právě na mapperu.

@achtan
Copy link

achtan commented Oct 10, 2014

+1 ja som sa presne toto uz raz pokusal pretlacit, sand sa to tera podari! :)

@castamir
Copy link
Collaborator Author

přidán dodatek ohledně překladu názvu property na databázový sloupec

@Tharos
Copy link
Owner

Tharos commented Oct 10, 2014

Já nejsem proti, ale měli bychom to nějak rozumně vymyslet. Nerad bych kvůli tomu udělal nějaký hluboký BC break. Také bych byl nerad, aby to rozšířilo rozhraní IMapper (třeba o další parametr u vybraných metod), protože jeho složitost už mi přijde na hraně.

Zamyslím se, jak by tohle mohlo jít elegantně vyřešit a zároveň cestou nejmenšího odporu.

@Tharos Tharos added the RFC label Oct 10, 2014
@castamir
Copy link
Collaborator Author

castamir commented Nov 2, 2015

Já myslím, že tam není potřeba nic zvláštního vymýšlet... :D

Mám-li např. vazbu

/**
 * @property Author $author m:hasOne
 * @property Author $anotherAuthor m:hasOne
 */

pak bych ocekaval vychozi hodnoty pro nazev sloupce: author resp. another_author (dle mapovaci konvence field->column, kterou se to stejne prohani jiz nyni) a pro tabulky v obou pripadech author (dle mapovani z EntityClass na tabulky)

Ano, bude to BC break, protoze se zmeni vychozi hodnota, ale v praxi tam stejne vsichni maji

/**
 * @property Author $author m:hasOne
 * @property Author $anotherAuthor m:hasOne(another_author)
 */

a pokud tam nekdo ma neco jineho, tak ma stejne uplne jiny, svuj vlastni specificky nazev, ktery tam musi ponechat nezavisle na teto zmene, takze se ho to nedotkne...

U ostatnich vazeb to asi nebude tak horke, ale u te HasOne by se to docela hodilo. Ja to ted sice resim pres life-templates v phpstormu, ale to taky neni uplne to prave orechove... nechci to tam mit. Jedine, kde mi to vlastne prichazi v uvahu a kde bych to teoreticky chtel doplnovat, tak je u vazby HasMany a to u nazvu propojovaci tabulky (obcas propojuju 2 tabulky s dlouhymi nazvy, cimz ten problem dlouhych nazvu jeste zhorsim :D)

@castamir castamir self-assigned this Feb 17, 2016
@castamir castamir added this to the v3.0 milestone Feb 21, 2016
@castamir
Copy link
Collaborator Author

je to BC Break - odlozeno

@castamir castamir removed this from the v3.0 milestone Feb 29, 2016
@janpecha janpecha added this to the Version 3.3.0 milestone Apr 15, 2018
@janpecha
Copy link
Collaborator

Jen si tu odložím odkaz na forum, kde se tohle taky kdysi diskutovalo https://forum.dibiphp.com/cs/14592-lean-mapper-tenke-orm-nad-dibi?p=18#p115003

@janpecha
Copy link
Collaborator

janpecha commented Jul 8, 2018

Menší souhrn a možnosti řešení.

Aktuální stav

Řešení

V podstatě jediné řešení je rozšířit metodu IMapper::getRelationshipColumn o nové parametry, které zprostředkují mapperu potřebné informace. Dobře to kdysi shrnul Tharos na fóru. Možnosti jsou následující:

  1. přidat nepovinné parametry $entityClass a $property (getRelationshipColumn($sourceTable, $targetTable, $entityClass = null, $property = null)) - ale jak už psal Tharos ve zmiňovaném příspěvku, ty parametry by byly takové trochu divné

  2. přidat nepovinný parametr $relationshipName (getRelationshipColumn($sourceTable, $targetTable, $relationshipName = null)) s názvem vazby

    • uživatel se může rozhodnout jestli název vazby mapperu předá, nebo ne
    • mapper má volnou ruku v tom, jestli bude brát na název vazby ohled, či nikoli
    • pro hasOne by výchozí hodnotou $relationshipName mohl být název položky
    • pro ostatní typy vazeb by se $relationshipName nepředávalo
    • toto řešení je připraveno v PR Change of default column for hasOne (for #77) #127
  3. to samé jako 2) ale m:hasOne by bylo navíc rozšířeno o novou syntaxi, pomocí které by měl uživatel možnost ručně název vazby dospecifikovat (@property Author $maintainer m:hasOne(@maintainer))

Osobně bych byl ze začátku pro možnost 2), později by se to mohlo rozšířit o 3) pokud by to bylo potřeba.


Ať už zvolíme jakékoli řešení, bude to velký BC break, protože rozbijeme rozhraní IMapper a tím i všechny uživatelské implementace (Declaration of *::getRelationshipColumn(...) must be compatible with IMapper::getRelationshipColumn(...)). Řešit by se to zatím dalo tak, že $relationshipName nebude uvedeno mezi parametry, ale budeme ho v DefaultMapper vytahovat pomocí func_get_arg (viz PR #127). S reálnou úpravou rozhraní by se počkalo do verze 4.x.

janpecha added a commit to inlm/LeanMapper that referenced this issue Jul 19, 2018
- IMapper: added optional parameter $relationshipName for getRelationshipColumn()
- DefaultMapper: uses $relationshipName instead name of target table
janpecha added a commit that referenced this issue Jul 19, 2018
- IMapper: added optional parameter $relationshipName for getRelationshipColumn()
- DefaultMapper: uses $relationshipName instead name of target table
@janpecha janpecha modified the milestones: Version 3.3.0, Version 4.0.0 Jul 20, 2018
@janpecha
Copy link
Collaborator

Closed by #149

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

No branches or pull requests

4 participants