load default enabled theme during error messages #27974
Conversation
…eme in case of exceptions (error pages)
|
does looks a bit risky. if the info.xml parsing itself also fails, then we'll get a white page. would need to catch an exception within the exception |
|
It's the same code we are using for the install/update page. Also the same applies to If the instance runs into an exception and then gets another exception while parsing the info.xml, then something went extremely horribly wrong. |
| OC_Response::setStatus(OC_Response::STATUS_FORBIDDEN); | ||
| OC_Template::printErrorPage($ex->getMessage()); | ||
| } catch (Exception $ex) { | ||
| \OC::loadDefaultEnabledAppTheme(); |
There was a problem hiding this comment.
Hm you are loading the theme because OC_Template needs it when rendering output to the browser ... how about moving \OC::loadDefaultEnabledAppTheme(); to https://github.com/owncloud/core/blob/master/lib/private/legacy/template.php#L102? Loading will then be triggered by the OC_Template constructor itself.
Or you get the DI sortetd out for templates and inject it ... but that's something for 10.1 not 10.0.3
There was a problem hiding this comment.
We got proper loading for normal pages. For error pages we have to parse the info.xml files, and that's something we should not do for all pages. That's why i had to do this hacky way.
The best way imo would be to store the path to the enabled theme in a local config file which can be accessed through a config object. But currently we do not support dynamic writing to config files and the enabled theme is the activated theme app.
Once we cleaned up db/local config we could think about a better solution for this, but the scope is too big for just this right now.
|
I'd like to have a configuration like @phisch suggested, yet for now this is working well. |
PVince81
added
backport-request
p1-urgent
|
@phisch please backport, we need this in stable10 |
|
ah ok. I discarded the changelog entry because I believed it was a testing thing. I'll add it |
|
added here #28694 (comment) |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
As themes are now apps, and no apps are loaded during errors, the error messages are shown with the default theme. This loads any theme that has the
default_enabletag active for all error pages (not requests without errors). I reused the functionality that was used for the installation/update screen.Related Issue
https://github.com/owncloud/enterprise/issues/1984
How Has This Been Tested?
This has been tested by hand. I manually triggered all exceptions that run into error pages, and the theme loads successfully.
Sadly this brought my attention to another bug that was undetected until now:
Any theme that does not have the
default_enabletag, that is activated during an error, is not loaded.For 10.0.2 we should find a solution for this problem. We cannot rely on any database connection or whatever here. So the only way we can guarantee theme loading in every case, is if we save the enabled theme into the config file and use this one to enable the theme, and use the enabled apps (through database) as fallback.
I will create a tech-debt ticket for that with 10.0.2 milestone.
Screenshots (if appropriate):
Types of changes
Checklist: