flame-org / Modules

Nette modules on the Steroids
http://flame-org.github.io/Modules/
GNU Lesser General Public License v3.0
13 stars 7 forks source link

Enable services in route definitions #16

Closed TomasVotruba closed 9 years ago

TomasVotruba commented 10 years ago

Solves situation with filterIn/Out.

Big pron is that syntax is the same as in default RouterFactory.

I'm using this and it work properly.


Next step (not now) and more clear would be adding tag to the service (viz @matej21).

$router = $builder->getDefinition('router');
foreach(array_keys($builder->findByTag(...)) as $name) {
    $router->addSetup('offsetSet(?, ?)', array(NULL, '@' . $name));
}

What do you think?

jsifalda commented 10 years ago

Did you try whether the demo app is working with your patch? I am getting error Unable to create service 'routerServiceFactory.d6c5a2692a2df4bdf156a42fc8251c13', value returned by factory is not Nette\Application\IRouter type.. Can you check it please? Or what should I do to correct working?

TomasVotruba commented 10 years ago

I'll check it, didn't have much time today.

jsifalda commented 10 years ago

Great, thank you!

TomasVotruba commented 10 years ago

Try it now.

If it still doesn't work, could you share composer.json for demo?

TomasVotruba commented 10 years ago

I just added commit that enabled loading routers by tag.

I consider it best practice, since you can use same format as in sandbox router.

To add service to routes, just add a tag (config.neon):

services:
    -
        class: App\CoreModule\Routing\RouterFactory
        tags: [flame.modules.router]
jsifalda commented 10 years ago

Well, now it's working after I put the semicolon into file AppExtension.php on line 26 in demo ;)

A definitely want merge improvements which you committed (I really love tagging services!). However, I want to keep routes mock in modules as well. Can you prepare another pull request which would solve this issue please? Thank you again!

TomasVotruba commented 10 years ago

Thanks! Tagging is the way for service approach :).

I am not really sure what and where exactly you want to keep. If it is not much work for you, maybe we could skip explanation and you could merge this and apply fixes you need. Would that be ok for you?

jsifalda commented 10 years ago

Ok, I will do it;)

TomasVotruba commented 9 years ago

@jsifalda Ping, it starts to being rotten. Do you need help with anything to merge this?

I would like to use in multiple project and I don't want to extend composer by my repo vcs :)

jsifalda commented 9 years ago

@TomasVotruba I am sorry for my delay.

The problem is I am not sure if is it working... Can you add example of usage features in this pull into demo?

The routes definition in AppExtension contain syntax error (as mentioned before). The problem is the routes definition in the Extension is not working at all.

What I can skip, but I don't like it, is using of [] intend of array() ;)

And last thing please persist implementation of NetteRouteMocks. I am not sure how is it working and I don't have enough time to became more familiar with it. :)

Thanks a lot!

TomasVotruba commented 9 years ago

I understand your concerns, thanks for heads up!

I'll ping when I'm ready with this.

jsifalda commented 9 years ago

Yes, of course, tests would be great! But example in demo is mostly for people who want to see how to use it :) Thank you again!

TomasVotruba commented 9 years ago

How to use it is already here, isn't it? http://flame-org.github.io/Modules/

jsifalda commented 9 years ago

That is true! Well, I will add example to http://flame-org.github.io/Modules/ :)

TomasVotruba commented 9 years ago

Test's were added, also for PHP 5.3, 5.5 and 5.6 I fixed macro test and tracy tests.

All works: https://travis-ci.org/TomasVotruba/Modules/builds/34099057

Two more suggestion before merge:

1) Move tests in demo test and remove demo folder. It really has no use when all is explained on page and in tests.

2) Also, do you want to support Nette cs? There is a lot of mess in the code (files are very different) and it would make it more readable for other programmers.

TomasVotruba commented 9 years ago

Thank you!

jsifalda commented 9 years ago

Thanks a lot for great job!;)

TomasVotruba commented 9 years ago

I'm glad you are open to changes. Now this package has everything I need :)

jsifalda commented 9 years ago

be sure!;) Great to hear you will be using the package!