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

Allow array as presenter mapping #101

Closed
enumag opened this issue Oct 14, 2015 · 27 comments
Closed

Allow array as presenter mapping #101

enumag opened this issue Oct 14, 2015 · 27 comments
Milestone

Comments

@enumag
Copy link
Contributor

@enumag enumag commented Oct 14, 2015

The PresenterFactory splits the mapping mask to 3 parts - a prefix, a part which is reapeated for each module and a part for presenter. The problem is that there is no way to actually configure the 3 parts separately. In my case I need the mapping to look like this:

'*' => array('App\', 'Module\*Module\', 'Presenter\*Presenter')

This is currently not possible to achieve. I tried the following:

nette.application:
    mapping:
        *: App\Module\*Module\Presenter\*Presenter
# result:
# '*' => array('App\Module\', '*Module\', 'Presenter\*Presenter')
nette.application:
    mapping:
        *: App\*Module\Module\Presenter\*Presenter
# result:
# '*' => array('App\', '*Module\', 'Module\Presenter\*Presenter')

To achieve the mapping I want I'd have to create my own PresenterFactory. And I can't even extend the default one because the mapping property is private (don't want to hack it using reflection).

Instead I'd like the PresenterFactory to accept an array as well so that this would work:

nette.application:
    mapping:
        *: [ App\, Module\*Module\, Presenter\*Presenter ]
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

@JanTvrdik JanTvrdik commented Oct 15, 2015

👍

@Majkl578

This comment has been minimized.

Copy link
Contributor

@Majkl578 Majkl578 commented Oct 15, 2015

👎 It's even more obscure than current syntax. :(

@fprochazka

This comment has been minimized.

Copy link
Contributor

@fprochazka fprochazka commented Oct 16, 2015

I think it should be objects, then you would register

nette.application:
    mapping:
        *: Nette\Application\DefaultPresenterMapping("App\\", "Module\\*Module\\", "Presenter\\*Presenter")
        Custom: My\Custom\PresenterMapping()
        VeryCustom: @My\Custom\PresenterMapping() # find service in container

ofcourse, the default syntax could replace string for the Nette\Application\DefaultPresenterMapping due to compatibility, so you'd get

nette.application:
    mapping:
        *: App\Module\*Module\Presenter\*Presenter # interpreted as Nette\Application\DefaultPresenterMapping("App\Module\*Module\Presenter\*Presenter")
        Custom: My\Custom\PresenterMapping()
        VeryCustom: @My\Custom\PresenterMapping() # find service in container

and ofcourse, you'd have to implement an interface

class IPresenterMapping {
    public function formatPresenterClass($presenter);
    public function unformatPresenterClass($class);
}

Then PresenterFactory's only job would be to find the namespace in array and call the method.

@enumag

This comment has been minimized.

Copy link
Contributor Author

@enumag enumag commented Oct 16, 2015

@fprochazka Like! :-)

@lookyman

This comment has been minimized.

Copy link

@lookyman lookyman commented Oct 16, 2015

@fprochazka That is beautiful 👍

@Unlink

This comment has been minimized.

Copy link

@Unlink Unlink commented Oct 16, 2015

👍

@fprochazka

This comment has been minimized.

Copy link
Contributor

@fprochazka fprochazka commented Oct 17, 2015

Who's gonna implement it guys? :)

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 20, 2016

@enumag maybe something like this is more readable than array:

nette.application:
    mapping:
        *: App\[Module\*Module]\Presenter\*Presenter

@fprochazka is not it overengeneered?

@enumag

This comment has been minimized.

Copy link
Contributor Author

@enumag enumag commented Jan 20, 2016

@dg It's not bad but I slightly prefer array. In most cases I like to keep things simple instead of inventing new syntaxes.

@fprochazka

This comment has been minimized.

Copy link
Contributor

@fprochazka fprochazka commented Jan 20, 2016

@dg I don't think so, because in the current way, you have to invent "new magic syntaxes" to add mapping options. But "my way", you can just code it in plain PHP and mainly you can easily write a unittest for it, without having to pipe it through PresenterFactory first.

@hrach

This comment has been minimized.

Copy link
Contributor

@hrach hrach commented Jan 20, 2016

@dg: please, do not create new metalanguages. I'd like more the verbose php solution, than something hidden in the strings. It's much more difficult to do tools which should support it (intellij-neon plugin), etc.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 20, 2016

It is not metalanguage, but domain-specific language.

@hrach

This comment has been minimized.

Copy link
Contributor

@hrach hrach commented Jan 20, 2016

@dg: please, do not create new domain-specific language. Reasons above.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 20, 2016

It already exists. App\Module\*Module\Presenter\*Presenter.

@hrach

This comment has been minimized.

Copy link
Contributor

@hrach hrach commented Jan 20, 2016

I don't claim I like it, I wanted to point out I would like it wouldn't get any new features.

@fprochazka

This comment has been minimized.

Copy link
Contributor

@fprochazka fprochazka commented Jan 20, 2016

@dg well yeah, but it's way too magic (for beginners) imho. I have to study the code every time I start a new project. Mostly changing the first namespace is enough but anything more complex and you have to look at the code or directly extend the PresenterFactory to override it, as it's often simpler than try to hack the domain-specific language.

I'm curious, what is your experience with this from trainings and consultations?

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 20, 2016

On training I am using this infographic and I think that people understand:

2016-01-20_14-01-25

but anything more complex and you have to look at the code or directly extend the PresenterFactory to override it,

In what cases?

@fprochazka

This comment has been minimized.

Copy link
Contributor

@fprochazka fprochazka commented Jan 20, 2016

@dg basically anything that involves the *, adding and removing the module from the mask for example - I always have to check that how it works. Not that I would start project that often.

@petrhejna

This comment has been minimized.

Copy link

@petrhejna petrhejna commented Feb 14, 2016

I started thinking about implementation. Is this the behaviour you need? Do I understand it correctly?

test(function () {
    $factory = new PresenterFactory;
    $factory->setMapping([
        '*' => ['App\\', 'Module\\*Module\\', '\Presenter\\*Presenter'],
    ]);
    Assert::same('App\Module\Jupiter\Presenter\RedDwarf', $factory->formatPresenterClass('Jupiter:RedDwarf'));
    Assert::same('App\Module\Universe\Module\Jupiter\Presenter\RedDwarf', $factory->formatPresenterClass('Universe:Jupiter:RedDwarf'));
});

test(function () {
    $factory = new PresenterFactory;
    $factory->setMapping([
        '*' => ['App\\', '*Module\\', '*Presenter'],
    ]);
    Assert::same('App\Universe\Jupiter\RedDwarf', $factory->formatPresenterClass('Universe:Jupiter:RedDwarf'));
});
@enumag

This comment has been minimized.

Copy link
Contributor Author

@enumag enumag commented Feb 14, 2016

@Achse Actually no. Here is what I want.

test(function () {
    $factory = new PresenterFactory;
    $factory->setMapping([
        '*' => ['App\\', 'Module\\*Module\\', '\Presenter\\*Presenter'],
    ]);
    Assert::same('App\Module\JupiterModule\Presenter\RedDwarfPresenter', $factory->formatPresenterClass('Jupiter:RedDwarf'));
    Assert::same('App\Module\UniverseModule\Module\JupiterModule\Presenter\RedDwarfPresenter', $factory->formatPresenterClass('Universe:Jupiter:RedDwarf'));
});

And it works with the current implementation of PresenterFactory. The only problem is that I have to hack the mapping array using reflection.

@petrhejna

This comment has been minimized.

Copy link

@petrhejna petrhejna commented Feb 15, 2016

Ok, there is a proof of concept solution (I moved from red to greeen). Plase give me some feedback if I didn't miss any case or if there any missing functionality.

Then I'll do some code cleaning & refactoring. I would like to extract the logic of parsing into some "PresenterMappingParser".

@foxycode

This comment has been minimized.

Copy link
Contributor

@foxycode foxycode commented Mar 15, 2016

I like @fprochazka solution best. And implementation would be much more simplier than in PR #119

@fprochazka

This comment has been minimized.

Copy link
Contributor

@fprochazka fprochazka commented Mar 15, 2016

I'd like to ask a question:

How much do you have to do, to write a test for the mapping configuration with current syntax? VS How much you'd have to do, to unittest a solution, where the mapping logic would be separated from PresenterFactory?

In terms of infrastructure and ensuring that what you've tested is in fact what the app is using... will you be loading the config and getting the presenter factory from DI Container? Will you be copying the mask from config to unittest? You'll also have to mock arguments of PresenterFactory. Etc, etc...

Imho the way I've proposed is cleaner and promotes testability.

@petrhejna

This comment has been minimized.

Copy link

@petrhejna petrhejna commented Mar 15, 2016

I assume discusion here have nothing to do with my PR and it's still about "@gd -like" syntax versus. arrays as @fprochazka proposed.

When you came with conclusion I can rewrite my PR. With current logic, I tried to be able to do it both ways, but the complexity of that is imho awful.

@xificurk

This comment has been minimized.

Copy link
Contributor

@xificurk xificurk commented Mar 15, 2016

I've been testing for a while now something like this: https://gist.github.com/xificurk/3eb69d7f3f7dd44abbcd
It works well for my needs, but I'm still not completely satisfied with the current code.

My main motivation was that the default presenter mapping in Nette is pretty "dumb", here are two problems I ran into:

  1. Default mapping works only for top-level modules, but not submodules independently. I bundle presenters to corresponding composer packages and I would like to reference them as presenters inside submodule 'Vendor:Package'. To make this work with default mapping I would need to have exactly same namespace structure for every package (which is not always practical) and CompilerExtension in every package would need to set the global mapping for 'Vendor' module (which is ugly).
  2. For some applications I need to do a minor customization of the installed presenter, i.e. I would like to swap Vendor\Package\FooPresenter for e.g. App\FooPresenter without breaking links etc. This can't be done with default mapping at all.
@enumag

This comment has been minimized.

Copy link
Contributor Author

@enumag enumag commented Apr 3, 2016

@Achse Thanks for solving it!

@dg Is it possible to have this in 2.3 as well? There is no BC break so it should be fine.

dg added a commit that referenced this issue Apr 3, 2016
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Apr 3, 2016

@enumag done

dg added a commit that referenced this issue Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.