PresenterFactory as callback & changed invalid link handling [WIP]

[dg]

Two different things.

1. replaced PresenterFactory dependency on DI with simple callback.

• dependency on DIC is bad (it is the only one dependency on DIC in whole framework)
• current PresenterFactory constructor is different in 2.2 and 2.3, so there will still be BC break
• partially solves Added presenter mapper #16

2. new invalidLinkMode

• handling of invalid links is not insufficient, we need warnings
• I am not happy with InvalidLinkMode: changed invalid link handling #65, where (for partial combatibility) is mode that triggers warnings named INVALID_LINK_SILENT

[fprochazka]

A lot of people are inheriting the class and this might make an unnecesary BC Break. It would be imho better to add sane default value to the property and create setter, just like @klimesf had it.

[klimesf]

👍

[stekycz]

Typehint for $container?

What does $touchToRebuild mean? What should be the value? A comment would be nice.

[dg]

I added typehint.

[hrach]

• first commit contains changes with invalid link hadling, probably bug
• at least invalid link handling should be postponed to 2.4

---

PresenterFactory:

• current proposed design doesn't make sense at all,
• there is nothing wrong to have a dependency on DIC, if there is a interface and possibility for own replacement.
• create Factory, which in fact deosn't create a instance, is wrong. Passing logic through clousure is wrong. In the end, the dependency on the DIC is still present, in static Helper.
• Corrent solution should be:
  • provide default implementation without DIC.
  • provide DIC implementation in bridges namespace. (edit: which will be automatically injected into DIC...)

[fprochazka]

I like the closure, but I don't like that it's static class, when it doesn't have to be static class.

you could have created

class PresenterFactoryCallback
{
    public function __construct(/* deps */);
    public function __invoke($class);
}

and attach it like

$presenterFactory = $container->addDefinition($this->prefix('presenterFactory'))
    ->setFactory('Nette\Application\PresenterFactory', [
        new Nette\DI\Statement('Nette\Bridges\ApplicationDI\PresenterFactoryCallback')
    ])

IMHO a lot nicer, what do you think?

[dg]

Yes, it is fine. I take this more as a cosmetic detail, which can be changed at any time.

[dg]

@hrach I see no changes in invalid link handling in first commit.

This can be of course postponed to 2.4, but it seems to me unnecessary to wait a year before invalid links will throw warnings.

ad design: to create interface & its implementation where can be used simple callback is possible, but over complicated.

[fprochazka]

I'm 👍 for warnings on invalid links. They don't kill production and I wanna know about them.

[hrach]

@dg sorry, I missread.

ad design: to create interface & its implementation where can be used simple callback is possible, but over complicated.

That's more Filip's proposal than mine. I see no reason to create another interface, when one is present. It's IPresenterFactory. It's totally ok to have one imlementaion dependent on DIC:

[dg]

It's totally ok to have one imlementaion dependent on DIC:

I agree, the motivation was to get rid of changes in constructor, #66 (comment)