OO: Change private to protected

[guilhermeaiolfi]

Hi,

why most of properties and methods in zend-expressive are private? It makes it really difficult to extend without copying the whole class. That's against the OO best practices recommendation.

I purpose making all properties and methods that are private today, protected.

I can send a pull request if that's acceptable.

Thanks

[geerteltink]

Properties and methods are private so they can be changed without breaking compatibility. If every method was public or protected we probably would need to release a new major version every few months because of BC-breaks.

I don't know why and what class you are extending but that's the exact use case for private methods and properties. We can change the code without you noticing it and your code still works.

But if there is a good reason to make something public then it can be changed or a new public method can be added so you can access what you need. It has been done in the past.

[guilhermeaiolfi]

If they were public I would agree. But protected are used to scope them to the class but allow extending the class. The way they are today means that every class in zend-expressive are considered final. So I can't extend its functionality or change its behavior to what I want to do. What I want to do is irrelevant, the importance here is be able to extend. Consider this:

if I want to make Application class do something else, I need to copy the whole class and change the bits I want. If it were protected I would change only what was different and leave all the rest in the parent class. When you, developers of zend-expressive, change this class in the future, even if that breaks compatibility, it's much easier for me to see what was affected if I have overwritten some methods than looking and comparing the whole class.

[Ocramius]

If we change a class and it breaks for you, this is a BC break and needs to be reverted.

Please also read http://ocramius.github.io/blog/when-to-declare-classes-final/

We provide composition endpoints all over the framework: inheritance is mostly not needed at all, even when extending framework functionality.

On 28 Jul 2017 3:21 PM, "Guilherme Aiolfi" notifications@github.com wrote:

If they were public I would agree. But protected are used to scope them to the class but allow extending the class. The way they are today means that every class in zend-expressive are considered final. So I can't extend its functionality or change its behavior to what I want to do. What I want to do is irrelevant, the importance here is be able to extend. Consider this:

if I want to make Application class do something else, I need to copy the whole class and change the bits I want. If it were protected I would change only what was different and leave all the rest in the parent class. When you, developers of zend-expressive, change this class in the future, even if that breaks compatibility, it's much easier for me to see what was affected if I have overwritten some methods than looking and comparing the whole class.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-expressive/issues/501#issuecomment-318650073, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakJVHEubIIJoNK0FExW0pdI6z-rAqks5sSeBBgaJpZM4Omgiq .

[guilhermeaiolfi]

First I though you were joking. Then I saw the benefit behind the idea and went to implement my changes the way you were suggesting.

Some considerations:

1) I have 3 layers of abstraction for my Router class now: MyRouter -> AuraRouter -> RouterContainer. Layers of abstraction add complexities. Worst, RouterContainer has a basepath config option (passed in the __construct), it is hidden in the class, so I had to duplicated the config in the layers above. In the end I have to configure 3 routers to use my own. I can't abstract the creation and configuration of that class because I have to respect the layers below. So anyone who wants to use my class, now needs to go to the docs and see that it needs to instantiate RouterContainer, pass that to AuraRouter and then create MyRouter and pass AuraRouter to it. If that developer wants to change something, he'll add another layer and things start to smell.

2) When extending, you change what you want and leave all the rest. For MyRouter I had to implement 2 other proxy methods to respect the RouterInterface. RouterInterface has only 3 methods, but for other interfaces that can add a shitload of dummy code. Adding an unnecessary layer of abstraction and making things slower (maybe).

3) Some cases will end up being all copy and paste like someone commented in your article. That's the worst scenario for me and that's something that should be avoid. That's simply not acceptable. I can't accept that as a reasonable approach to creating code.

All considered, I think it's should be up to the developer to choose what he thinks is the best way. If you want to use composition, you still can without it being final or method/properties being protected. But the other way around is not true, I can't use inheritance the way things are today.

But I guess it's up to you to decided. I would like to have the choice.

Thanks.

[geerteltink]

Yes that router interface has 3 public methods. That's all that's needed to make it work. You can extend a router and overwrite those 3 public methods. Actually all the code could have been in just those 3 public methods. But to make the code maintainable and easy testable it is extracted into smaller private methods. So where a developer places a code doesn't matter much. It can be all in one public function with 100 lines of code or it can be in 1 public and 9 private functions. But at the end it has to do what that public method in that interface tells you what it should be doing.

Some cases will end up being all copy and paste like someone commented in your article. That's the worst scenario for me and that's something that should be avoid. That's simply not acceptable. I can't accept that as a reasonable approach to creating code.

I agree with that you could end up copy pasting the entire class. I don't agree with that it should be avoided. The zend-expressive-aurarouter package is created with a purpose and it does just that. How it does that internally doesn't matter, as long as it implements the RouterInterface. We can change or remove private methods if needed, optimize code or rewrite the whole class and you won't notice it when you update that package since it's all done internally in private methods.

I guess the question is why would you want to extend the AuraRouter class? At least I think that's what you are doing with MyRouter. Is the original class lacking functionality? You can add and create routes, generate URLs. Or you can even get directly all registered routes with Application::getRoutes(). The RouteResult is injected into the Request object. I'm not sure what else you need from the router.

[guilhermeaiolfi]

I guess the question is why would you want to extend the AuraRouter class? At least I think that's what you are doing with MyRouter. Is the original class lacking functionality? You can add and create routes, generate URLs. Or you can even get directly all registered routes with Application::getRoutes(). The RouteResult is injected into the Request object. I'm not sure what else you need from the router.

I think that's the mindset around the way expressive is implementing using that approach @Ocramius advocates: "What else do you need from what we're offereing?" instead of "How can we alieviate the pain from what you are trying to do?".

