-
-
Notifications
You must be signed in to change notification settings - Fork 173
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
New option to disable the vue template compiler #6965
base: develop-minor
Are you sure you want to change the base?
Conversation
config/areas/system/views.php
Outdated
@@ -80,11 +81,19 @@ | |||
]; | |||
} | |||
|
|||
if ($debugMode === true && $kirby->option('panel.vue.compiler', true) === true) { |
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.
I get why this security warning depends on the debug mode: If we would always display it, we would have a security warning for the default configuration of the panel.vue.compiler
option, which is weird. But IMO making it dependent on the debug mode is also weird – it's not an information one needs during debugging, but especially in production.
So I wonder if we can find a different solution:
- Maybe we can find a different way or place to display this warning in a context-sensitive way.
- Maybe it is possible to auto-detect whether any installed plugin needs the template compiler and control the default value of the option accordingly? Then we could display this warning even without debug mode.
- ...
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.
I wouldn't display it without debug mode or some other restriction (admins only?) cause the choice to have it might be done very deliberately by the developer, but then the users/customers freak out due to this warning if it's shown to them in production.
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 true. I like the idea of admin-only.
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.
Or we only show it as long as the config option isn't set (so instead of passing a fallback/default we first check for null).
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.
I love that last idea.
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.
I've implemented it like that.
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.
@bastianallgeier Now he message is shown when set to true
explicitly. My suggestion was rather to check for null, show it as long as no config option is set.
I've had a closer look at the coverage and I really think it's another issue from Codecov. The lines are all covered. |
Summary of changes
Todos
Assets::vue
Changelog
Enhancements