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

Change private to protected to allow override #7

Closed
wants to merge 1 commit into from

Conversation

smalot
Copy link

@smalot smalot commented Dec 24, 2018

The getWhoopsInstance method is very useful because it is run for each request, but in private scope, it is too limited.

@oscarotero
Copy link
Member

Hi. This brings up a well-know discussion about private / protected methods and properties by default (https://www.reddit.com/r/PHP/comments/3jdbkd/protected_or_private_as_a_default_visibility_and/)

I'm inclined to use always private unless there're good reasons to extend and modify a method behavior. What's the use case to change the visibility to all these methods and properties? The getWhoopsInstance is just a method to return a default Whoops\Run if no one is passed to the constructor.

@smalot
Copy link
Author

smalot commented Dec 30, 2018

Hi. I'm using your middleware in a Swoole http server app.
So your getWhoopsInstance run multiple times because php is always running. So a new Whoops\Run is returned for each Request and depends on its content type.
Using the whoops instance from the constructor, will use still the same one.

With a protected method, it will allow to override the behavior and allow to create a new instance for each request.

@smalot
Copy link
Author

smalot commented Dec 30, 2018

To note that the WhoopsHandlerContainer contains only protected method

@oscarotero
Copy link
Member

I see.
I'm not clear about what do you want: run a new Whoops\Run instance for each request or use always the same instance? Or maybe use the same instance for each content-type response? Maybe we can change only getWhoopsInstance method to protected (but keep the rest of methods and properties private). So you can do something like the following:

protected function getWhoopsInstance(ServerRequestInterface $request): Run
{
    $key = $request->getHeaderLine('Accept');
    if (empty($this->runInstances[$key])) {
        $this->runInstances[$key] = parent::getWhoopsInstance($request);
    }

    return $this->runInstances[$key];
}

What do you think?

To note that the WhoopsHandlerContainer contains only protected method

Yes, that's right but this is a more opinionated and more likely to want to change or customize.

oscarotero added a commit that referenced this pull request Nov 30, 2019
@oscarotero
Copy link
Member

Close this. The new version has the getWhoopsInstancemethod protected

@oscarotero oscarotero closed this Nov 30, 2019
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.

2 participants