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

Increase method visibility #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

skyosev
Copy link

@skyosev skyosev commented Jul 14, 2015

I really don't see a reason why the method of this class should be private. There are some realistic cases for extending the class and overriding some functionalities. For example I would want to throw events at some points from the dispatch cycle. The only solutions for me are to replace the whole class (pretty ugly) or modify the source code directly (renders composer useless).

Best Regards

I really don't see a reason why the method of this class should be private. There are some realistic cases for extending the class and overriding some functionalities. For example I would want to throw events at some points from the dispatch cycle. The only solutions for me are to replace the whole class (pretty ugly) or modify the source code directly (renders composer useless). 

Best Regards
@mrjgreen
Copy link
Owner

Hi! Thanks for the feedback. I could make this protected. I'll think about it over the weekend. I feel the correct solution is to break this part of the class out into a handler object which can be injected in. Then you could write your own handler which implements a handler interface, instead of using inheritance. That may be a breaking change so I will look at trying to do that for the next version.

https://github.com/mrjgreen/phroute/tree/handler/src/Phroute

I actually have a partially complete branch which attempts to do this, but I had to make too many BC breaks to make it work. I'll try and find a way of doing this thats a little less destructive :)

@frederikbosch
Copy link
Contributor

@skyosev Why don't you use composition inside your code? So you inject the Phroute Dispatcher into your own created Dispatcher? And then you decorate the Phroute dispatcher with your own methods!

@kamalkhan
Copy link

Hi, could you also pass the variables ($var) onto the resolver resolve method? Right now I will have to fork the project and modify the Dispatcher class to send the $var as the second parameter to my resolver method. As @skyosev has mentioned about the visibility, I wouldn't have asked if the class had protected methods instead of private.

Thanks!

@W1zzardTPU
Copy link

+1 on merging this to avoid forking the whole project

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

Successfully merging this pull request may close these issues.

5 participants