Skip to content

Move OC_Defaults to OCP\Defaults #4256

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

Merged
LukasReschke merged 2 commits into from
Apr 11, 2017
Merged

Move OC_Defaults to OCP\Defaults #4256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b4adf6
Move OC_Defaults to OCP\Defaults
MorrisJobke Apr 7, 2017
d83c8e0
ThemingDefaults append cacheBusterCounter to logo URL by default
MorrisJobke Apr 9, 2017
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
3 changes: 2 additions & 1 deletion apps/federatedfilesharing/settings-personal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@
$cloudID = \OC::$server->getUserSession()->getUser()->getCloudId();
$url = 'https://nextcloud.com/federation#' . $cloudID;
$logoPath = \OC::$server->getURLGenerator()->imagePath('core', 'logo-icon.svg');
$theme = \OC::$server->getThemingDefaults();
/** @var \OCP\Defaults $theme */
$theme = \OC::$server->query(\OCP\Defaults::class);
Copy link
Member

@nickvergessen nickvergessen Apr 10, 2017

Choose a reason for hiding this comment

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

Cant we make the method return the same thing? Sounds much better to me, or whats the difference?

Copy link
Member

@rullzer rullzer Apr 10, 2017

Choose a reason for hiding this comment

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

We could but all those wrapper things are so weird to me. Quering the right class directly seems so much nicer to me...

Copy link
Member

@nickvergessen nickvergessen Apr 10, 2017

Choose a reason for hiding this comment

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

Well it's public API and therefor the method needs to return the correct item anyway?!

Copy link
Member

@rullzer rullzer Apr 10, 2017

Choose a reason for hiding this comment

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

pfff well still...
but fine by me.

Copy link
Member Author

@MorrisJobke MorrisJobke Apr 10, 2017

Choose a reason for hiding this comment

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

The methods are slightly different -.- and one is public and the other is private namespace :/

