Move Interface Definitions to the ServerContainer #3901

rullzer · 2017-03-17T13:36:18Z

Followup of #3897
Implementation of #2043 (comment)

This should allow us to the interface defintions to the servercontainer. So we don't duplicate everything for each App.

mention-bot · 2017-03-17T13:36:20Z

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @BernhardPosselt, @MorrisJobke and @nickvergessen to be potential reviewers.

MorrisJobke · 2017-03-17T18:47:41Z

lib/private/ServerContainer.php

@@ -79,7 +79,7 @@ public function query($name) {
 			$segments = explode('\\', $name);
 			$appContainer = $this->getAppContainer(strtolower($segments[1]));
 			try {
-				return $appContainer->query($name);
+				return $appContainer->query($name, false);


Why is the false here needed? Shouldn't it try the server container from the app container and done?

To avoid possible endless loops. Like if you request a non existing class.

Is there a way to maybe fix this by overriding the method in the server container instead?

This is in the servercontainer. The thing is that once we call the query method of the servercontainer we don't know which container called that.

ok, lol my bad :D

Ah right, i mean the method above which is defined in the DIContainer

Ah got it, the inerhitance hierarchy is borked. The DIContainer is the base container for apps, however the server container also uses it.

Then I'd say simply subclass it and replace it here: https://github.com/nextcloud/server/blob/master/lib/public/AppFramework/App.php#L68

rullzer · 2017-03-19T20:21:50Z

So 1 thing I did think of is:

Assume that in the server there is a registeredService Foo that wants a service Bar Injected.

Now if an app myApp overwrites Bar and wants Foo injected. Then that will not happen with this code. Since when looking for Foo we no longer search for Bar in the apps container.

Mmmm. let me set this back to develop to think some more.

rullzer · 2017-03-19T20:23:40Z

I'll see if I can come up with some tests to check the behavior we want. Should make discussing a bit easier.

BernhardPosselt · 2017-03-19T20:18:14Z

lib/private/ServerContainer.php

@@ -79,7 +79,7 @@ public function query($name) {
 			$segments = explode('\\', $name);
 			$appContainer = $this->getAppContainer(strtolower($segments[1]));
 			try {
-				return $appContainer->query($name);
+				return $appContainer->query($name, false);


ok, lol my bad :D

BernhardPosselt · 2017-03-19T20:18:52Z

lib/private/ServerContainer.php

@@ -79,7 +79,7 @@ public function query($name) {
 			$segments = explode('\\', $name);
 			$appContainer = $this->getAppContainer(strtolower($segments[1]));
 			try {
-				return $appContainer->query($name);
+				return $appContainer->query($name, false);


Ah right, i mean the method above which is defined in the DIContainer

BernhardPosselt · 2017-03-19T20:29:38Z

lib/private/ServerContainer.php

@@ -79,7 +79,7 @@ public function query($name) {
 			$segments = explode('\\', $name);
 			$appContainer = $this->getAppContainer(strtolower($segments[1]));
 			try {
-				return $appContainer->query($name);
+				return $appContainer->query($name, false);


Ah got it, the inerhitance hierarchy is borked. The DIContainer is the base container for apps, however the server container also uses it.

Then I'd say simply subclass it and replace it here: https://github.com/nextcloud/server/blob/master/lib/public/AppFramework/App.php#L68

BernhardPosselt · 2017-03-19T20:56:52Z

lib/private/AppFramework/DependencyInjection/DIContainer.php

+		try {
+			return parent::query($name);
+		} catch (QueryException $e) {
+			if ($checkServerContainer === false) {


if (strpos(OC\App::buildAppNamespace($this->query('AppName')), $name) !== 0) { return $this->getServer()->query($name); }

Only forwards the request to the server container if the classes lie somewhere else. If you swap out interfaces from the server in your own container it wont hit the server container since you don't hit the catch block

BernhardPosselt · 2017-03-19T20:58:05Z

lib/private/AppFramework/DependencyInjection/DIContainer.php

+		try {
+			return parent::query($name);
+		} catch (QueryException $e) {
+			if ($checkServerContainer === false) {


Only forwards the request to the server container if the classes lie somewhere else. If you swap out interfaces from the server in your own container it wont hit the server container since you don't hit the catch block

codecov-io · 2017-03-19T21:34:11Z

Codecov Report

Merging #3901 into master will decrease coverage by 0.01%.
The diff coverage is 98.85%.

@@             Coverage Diff              @@
##             master    #3901      +/-   ##
============================================
- Coverage     54.28%   54.27%   -0.02%     
- Complexity    21147    21156       +9     
============================================
  Files          1304     1304              
  Lines         80805    80793      -12     
  Branches       1282     1282              
============================================
- Hits          43867    43852      -15     
- Misses        36938    36941       +3

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Server.php	`92.96% <100%> (+0.17%)`	`120 <1> (ø)`	⬇️
...e/AppFramework/DependencyInjection/DIContainer.php	`82.97% <96.15%> (-6.13%)`	`25 <3> (+9)`
lib/private/Security/CertificateManager.php	`93.81% <0%> (+1.03%)`	`38% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd0e42e...92f50c7. Read the comment docs.

MorrisJobke · 2017-03-20T09:21:25Z

.htaccess

+#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####
+
+ErrorDocument 403 /core/templates/403.php
+ErrorDocument 404 /core/templates/404.php


yeah yeah fixed. It just still sucks that I can't just do a git commit -a because I ran the testsuite.

nickvergessen · 2017-03-20T11:42:08Z

lib/private/Server.php

@@ -477,7 +494,7 @@ public function __construct($webRoot, \OC\Config $config) {
 		$this->registerService('CredentialsManager', function (Server $c) {
 			return new CredentialsManager($c->getCrypto(), $c->getDatabaseConnection());
 		});
-		$this->registerService('DatabaseConnection', function (Server $c) {
+		$this->registerService(IDBConnection::class, function (Server $c) {


Needs adjustments in the unit tests as well

server/tests/lib/TestCase.php

Line 141 in b62b82c

\OC::$server->registerService('DatabaseConnection', function () {

PS its multiple times.

BernhardPosselt · 2017-03-20T17:31:13Z

Looks good I think 👍

MorrisJobke · 2017-03-20T18:40:41Z

@rullzer Could you have a look why that many integration tests fail?

rullzer · 2017-03-21T07:52:13Z

@MorrisJobke yes on it. It is because doing it half isn't going to work ;)

To align with #2043 (comment) This would mean that AppContainers only hold the AppSpecific services Signed-off-by: Roeland Jago Douma <[email protected]>

Signed-off-by: Roeland Jago Douma <[email protected]>

* Moved some interface definitions to Server.php (more to come) * Build/Query only for existing classes in the AppContainer * Build/Query only for classes of the App in the AppContainer * Offload other stuff to the servercontainer Signed-off-by: Roeland Jago Douma <[email protected]>

Signed-off-by: Roeland Jago Douma <[email protected]>

icewind1991 · 2017-03-21T16:18:09Z

💥 causes infinite recursion in DI for me 💥

icewind1991 · 2017-03-21T16:27:09Z

Seems to be caused by apps where there namespace is not equal to their app id (files_automatedtagging vs \OCA\FilesAutomatedTagging)

rullzer · 2017-03-21T19:19:00Z

@icewind1991 mmm fuck. yeah of course that could happen. Let me fix a PR for that. I think we have a way around it.

As stated in #3901 (comment) appid's don't have to match the namespace. Work around this Signed-off-by: Roeland Jago Douma <[email protected]>

rullzer added 2. developing Work in progress enhancement labels Mar 17, 2017

rullzer added this to the Nextcloud 12.0 milestone Mar 17, 2017

rullzer requested review from BernhardPosselt, nickvergessen, MorrisJobke and LukasReschke March 17, 2017 13:36

MorrisJobke reviewed Mar 17, 2017

View reviewed changes

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 19, 2017

LukasReschke approved these changes Mar 19, 2017

View reviewed changes

rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 19, 2017

BernhardPosselt requested changes Mar 19, 2017

View reviewed changes

BernhardPosselt reviewed Mar 19, 2017

View reviewed changes

BernhardPosselt requested changes Mar 19, 2017

View reviewed changes

rullzer force-pushed the di_ng branch from a46cef6 to 1d5a897 Compare March 19, 2017 21:10

rullzer force-pushed the di_ng branch from a1c60cb to ce2449e Compare March 19, 2017 22:16

MorrisJobke reviewed Mar 20, 2017

View reviewed changes

rullzer force-pushed the di_ng branch from 49621fc to cc44e28 Compare March 20, 2017 09:21

nickvergessen requested changes Mar 20, 2017

View reviewed changes

rullzer force-pushed the di_ng branch 5 times, most recently from 6c92710 to 38068ed Compare March 20, 2017 15:56

nickvergessen approved these changes Mar 20, 2017

View reviewed changes

BernhardPosselt approved these changes Mar 20, 2017

View reviewed changes

rullzer added 8 commits March 21, 2017 08:52

Update query method for DIContainer

8862021

To align with #2043 (comment) This would mean that AppContainers only hold the AppSpecific services Signed-off-by: Roeland Jago Douma <[email protected]>

PoC of moving the interface classes to the servercontainer

df14684

Signed-off-by: Roeland Jago Douma <[email protected]>

More elegant handling of recursion

246e9ce

Signed-off-by: Roeland Jago Douma <[email protected]>

Extend DI tests

7cece61

Signed-off-by: Roeland Jago Douma <[email protected]>

Fix settings tests

c92b9ce

Signed-off-by: Roeland Jago Douma <[email protected]>

Fix NODB tests etc

0fb5776

Signed-off-by: Roeland Jago Douma <[email protected]>

Move a lot of stuff over to the ServerContainer

48c3452

Signed-off-by: Roeland Jago Douma <[email protected]>

rullzer force-pushed the di_ng branch from 38068ed to 48c3452 Compare March 21, 2017 09:30

Core is also a special app

92f50c7

Signed-off-by: Roeland Jago Douma <[email protected]>

LukasReschke merged commit 04f419b into master Mar 21, 2017

LukasReschke deleted the di_ng branch March 21, 2017 10:06

rullzer added a commit that referenced this pull request Mar 21, 2017

Make DI work for all apps

67909cf

As stated in #3901 (comment) appid's don't have to match the namespace. Work around this Signed-off-by: Roeland Jago Douma <[email protected]>

rullzer mentioned this pull request Mar 21, 2017

Make DI work for all apps #3977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Interface Definitions to the ServerContainer #3901

Move Interface Definitions to the ServerContainer #3901

rullzer commented Mar 17, 2017

mention-bot commented Mar 17, 2017

MorrisJobke Mar 17, 2017

rullzer Mar 17, 2017

BernhardPosselt Mar 19, 2017

rullzer Mar 19, 2017

BernhardPosselt Mar 19, 2017

BernhardPosselt Mar 19, 2017

BernhardPosselt Mar 19, 2017

rullzer commented Mar 19, 2017

rullzer commented Mar 19, 2017

BernhardPosselt Mar 19, 2017

BernhardPosselt Mar 19, 2017

BernhardPosselt Mar 19, 2017

BernhardPosselt Mar 19, 2017

BernhardPosselt Mar 19, 2017

BernhardPosselt Mar 19, 2017

codecov-io commented Mar 19, 2017 •

edited

Loading

MorrisJobke Mar 20, 2017

rullzer Mar 20, 2017

nickvergessen Mar 20, 2017

nickvergessen Mar 20, 2017

BernhardPosselt commented Mar 20, 2017

MorrisJobke commented Mar 20, 2017

rullzer commented Mar 21, 2017

icewind1991 commented Mar 21, 2017 •

edited

Loading

icewind1991 commented Mar 21, 2017

rullzer commented Mar 21, 2017

Move Interface Definitions to the ServerContainer #3901

Move Interface Definitions to the ServerContainer #3901

Conversation

rullzer commented Mar 17, 2017

mention-bot commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rullzer commented Mar 19, 2017

rullzer commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 19, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BernhardPosselt commented Mar 20, 2017

MorrisJobke commented Mar 20, 2017

rullzer commented Mar 21, 2017

icewind1991 commented Mar 21, 2017 • edited Loading

icewind1991 commented Mar 21, 2017

rullzer commented Mar 21, 2017

codecov-io commented Mar 19, 2017 •

edited

Loading

icewind1991 commented Mar 21, 2017 •

edited

Loading