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

Is it possible to change private visibility to protected? #3125

Open
solventt opened this issue Oct 28, 2021 · 7 comments
Open

Is it possible to change private visibility to protected? #3125

solventt opened this issue Oct 28, 2021 · 7 comments
Labels

Comments

@solventt
Copy link
Contributor

When I launch functional tests I don't need exception trace log output because Its huge. I want to see only an exception type, code, message, file and line. So I decided to override the formatExceptionFragment method (remove 4 strings forming trace output) of PlainTextErrorRenderer. But the method is private and called from the __invoke method. And so I cant override formatExceptionFragment in child class - to do it - I should also copy __invoke to child class. But then there is no point in inheritance either - you get a separate class, almost identical to PlainTextErrorRenderer, except for 4 lines.

It seems redundant to me to copy almost all the code from PlainTextErrorRenderer just to remove the 4 lines of code responsible for the trace output. Is it possible to change private visibility to protected of the formatExceptionFragment method?

@zelding
Copy link

zelding commented Oct 28, 2021

I think you should create your own error renderer;
Implement the Slim\Interfaces\ErrorRendererInterface, and use that during tests

@l0gicgate
Copy link
Member

I would be open to making those methods protected for extensibility. I don't have a strong opinion on that @solventt

@odan
Copy link

odan commented Nov 2, 2021

I think these methods should not be set to protected, otherwise we have to deal with unexpected breaking changes later on.
A custom error handler would be a better approach (as mentioned by zelding).

@l0gicgate
Copy link
Member

@odan considering Slim 4 is in a pretty stable place right now changing the method modifier to protected wouldn't be the end of the world. If we were at the beginning of a release cycle I may have a different opinion on that. I don't foresee us having to introduce breaking changes to those renderers. While not impossible that we eventually break things, letting users extend these renderers at their own risk would be fine with me.

@solventt
Copy link
Contributor Author

solventt commented Nov 3, 2021

@zelding Of course I did. I wrote above that now I have almost a copy of existing PlainTextErrorRenderer. I only suggested it.

@eerison
Copy link

eerison commented Dec 2, 2023

@odan considering Slim 4 is in a pretty stable place right now changing the method modifier to protected wouldn't be the end of the world. If we were at the beginning of a release cycle I may have a different opinion on that. I don't foresee us having to introduce breaking changes to those renderers. While not impossible that we eventually break things, letting users extend these renderers at their own risk would be fine with me.

I totally agree with @odan , if you think in keep this project up to date, then don't let users extend your code.

If someone wants to change the behavior, then implementing the interface and inject the new one.

And if he want to reuse some code that exists, then he can just decorate the class and inject.

I faced this problem on other OSS projects and we need to keep an code that must be compatible with 2 ways. Then IMHO don't do this.

@eerison
Copy link

eerison commented Dec 2, 2023

@zelding Of course I did. I wrote above that now I have almost a copy of existing PlainTextErrorRenderer. I only suggested it.

Did you try to decorate this render?

YourPlainTextErrorRender implement RenderInterface
construct(PlainTextErrorRender $render)

Public function medthodA()
This->render->methodA();
/// your change

Note 1 : sorry for the ugly example, I'm on phone

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

No branches or pull requests

5 participants