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

DefaultMapper: added $defaultEntityNamespace to constructor #113

Open
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@janpecha
Collaborator

janpecha commented Aug 29, 2017

Umožňuje předat $defaultEntityNamespace přímo do konstruktoru, takže odpadá nutnost dědit, jen abychom změnili výchozí namespace.

$mapper = new LeanMapper\DefaultMapper('App\Model\Entities`);

Změna by měla být plně zpětně kompatibilní (doufám), viz test. Pak jsem ještě opravil anotaci u property ($defaultEntityNamespace může být null).

public function __construct($defaultEntityNamespace = 'Model\Entity')
{
if ($this->defaultEntityNamespace === 'Model\Entity') { // because BC
$this->defaultEntityNamespace = $defaultEntityNamespace;

This comment has been minimized.

@castamir

castamir Aug 29, 2017

Collaborator

Co že má tohle dělat?

This comment has been minimized.

@janpecha

janpecha Aug 30, 2017

Collaborator

Potřebujeme zajistit, aby to bylo kompatibilní s tímto kódem:

class MyMapper extends LeanMapper\DefaultMapper
{
    protected $defaultEntityNamespace = 'App\Entities';
}

Kdyby tam ta podmínka nebyla, přepíše konstruktor uživatelem nastavenou hodnotu property. Ta podmínka by tedy měla zaručit to, že k přepsání dojde jen v případě, že nebyla property změněna na jinou hodnotu.

Alternativně by šlo asi použít i následující kód:

public function __construct($defaultEntityNamespace = FALSE) {
    $this->defaultEntityNamespace = $defaultEntityNamespace !== FALSE ? $defaultEntityNamespace : $this->defaultEntityNamespace;
}

Ale oboje mi přijde podobně špatné :)

@castamir

This comment has been minimized.

Collaborator

castamir commented Aug 29, 2017

Jinak nevím, nevidím tam moc přínos - dědičnost mi v tomto případě přijde lepší

@janpecha

This comment has been minimized.

Collaborator

janpecha commented Aug 30, 2017

Pokud někdo používá implicitní filtry nebo single table inheritance, tak to skutečně moc přínos nemá, protože tak jako tak musí ten mapper podědit, nebo si napsat vlastní.

V ostatních případech - kdy člověk nepotřebuje mapper dědit - to však smysl má. Třeba já na projektech používám stále stejný mapper, u kterého jen potřebuji změnit namespace pro entity podle konkrétního projektu. V konfiguračním souboru tedy napíšu:

leanmapper:
    defaultEntityNamespace: App\Model\Entities

A tahle hodnota se mi předá přes konstruktor do mapperu a nic víc nemusím řešit.

@castamir

This comment has been minimized.

Collaborator

castamir commented Aug 30, 2017

Já používám vlastní extension - je to součástí balíčku s celou řadou drobných rozšíření (Query Object, automatická registrace repozitářů, specifické mapování entit do různých namespace, včetně zanořených atp)

https://github.com/Joseki/LeanMapper-extension/blob/master/src/Joseki/LeanMapper/DI/Extension.php

@janpecha

This comment has been minimized.

Collaborator

janpecha commented Aug 30, 2017

Taky nad tím mám ještě extension pro Nette, to v tomto případě ale nehraje příliš roli. Pokud projekt od počátku dodržuje stanovené konvence, tak statický mapper v mnoha případech úplně stačí a jediné co je potřeba, je změnit globálně výchozí namespace. A to by IMHO mělo být možné i bez dědičnosti.

Samozřejmě záleží na konkrétním projektu - jsou projekty, kdy statický mapper dostačuje, jsou projekty kdy ne a je potřeba sáhnout po něčem složitějším.

@janpecha janpecha added the RFC label Apr 14, 2018

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

@janpecha janpecha modified the milestones: Version 3.3.0, Version 3.4.0 May 6, 2018

@janpecha janpecha modified the milestones: Version 3.4.0, Version 4.0.0 Aug 11, 2018

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