-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Avoid duplicate App container creation #14548
Avoid duplicate App container creation #14548
Conversation
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
How to verify this? |
try { | ||
$this->container = \OC::$server->getRegisteredAppContainer($appName); | ||
} catch (QueryException $e) { | ||
$this->container = new \OC\AppFramework\DependencyInjection\DIContainer($appName, $urlParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the container need to be registered then in the server container to avoid it being instantiated twice again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what DIContainer does already.
|
Ah ok so this doesn't solve the 2 constructor calls (let do that separatly). But it is quite elegant. However. it does seem like our tests are not as clean as we'd like... |
CI failure :/ |
Signed-off-by: Joas Schilling <coding@schilljs.com>
🤖 beep boop beep 🤖 Here are the logs for the failed build: Status of 16809: failureDB=mysql, ENABLE_REDIS=false, PHP=7.3Show full log
DB=mysqlmb4, ENABLE_REDIS=false, PHP=7.3Show full log
TESTS=integration-federation_features
Show full log
TESTS=integration-sharing-v1
Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=app-files
Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=app-files-sharing-link
Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=login
Show full log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, tested and works 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do this
This patch breaks the
|
Will this and all other relevant patches which are required to fix regressions be backported to 15 and 14? |
14 is running out of support pretty soon And as for 15, I'm not sure, since it might require work by other apps to fix stuff too. |
Signed-off-by: Joas Schilling coding@schilljs.com