Like I said ealier, what I want to do is irrelevante. But if you need to know, it's not just the Router I'm changing. If you are following the discussions I'm starting, it's all based in the work I'm doing now: essencialy I'm trying to replicate the bits that Agavi have and are not present in zend-expressive. Zend-expressive IS more modern and future proof. But it's very basic, if you think that the expressive router does everything possible and I shouldn't be complaining, look the AgaviRouting class, just the parts I'm trying to implement now:

1) routing callbacks: so ->generateUri('my.route', { id: 1 }), could, for example, based on the id go to the database, get the slug for it, and use that for the URL;

2) path cuts: remove a path pattern and set an attribute based on that pattern and keep looking for another match... so things like /json and and /xml in a URL would influence the way we deliver the view down the pipe. That could be done in a middleware, except I need to be able to revert that rule and create the appropriate URL in my views, so it needs to be a router rule and not a middleware.

Always trying to keep the API closer to Agavi than expressive so the migration will be easier.

You may argue that it's not what expressive is for. But although I'm indeed changing some fundamentals parts of zend-expressive, it is already making my life easier. I'm grateful for that and for the work you had put into it. I was just arguing how it could be even more helpful.

The path I'm taking I think l'll end up using just the low level libraries but it shouldn't prevent me from using it before I really needed.

Anyways, I'm closing this since now I know it's not a oversight/bug and it was designed that way on purpose. Thank you for taking the time to explain that.

[geerteltink]

"What else do you need from what we're offereing?" instead of "How can we alieviate the pain from what you are trying to do?".

I actually asked what you are trying to do so we could help you or even add features so it makes it easier what you are trying to do. I'm sorry if I didn't express myself right or you understood otherwise.

I'm trying to replicate the bits that Agavi have and are not present in zend-expressive. routing callbacks: so ->generateUri('my.route', { id: 1 }), could, for example, based on the id go to the database, get the slug for it, and use that for the URL;

You are not trying to extend the framework, you are trying to change its inner workings. In my opinion (doesn't mean that has to be the expressive way) the router should generate routes. It shouldn't know anything about the database. Since you have the entity id, you can get the entity in your action or middleware and inject that into the template renderer. In there you call: $this->url('blog.post.view', ['id' => $post->getId(), 'slug' => $post->getSlug()]) The URL is nicely generated by the router without knowing anything about the domain.

Another solution could be writing your own view/template helper/extension. It would grab all the data from the database and feed it to the router so it has all the data it needs to generate the url. That way you mimic the old framework but don't change the expressive router.

path cuts: remove a path pattern and set an attribute based on that pattern and keep looking for another match... so things like /json and and /xml in a URL would influence the way we deliver the view down the pipe. That could be done in a middleware, except I need to be able to revert that rule and create the appropriate URL in my views, so it needs to be a router rule and not a middleware.

At the point where you render the view your app should know about what template to render. Since html, json and xml are complete different formats, I would choose different templates: blog.post.index.html, blog.post.index.xml. Again, this is how I did this, not how Expressive forces you how to do it.

Always trying to keep the API closer to Agavi than expressive so the migration will be easier.

I've been in that situation. It doesn't matter what underlying framework you choose (expressive, slim, zend-mvc, symfony), they all work in their own way and changing how they do things internally means extending and rewriting code. Rewriting the code might be easier. And changing the inner workings might seem the best way right now, but think about the future and a possible expressive version 3. Since you probably changed so much code, you need to go through the same process again.

I've found out that there are better ways to migrate. I ended up rewriting the code in stages (one module at a time). What helped me is that you can actually wrap your old application inside expressive and whatever expressive couldn't handle yet, was forwarded to the old application.

So will it be worth it to change how Expressive works and port functionality from another framework to make migration easier, or should you embrace a new framework and accept how it works and rewrite your code? That's a question only you can answer for yourself.

[guilhermeaiolfi]

In my opinion (doesn't mean that has to be the expressive way) the router should generate routes.

You missed a "not" there? Because If you have not, I agree with you. One think that I did was to make the class stateful. Actually, I created I Context class that hold the important things and I get the information I need from there in the Router or anywhere else. So I eliminated two Helpers, two middlewares for those helpers. Justt one method handle all the URL stuff. If I want an absolute URL I just call ->gen("route.name", $params, [ "absolute" => true ]);. I'm using sub-requests and I haven't seen any case that it would bite me doing that way.

Rewriting the code might be easier. And changing the inner workings might seem the best way right now, but think about the future and a possible expressive version 3. Since you probably changed so much code, you need to go through the same process again.

Agree 100%. I'm being carefully what I'm porting and embracing new concepts where I see fit.

So will it be worth it to change how Expressive works and port functionality from another framework to make migration easier, or should you embrace a new framework and accept how it works and rewrite your code? That's a question only you can answer for yourself.

There are a lot of things I won't migrate because I think the expressive approach is more efficiente/simple/better. For example: config manager, error handler, DI. I'm only borrow what I think is better in the Agavi land, keeping the API as close as possible is just a side effect actually.

[Ocramius]

Jumping in to add one detail, as I didn't read the whole wall of text above.

We used to have everything protected or extensible in general, and it was (and still is, for some APIs in ZF3) a big mess that nobody really can fix.

APIs should be made extensible only where explicitly designed for inheritance. Designing for inheritance is extremely hard, as inheritance is one of the strictest types of coupling available.

[piotrek-r]

@guilhermeaiolfi Please take a look at what Dependency inversion principle is about. It's part of SOLID principles.