theming: handle not being in the serverroot #8463
Conversation
Currently, the theming app assumes it's in the serverroot. However, with Nextcloud's flexibility regarding configurable app paths, this is not a safe assumption to make. If it happens to be an incorrect assumption, the theming app fails to work. Instead of relying on the serverroot, just use the path from the AppManager and utilize relative paths for assets from there. Fix nextcloud#8462 Signed-off-by: Kyle Fazzari <[email protected]>
| @@ -118,6 +123,12 @@ public function __construct( | |||
| $this->appData = $appData; | |||
| $this->scssCacher = $scssCacher; | |||
| $this->urlGenerator = $urlGenerator; | |||
|
|
|||
| if (!is_null($appManager)) { | |||
There was a problem hiding this comment.
There should be no need for such a check. The AppManager will be injected automatically.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated the tests to add the mock in setUp.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: 😉
There was a problem hiding this comment.
Ahhh and the puzzle pieces come together! Thank you for helping me understand this :) .
| @@ -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'); | |||
juliusknorr
added
bug
3. to review
|
(Note that the test failure here seems unrelated.) |
|
I'd very much like to backport this for 12.0.6 and 13.0.1! |
Signed-off-by: Kyle Fazzari <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #8463 +/- ##
============================================
- Coverage 51.82% 51.74% -0.08%
- Complexity 25363 25377 +14
============================================
Files 1601 1601
Lines 95018 95044 +26
Branches 1377 1377
============================================
- Hits 49241 49182 -59
- Misses 45777 45862 +85
|
Signed-off-by: Kyle Fazzari <[email protected]>
|
Thanks folks. Can I get a yea/nay on backporting to stable{12,13}? Happy to propose. |
Looks fine - please create the backport PRs for 12 and 13 👍 |
|
Backport PRs are up: |
Currently, the theming app assumes it's in the serverroot. However, with Nextcloud's flexibility regarding configurable app paths, this is not a safe assumption to make. If it happens to be an incorrect assumption, the theming app fails to work.
Instead of relying on the serverroot, this PR fixes #8462 by simply using the app path from the AppManager and utilizing relative paths for assets from there. Note that, in order to make this testable, the ability to inject an
IAppManagerto theThemingControllerwas added.As this has been broken since v12, a backport to both stable13 and stable12 would be great (happy to do the legwork if given the okay).