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.

Sign up for GitHub

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

theming: handle not being in the serverroot #8463

Merged
merged 3 commits into from
Feb 26, 2018
Merged

theming: handle not being in the serverroot #8463

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1f1824
theming: handle not being in the serverroot
kyrofa Feb 21, 2018
25c89a6
Add mock to setup
kyrofa Feb 24, 2018
8dcbea1
Remove null check
kyrofa Feb 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions apps/theming/lib/Controller/ThemingController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
use OCA\Theming\Util;
use OCP\ITempManager;
use OCP\IURLGenerator;
use OCP\App\IAppManager;

/**
* Class ThemingController
Expand Down Expand Up @@ -78,6 +79,8 @@ class ThemingController extends Controller {
private $scssCacher;
/** @var IURLGenerator */
private $urlGenerator;
/** @var IAppManager */
private $appManager;

/**
* ThemingController constructor.
Expand All @@ -93,6 +96,7 @@ class ThemingController extends Controller {
* @param IAppData $appData
* @param SCSSCacher $scssCacher
* @param IURLGenerator $urlGenerator
* @param IAppManager $appManager
*/
public function __construct(
$appName,
Expand All @@ -105,7 +109,8 @@ public function __construct(
ITempManager $tempManager,
IAppData $appData,
SCSSCacher $scssCacher,
IURLGenerator $urlGenerator
IURLGenerator $urlGenerator,
IAppManager $appManager = NULL
) {
parent::__construct($appName, $request);

Expand All @@ -118,6 +123,12 @@ public function __construct(
$this->appData = $appData;
$this->scssCacher = $scssCacher;
$this->urlGenerator = $urlGenerator;

if (!is_null($appManager)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no need for such a check. The AppManager will be injected automatically.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, only in this test case, right? If it's not injected (like the other test cases, or in typical usage) it should default back to the server's app manager. Perhaps I misunderstand?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mock should just be added to the controller here: https://github.com/kyrofa/server/blob/a1f18241166f335b89a6cf3d002efd3d825fde19/apps/theming/tests/Controller/ThemingControllerTest.php#L109

Then of course the method need to be configured to the other tests as well.

Copy link
Member Author

@kyrofa kyrofa Feb 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that, although we'll still need the null check for the real system. Eventually this parameter could become non-optional and the callers could all start passing it in, but that's a larger change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the tests to add the mock in setUp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great. Then you can get rid of the is_null check since the appManager will be injected automatically, just like the other constructor arguments. So it will always be \OC::$server->getAppManager() except for the tests where we call the constructor ourselfs and it will be mocked then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, indeed, I'm not sure how the other constructor arguments are handed off. There must be some sort of introspection magic happening there that I haven't seen, because you're right-- removing the null check works! I've pushed that up, but can you show me where this magic is happening so I can further my understanding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can read more about the dependency injection here: https://docs.nextcloud.com/server/13/developer_manual/app/container.html#how-does-automatic-assembly-work

This is basically where the "magic" happens: 😉

server/lib/private/AppFramework/Utility/SimpleContainer.php

Line 50 in 0eebff1

private function buildClass(ReflectionClass $class) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh and the puzzle pieces come together! Thank you for helping me understand this :) .

$this->appManager = $appManager;
} else {
$this->appManager = \OC::$server->getAppManager();
}
}

/**
Expand Down Expand Up @@ -409,12 +420,13 @@ public function getLoginBackground() {
* @return FileDisplayResponse|NotFoundResponse
*/
public function getStylesheet() {
$appPath = substr(\OC::$server->getAppManager()->getAppPath('theming'), strlen(\OC::$SERVERROOT) + 1);
$appPath = $this->appManager->getAppPath('theming');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈 👍


/* SCSSCacher is required here
* We cannot rely on automatic caching done by \OC_Util::addStyle,
* since we need to add the cacheBuster value to the url
*/
$cssCached = $this->scssCacher->process(\OC::$SERVERROOT, $appPath . '/css/theming.scss', 'theming');
$cssCached = $this->scssCacher->process($appPath, 'css/theming.scss', 'theming');
if(!$cssCached) {
return new NotFoundResponse();
}
Expand Down
34 changes: 34 additions & 0 deletions apps/theming/tests/Controller/ThemingControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,40 @@ public function testGetStylesheetFails() {
$this->assertEquals($response, $actual);
}

public function testGetStylesheetOutsideServerroot() {
$this->themingController = new ThemingController(
'theming',
$this->request,
$this->config,
$this->themingDefaults,
$this->util,
$this->timeFactory,
$this->l10n,
$this->tempManager,
$this->appData,
$this->scssCacher,
$this->urlGenerator,
$this->appManager
);
$this->appManager->expects($this->once())->method('getAppPath')->with('theming')->willReturn('/outside/serverroot/theming');
$file = $this->createMock(ISimpleFile::class);
$file->expects($this->any())->method('getName')->willReturn('theming.css');
$file->expects($this->any())->method('getContent')->willReturn('compiled');
$this->scssCacher->expects($this->once())->method('process')->with('/outside/serverroot/theming', 'css/theming.scss', 'theming')->willReturn(true);
$this->scssCacher->expects($this->once())->method('getCachedCSS')->willReturn($file);

$response = new Http\FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'text/css']);
$response->cacheFor(86400);
$expires = new \DateTime();
$expires->setTimestamp($this->timeFactory->getTime());
$expires->add(new \DateInterval('PT24H'));
$response->addHeader('Expires', $expires->format(\DateTime::RFC1123));
$response->addHeader('Pragma', 'cache');

$actual = $this->themingController->getStylesheet();
$this->assertEquals($response, $actual);
}

public function testGetJavascript() {
$this->themingDefaults
->expects($this->at(0))
Expand Down