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

Does debug extension need the controller? #3563

Closed
SeriousKen opened this issue May 29, 2018 · 7 comments
Closed

Does debug extension need the controller? #3563

SeriousKen opened this issue May 29, 2018 · 7 comments

Comments

@SeriousKen
Copy link

/**
* Creates the extension instance.
* @param \Cms\Classes\Controller $controller The CMS controller object.
*/
public function __construct(Controller $controller)
{
$this->controller = $controller;
}

The debug extension requires the controller in its constructor but doesn't reference it anywhere. Should this be refactored out?

@LukeTowers
Copy link
Contributor

@SeriousKen is it causing any problems to have it there?

@SeriousKen
Copy link
Author

SeriousKen commented May 30, 2018

@LukeTowers It's not causing a problem as such, but I noticed it when looking into this issue in the Pages plugin. The Twig environment is newed up in the CmsCompoundObject without the DebugExtension, but you can't new up the DebugExtension without a controller instance, which the CmsCompoundObject doesn't have access to.

Also, in the interests of clean code, anything that is redundant should be removed.

@Samuell1
Copy link
Member

Samuell1 commented Aug 7, 2019

@LukeTowers

@bennothommo
Copy link
Contributor

@Samuell1 I'll leave this open. It's something that may need to be looked into in the future.

@Samuell1
Copy link
Member

Samuell1 commented Aug 7, 2019

@bennothommo it was just ping if this is actual, i can then do PR to remove it, because there is still label response needed.

@bennothommo
Copy link
Contributor

@Samuell1 by all means, have a go at it if you want to :). Probably is worth having a look at.

@github-actions
Copy link

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.

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

No branches or pull requests

5 participants