$color = $theme->getColorPrimary();
$textColor = "#ffffff";
if(\OC::$server->getAppManager()->isEnabledForUser("theming")) {
Expand Down
3 changes: 2 additions & 1 deletion apps/files_sharing/lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
use OCA\Files_Sharing\Controller\ExternalSharesController;
use OCA\Files_Sharing\Controller\ShareController;
use OCA\Files_Sharing\Middleware\SharingCheckMiddleware;
use OCP\Defaults;
use OCP\Federation\ICloudIdManager;
use \OCP\IContainer;
use OCP\IServerContainer;
Expand Down Expand Up @@ -67,7 +68,7 @@ public function __construct(array $urlParams = array()) {
$federatedSharingApp->getFederatedShareProvider(),
$server->getEventDispatcher(),
$server->getL10N($c->query('AppName')),
$server->getThemingDefaults()
$server->query(Defaults::class)
);
});
$container->registerService('ExternalSharesController', function (SimpleContainer $c) {
Expand Down
4 changes: 2 additions & 2 deletions apps/files_sharing/lib/Controller/ShareController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class ShareController extends Controller {
* @param FederatedShareProvider $federatedShareProvider
* @param EventDispatcherInterface $eventDispatcher
* @param IL10N $l10n
* @param \OC_Defaults $defaults
* @param Defaults $defaults
*/
public function __construct($appName,
IRequest $request,
Expand All @@ -122,7 +122,7 @@ public function __construct($appName,
FederatedShareProvider $federatedShareProvider,
EventDispatcherInterface $eventDispatcher,
IL10N $l10n,
\OC_Defaults $defaults) {
Defaults $defaults) {
parent::__construct($appName, $request);

$this->config = $config;
Expand Down
2 changes: 1 addition & 1 deletion apps/files_sharing/tests/Controller/ShareControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ protected function setUp() {
$this->federatedShareProvider,
$this->eventDispatcher,
$this->getMockBuilder('\OCP\IL10N')->getMock(),
$this->getMockBuilder('\OC_Defaults')->getMock()
$this->getMockBuilder('\OCP\Defaults')->getMock()
);


Expand Down
3 changes: 2 additions & 1 deletion apps/provisioning_api/lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use OC\Settings\Mailer\NewUserMailHelper;
use OCA\Provisioning_API\Middleware\ProvisioningApiMiddleware;
use OCP\AppFramework\App;
use OCP\Defaults;
use OCP\Util;

class Application extends App {
Expand All @@ -18,7 +19,7 @@ public function __construct(array $urlParams = array()) {

$container->registerService(NewUserMailHelper::class, function(SimpleContainer $c) use ($server) {
return new NewUserMailHelper(
$server->getThemingDefaults(),
$server->query(Defaults::class),
$server->getURLGenerator(),
$server->getL10N('settings'),
$server->getMailer(),
Expand Down
8 changes: 4 additions & 4 deletions apps/provisioning_api/lib/Controller/UsersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@
use OC\Settings\Mailer\NewUserMailHelper;
use \OC_Helper;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\OCS\OCSException;
use OCP\AppFramework\OCS\OCSForbiddenException;
use OCP\AppFramework\OCSController;
use OCP\Defaults;
use OCP\Files\NotFoundException;
use OCP\IConfig;
use OCP\IGroup;
Expand Down Expand Up @@ -69,7 +69,7 @@ class UsersController extends OCSController {
private $urlGenerator;
/** @var IMailer */
private $mailer;
/** @var \OC_Defaults */
/** @var Defaults */
private $defaults;
/** @var IFactory */
private $l10nFactory;
Expand All @@ -88,7 +88,7 @@ class UsersController extends OCSController {
* @param string $fromMailAddress
* @param IURLGenerator $urlGenerator
* @param IMailer $mailer
* @param \OC_Defaults $defaults
* @param Defaults $defaults
* @param IFactory $l10nFactory
* @param NewUserMailHelper $newUserMailHelper
*/
Expand All @@ -103,7 +103,7 @@ public function __construct($appName,
$fromMailAddress,
IURLGenerator $urlGenerator,
IMailer $mailer,
\OC_Defaults $defaults,
Defaults $defaults,
IFactory $l10nFactory,
NewUserMailHelper $newUserMailHelper) {
parent::__construct($appName, $request);
Expand Down
6 changes: 3 additions & 3 deletions apps/provisioning_api/tests/Controller/UsersControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@
use OC\SubAdmin;
use OCA\Provisioning_API\Controller\UsersController;
use OCP\AppFramework\Http\DataResponse;
use OCP\Defaults;
use OCP\IConfig;
use OCP\IGroup;
use OCP\ILogger;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IURLGenerator;
Expand Down Expand Up @@ -74,7 +74,7 @@ class UsersControllerTest extends TestCase {
private $urlGenerator;
/** @var IMailer|PHPUnit_Framework_MockObject_MockObject */
private $mailer;
/** @var \OC_Defaults|PHPUnit_Framework_MockObject_MockObject */
/** @var Defaults|PHPUnit_Framework_MockObject_MockObject */
private $defaults;
/** @var IFactory|PHPUnit_Framework_MockObject_MockObject */
private $l10nFactory;
Expand All @@ -93,7 +93,7 @@ protected function setUp() {
$this->accountManager = $this->createMock(AccountManager::class);
$this->urlGenerator = $this->createMock(IURLGenerator::class);
$this->mailer = $this->createMock(IMailer::class);
$this->defaults = $this->createMock(\OC_Defaults::class);
$this->defaults = $this->createMock(Defaults::class);
$this->l10nFactory = $this->createMock(IFactory::class);
$this->newUserMailHelper = $this->createMock(NewUserMailHelper::class);

Expand Down
8 changes: 4 additions & 4 deletions apps/theming/lib/Controller/IconController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,19 @@

use OCA\Theming\IconBuilder;
use OCA\Theming\ImageManager;
use OCA\Theming\ThemingDefaults;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\Http\FileDisplayResponse;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Defaults;
use OCP\Files\NotFoundException;
use OCP\IRequest;
use OCA\Theming\Util;
use OCP\IConfig;

class IconController extends Controller {
/** @var ThemingDefaults */
/** @var Defaults */
private $themingDefaults;
/** @var Util */
private $util;
Expand All @@ -54,7 +54,7 @@ class IconController extends Controller {
*
* @param string $appName
* @param IRequest $request
* @param ThemingDefaults $themingDefaults
* @param Defaults $themingDefaults
* @param Util $util
* @param ITimeFactory $timeFactory
* @param IConfig $config
Expand All @@ -64,7 +64,7 @@ class IconController extends Controller {
public function __construct(
$appName,
IRequest $request,
ThemingDefaults $themingDefaults,
Defaults $themingDefaults,
Util $util,
ITimeFactory $timeFactory,
IConfig $config,
Expand Down
15 changes: 4 additions & 11 deletions apps/theming/lib/ThemingDefaults.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,13 @@ public function getLogo() {
$logoExists = false;
}

$cacheBusterCounter = $this->config->getAppValue('theming', 'cachebuster', '0');

if(!$logo || !$logoExists) {
return $this->urlGenerator->imagePath('core','logo.svg');
return $this->urlGenerator->imagePath('core','logo.svg') . '?v=' . $cacheBusterCounter;
}

return $this->urlGenerator->linkToRoute('theming.Theming.getLogo');
return $this->urlGenerator->linkToRoute('theming.Theming.getLogo') . '?v=' . $cacheBusterCounter;
Copy link
Member Author

@MorrisJobke MorrisJobke Apr 9, 2017

Choose a reason for hiding this comment

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

@juliushaertl I would like to have your feedback on this. I think this is the cleaner way to handle this cache buster, because then the getLogo call always returns the full URL and it's not needed to know that this needs to be appended.

}

/**
Expand Down Expand Up @@ -190,15 +192,6 @@ public function shouldReplaceIcons() {
return $value;
}

/**
* Gets the current cache buster count
*
* @return string
*/
public function getCacheBusterCounter() {
return $this->config->getAppValue('theming', 'cachebuster', '0');
}

/**
* Increases the cache buster key
*/
Expand Down
6 changes: 3 additions & 3 deletions apps/theming/tests/Controller/IconControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
use OC\Files\SimpleFS\SimpleFile;
use OCA\Theming\ImageManager;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataDisplayResponse;
use OCP\AppFramework\Http\NotFoundResponse;
use OCP\Defaults;
use OCP\Files\IRootFolder;
use OCP\Files\NotFoundException;
use OCP\IConfig;
Expand All @@ -41,7 +41,7 @@
class IconControllerTest extends TestCase {
/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
private $request;
/** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */
/** @var Defaults|\PHPUnit_Framework_MockObject_MockObject */
private $themingDefaults;
/** @var Util */
private $util;
Expand All @@ -58,7 +58,7 @@ class IconControllerTest extends TestCase {

public function setUp() {
$this->request = $this->getMockBuilder('OCP\IRequest')->getMock();
$this->themingDefaults = $this->getMockBuilder('OCA\Theming\ThemingDefaults')
$this->themingDefaults = $this->getMockBuilder('OCP\Defaults')
->disableOriginalConstructor()->getMock();
$this->util = $this->getMockBuilder('\OCA\Theming\Util')->disableOriginalConstructor()
->setMethods(['getAppImage', 'getAppIcon', 'elementColor'])->getMock();
Expand Down
18 changes: 14 additions & 4 deletions apps/theming/tests/ThemingDefaultsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,25 +417,35 @@ public function testGetBackgroundCustom() {

public function testGetLogoDefault() {
$this->config
->expects($this->once())
->expects($this->at(0))
->method('getAppValue')
->with('theming', 'logoMime')
->willReturn('');
$this->config
->expects($this->at(1))
->method('getAppValue')
->with('theming', 'cachebuster', '0')
->willReturn('0');
$this->appData
->expects($this->once())
->method('getFolder')
->with('images')
->willThrowException(new \Exception());
$expected = $this->urlGenerator->imagePath('core','logo.svg');
$expected = $this->urlGenerator->imagePath('core','logo.svg') . '?v=0';
$this->assertEquals($expected, $this->template->getLogo());
}

public function testGetLogoCustom() {
$this->config
->expects($this->once())
->expects($this->at(0))
->method('getAppValue')
->with('theming', 'logoMime')
->willReturn('image/svg+xml');
$this->config
->expects($this->at(1))
->method('getAppValue')
->with('theming', 'cachebuster', '0')
->willReturn('0');
$simpleFolder = $this->createMock(ISimpleFolder::class);
$this->appData
->expects($this->once())
Expand All @@ -447,7 +457,7 @@ public function testGetLogoCustom() {
->method('getFile')
->with('logo')
->willReturn('');
$expected = $this->urlGenerator->linkToRoute('theming.Theming.getLogo');
$expected = $this->urlGenerator->linkToRoute('theming.Theming.getLogo') . '?v=0';
$this->assertEquals($expected, $this->template->getLogo());
}
}
22 changes: 0 additions & 22 deletions core/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,8 @@

namespace OC\Core;

use OC\AppFramework\Utility\SimpleContainer;
use OC\Core\Controller\JsController;
use OC\Core\Controller\OCJSController;
use OC\Security\IdentityProof\Manager;
use OC\Server;
use OCP\AppFramework\App;
use OC\Core\Controller\CssController;
use OCP\AppFramework\Utility\ITimeFactory;
Expand Down Expand Up @@ -70,25 +67,6 @@ public function __construct() {
$container->query(ITimeFactory::class)
);
});
$container->registerService(OCJSController::class, function () use ($container) {
/** @var Server $server */
$server = $container->getServer();
return new OCJSController(
$container->query('appName'),
$server->getRequest(),
$server->getL10N('core'),
// This is required for the theming to overwrite the `OC_Defaults`, see
// https://github.com/nextcloud/server/issues/3148
$server->getThemingDefaults(),
$server->getAppManager(),
$server->getSession(),
$server->getUserSession(),
$server->getConfig(),
$server->getGroupManager(),
$server->getIniWrapper(),
$server->getURLGenerator()
);
});
Copy link
Member Author

@MorrisJobke MorrisJobke Apr 7, 2017

Choose a reason for hiding this comment

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

This reverts basically #3163, because it is now properly injected as OCP\Defaults from the server container. Fixed the cause not the symptoms 🙌 cc @nickvergessen

$container->registerService(JsController::class, function () use ($container) {
return new JsController(
$container->query('AppName'),
Expand Down
3 changes: 2 additions & 1 deletion core/Command/Maintenance/Install.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use InvalidArgumentException;
use OC\Setup;
use OC\SystemConfig;
use OCP\Defaults;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Helper\QuestionHelper;
use Symfony\Component\Console\Input\InputInterface;
Expand Down Expand Up @@ -70,7 +71,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
// validate the environment
$server = \OC::$server;
$setupHelper = new Setup($this->config, $server->getIniWrapper(),
$server->getL10N('lib'), $server->getThemingDefaults(), $server->getLogger(),
$server->getL10N('lib'), $server->query(Defaults::class), $server->getLogger(),
$server->getSecureRandom());
$sysInfo = $setupHelper->getSystemInfo(true);
$errors = $sysInfo['errors'];
Expand Down
7 changes: 4 additions & 3 deletions core/Controller/LostController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
use \OCP\AppFramework\Controller;
use \OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Defaults;
use OCP\Encryption\IManager;
use \OCP\IURLGenerator;
use \OCP\IRequest;
Expand All @@ -58,7 +59,7 @@ class LostController extends Controller {
protected $urlGenerator;
/** @var IUserManager */
protected $userManager;
/** @var \OC_Defaults */
/** @var Defaults */
protected $defaults;
/** @var IL10N */
protected $l10n;
Expand All @@ -82,7 +83,7 @@ class LostController extends Controller {
* @param IRequest $request
* @param IURLGenerator $urlGenerator
* @param IUserManager $userManager
* @param \OC_Defaults $defaults
* @param Defaults $defaults
* @param IL10N $l10n
* @param IConfig $config
* @param ISecureRandom $secureRandom
Expand All @@ -96,7 +97,7 @@ public function __construct($appName,
IRequest $request,
IURLGenerator $urlGenerator,
IUserManager $userManager,
\OC_Defaults $defaults,
Defaults $defaults,
IL10N $l10n,
IConfig $config,
ISecureRandom $secureRandom,
Expand Down
Loading