Develop the 1.9 theme and plugin system #4601
Conversation
* Move the default theme's templates from `inc/views/base` to various
directories under `inc/themes/core.default/devdist`.
* Create a set of theme-related functions in `inc/functions_themes.php`
which facilitate querying the filesystem for theme directories,
including at the top level a correctly-ordered list of template
directories for Twig to search.
* Cache the result of the new `get_themelet_dirs()` function. (To
consider: is any (probably very minor) speed up offered by this caching
worth the increased cost of keeping the cache up to date?).
* Make some minor updates to the `inc/hello.php` file to support Twig
template rendering, and create the relevant Twig templates. (Note:
because a Twig equivalent for `find_replace_templatesets()` has not yet
been implemented, to test this amended plugin, the line `{{ hello|raw
}}` needs to be manually added to the frontend template
`index/index.twig`).
* Add a hook to the `\MyBB\template()` function so that parameters can
be added to the template's `context` variables.
…selection This support is preliminary and probably very incomplete.
This was a minor error in the previous commit.
Resolves mybb#4535. As indicated in the issue, I am not clued up on this area of the code but have tried to fix this issue with this PR anyway. I hope that those in the know will correct it if it's deficient.
* Add the concept of a resource "specifier" string, beginning with ~ for theme resources and ! for plugin themelet resources. * Add support for resource specifiers to MyBB::get_asset_url(), via the new function resolve_themelet_resource(). * Adapt ThemeExtension::getStylesheets() to use resource specifiers, as well as to support plugin stylesheets. * Add a stylesheet to the "hello" plugin so as to test the code (it works). Also update that plugin's compatibility property to "19*". * Add a `resource.php` script which returns themelet resources stipulated using a resource specifier via a "specifier" URL query parameter. * Set $theme when in the ACP because some functions render theme templates in *both* user *and* ACP mode. * Add the function install_core_19_theme_to_db() for use in installation/upgrade routines. Adapt installation/update routines to use it. * Fix a couple of errors which prevented the Themes and Templates ACP pages from loading due to a change to the schema of the `themes` database table.
Change $codename everywhere it occurs to the more descriptive sub-types `$theme_codename` and `$plugin_codename`.
Simply a copy of the previously externally-referenced file https://mybb.github.io/1.9-ui-framework/assets/css/main.css now auto-included by the new code introduced in the commit two back.
The bugfix is in inc/functions_themes.php: A check for a 'css' extension was being run on $resource_path which hadn't yet been set to a meaningful value.
As per the current version of the spec. This was an oversight.
* Logic: all plugins were being checked for the resource, instead of just the plugin that owns the resource. * Caching: * Leverage the existing `getIncludedFiles()` method of the SCSS compilation result, storing it in a dependencies (.deps) file in the cache directory, rather than running the custom function I'd written to generate those same results on every page load. * Also store a dependencies file in the cache directory for every resource, so that we know the cache is stale in the case that the target resolved resource changes - e.g., if a new board theme adds an override of that resource. The first line of the dependency file is the primary resource. Additional lines are dependencies, such as for SCSS `@import` files. * Additionally: refactor the code a little to avoid redundancy, by abstracting it into the little helper function `test_set_path()`.
Perform this conversion given that this is new code, and that I understand the coding standards for 1.9 are the same as for 2.0: https://github.com/mybb/standards Also, remove a redundant function that ought to have been removed in the previous commit.
The plugin resource specifier prefix changes from ! to ~p~ The current-theme resource specifier prefix changes from ~ to ~t~
* Move data out of the `_info()` function and into a JSON manifest file, but provide support for dynamic modification/generation of a plugin's `description` property via a `_dyndesc()` function. * So far only the listing of staged plugins in the ACP Plugins page is supported. Yet to add is support for: * Actually integrating (moving from `staging` to live), installing, and activating staged plugins. * Upgrading staged plugins when a version of the plugin is already installed. * Upload of zipped plugins via the ACP's Plugins page, auto-staging them, and then integrating, installing, and activating them.
"Integration" refers to moving the plugin's files from `staging/plugins/[codename]' to the root MyBB directory.
Make a few adjustments for coding style at the same time, and fix a minor issue or two.
The `array_merge_recursive()` semantics aren't suitable for some of our properties, such as when merging jscript attributes: we would end up with, e.g. ['defer' => [0 => 'defer', 1 => 'defer]] rather than simply ['defer' => 'defer']. The `array_replace_recursive()` semantics are better though probably not ideal.
This is so that a clean PR can be filed.
This is necessary because Git does not retain empty directories.
This is necessary because Git does not retain empty directories.
This is so that the PR can be merged cleanly.
Per agreement with dvz, who preferred the approach outlined in the spec (Cold Duplicate), from which I'd deviated. Active themelet archiving on installation/upgrade of themes and plugins via the ACP remains in place. The Cold Duplicate approach for when themes/plugins are copied in place via the filesystem is not yet implemented.
Revert due to the call to `$templates->cache()` in commit e1f73af which introduces a fatal error: `template::cache()` in `inc/templates.php` selects from the dropped DB table `templates`.
| } else { | ||
| // Plugin is compatible with this version? | ||
| $plugininfo = read_json_file(MYBB_ROOT."inc/plugins/{$codename}/plugin.json"); | ||
| if (empty($plugininfo['compatibility']) || $plugins->is_compatible($plugininfo['compatibility']) == false) { |
There was a problem hiding this comment.
I think this check is done above (#552) for upgrades
| @@ -517,36 +680,53 @@ | |||
|
|
|||
| $plugins->run_hooks("admin_config_plugins_plugin_list"); | |||
|
|
|||
| $form = new Form('index.php?module=config-plugins&action=upload', 'post', '', 1); | |||
There was a problem hiding this comment.
Was a permission for uploading plugins added ?
It probably should and a separated page for this should be added.
There was a problem hiding this comment.
We should:
- advertise the ACP's Plugins module as extremely dangerous regarding permissions altogether
- reject Theme Packages with
.phpfiles - add a config flag to disable uploads of any Packages through the ACP (i.e. impossible to modify with any permission level)
and IMO:
- disallow ACP uploading Plugin Packages - which contain
.phpfiles - by default (similarly, we might allow it with a config flag; IMO still not worth the risk)
otherwise the upload feature would serve vulnerability escalation to RCE on a silver platter.
There was a problem hiding this comment.
I agree with all your points but the first one which I quite don't understand.
Also, won't disabling PHP files on plugin packages kill the feature altogether ? What plugins don't use PHP files ?
There was a problem hiding this comment.
RE the 1st point: as Plugins can arbitrarily change the application's logic, it may make sense to warn administrators on the permission selection page, and in security guides, that they shouldn't grant access to the Plugins section of the ACP to anyone else unless necessary.
I've corrected the comment to indicate that only the new feature of in-ACP HTTP uploads is problematic (as opposed to FTP or similar level access).
| $plugininfo = $infofunc(); | ||
| $plugininfo['codename'] = $codename; | ||
| $codename = str_replace('.php', '', $plugin_file); | ||
| $plugininfo = read_json_file(MYBB_ROOT."inc/plugins/{$codename}/plugin.json", $err_msg, /*$show_errs = */false); |
There was a problem hiding this comment.
I thought only template updates were going to be the major requirement for 1.8 plugins to work on 1.9 but it seems that is no longer the case ?
Are required changes documented somewhere or somehow ?
There was a problem hiding this comment.
Not sure about this implementation, but the plan was to allow plugins to function mostly unaffected (only modifications of existing templates would break).
We want to introduce .json manifest files for both Themes and Plugins - in 1.9, it would be required for Themes, but only optional for Plugins (as an alternative to [codename]_info()), preserving compatibility in this area.
There was a problem hiding this comment.
I honestly would prefer if you would force the same structure for everything, including a certain folder structure inside the ZIP files. This a) would finally allow to script plugin installations (or ideally use composer for it) and b) simplify the whole plugin system and make things more maintainable. So if you already break the plugin system, take the chance and do it properly once instead of over and over again over the next couple releases.
Personally I would also implement a PluginInterface (along with either a trait or an abstract class for convenience) that defines initialize, enable, disable, install, uninstall etc methods and force the plugins to wrap their methods into a proper, ideally namespaced class. It's not really much more work for plugin authors to convert the currently global functions into a class, and on MyBB side you could get rid of all the if(function_exists()) stuff and simply call it.
This class could in the end also hold the metadata and what not.
| if (!is_mutable_theme($codename, $mybb->settings['themelet_dev_mode'])) { | ||
| $err_msg = $lang->error_immutable_theme; | ||
| } else { | ||
| // TODO: Abstract this code given that it is duplicated for saving in "advanced" mode below |
There was a problem hiding this comment.
Maybe abstract before merging ?
| flash_message($lang->error_invalid_stylesheet, 'error'); | ||
| admin_redirect("index.php?module=style-themes"); | ||
| } | ||
| // Possible TODO: Implement support on this same page for editing of any SCSS module |
There was a problem hiding this comment.
We should document these TODOs for later.
| @@ -0,0 +1,31 @@ | |||
| [This licence applies to the *.scss files in this directory and its subdirectories] | |||
|
|
|||
| BSD 3-Clause License | |||
There was a problem hiding this comment.
Why do we use a different license here ?
| @@ -0,0 +1,43 @@ | |||
| <?php | |||
|
|
|||
| function clone_and_archive_default_theme() | |||
There was a problem hiding this comment.
Why do we want to keep a copy of the default theme ?
There was a problem hiding this comment.
Before a different-than-planned approach was removed from this PR, it was necessary to call this, but now it should be opportunistic (to avoid copying on the first forum visit after update, or via the task system), and running the installer would not be necessary unless we actually need to run the release-specific migrations.
There was a problem hiding this comment.
So the default theme here is mutable ? I thought the theme being archived here was the inmutable version of the default theme.
There was a problem hiding this comment.
The kind of mutation that would be relevant here is the official changes occurring between different versions of the Core Theme.
The tools for applying upstream changes in child themes would benefit from a version-to-version diff presented to the user, demonstrating the changes that have to be somehow mirrored in the child theme (when, for whatever reason, it cannot happen automatically).
Core and Original Themes will indeed be protected from accidental changes, and the UI will guide users away from it unless they're in an "expert" "Development Mode".
|
|
||
|
|
||
|
|
||
| /** TODO: Migrate as necessary to 1.9 any of the below 1.8 code (now being |
There was a problem hiding this comment.
This is probable a cleanup to do before merging.
| @@ -303,10 +303,18 @@ min=0]]></optionscode> | |||
| <settingvalue><![CDATA[0]]></settingvalue> | |||
| <isdefault>1</isdefault> | |||
| </setting> | |||
| <setting name="themelet_dev_mode"> | |||
| <title>Development Mode (Themelets)</title> | |||
There was a problem hiding this comment.
Hiding this to the file level has been discussed ? We probably don't want production users to mess with the development features that easily.
There was a problem hiding this comment.
This relates to a mode for Extension authors, allowing to bypass Resource caches, edit Packages otherwise protected from accidental writes, etc. In the end it shouldn't impact internal stability or security.
Indeed, right now it may be confusing, and names appearing in the UI (like Development Mode) will need to be revisited, to make sure they're clear and approachable (without sacrificing accuracy).
I've added Reconcile developer/debug modes in the UX section of https://github.com/orgs/mybb/projects/3.
| // We'd prefer not to load this file, and instead load inc/init.php alone, because it is more | ||
| // lightweight, but we need it for its resolution of the current theme for the current user, which | ||
| // it sets $theme to, and which is used within resolve_themelet_resource(). | ||
| require_once './global.php'; |
There was a problem hiding this comment.
I'm thinking on the 1.8 context here, so might be wrong, but this might be too heavy to load for each resource, as far as I understand the below code and this file's purpose.
There was a problem hiding this comment.
This is only expected in "Development Mode". Its primary purpose is to allow skipping asset cache and proxy files from internal Packages instead (inc/), when they cannot be accessed directly via HTTP (a best practice we want to double down on).
There was a problem hiding this comment.
This is only expected in "Development Mode".
Oh, ok, so only disabling otherwise seems to be missing I think.
There was a problem hiding this comment.
Do you mean reverting to normal URLs?
This procedure should be handled automatically - normal paths would be used as arguments in custom Twig functions (asset_url()/asset(), which may be named differently in this PR), which will adjust the URLs to either point to the exposed cache directory (during normal operation), or resource.php when the special mode is on.
There was a problem hiding this comment.
I meant to disable the resource.php file when the special mode is off, at least in the file itself I couldn't notice a code that blocks it. It might be on the global.php file or init.php so if so ignore my comment.
Miscellaneous Changes
Theme System ImplementationThis is an implementation of the MyBB 1.9 Theme System specification. Code Changes Overview
Specification DeviationsThe code generally follows the linked abstract specification; the following exceptions are known:
|
I'm unsure about this, seems like some runtime data like group permissions would be better off if stored in the DB as well. |
|
sorry if this is the wrong way to bring this up, but since you are (thankfully) refactoring the templating and plugin system, may I kindly suggest that you will support composer to install plugins? As of now, it is a huge PITA to manage forum installations along with the plugins via GIT, have proper deployment methods and what not. And since you are migrating to a json file for plugin metadata anyways and that plugins now have their own directory, using a In addition, may I suggest to add a hook to the language loader class which forum "owners" could use to hook into via a custom plugin and adjust some language labels of either the core or other plugins? Right now you have to patch files which is a nightmare to maintain when you try to keep everything updated with security patches and what not. Thanks. PS: future wishlist :)
|
| if (!empty($page->plugin)) { | ||
| $modules_dir = MYBB_ROOT.'inc/plugins/'.$page->plugin.'/admin/modules'; | ||
| } | ||
|
|
||
| require $modules_dir."/".$run_module."/".$action_file; |
There was a problem hiding this comment.
would it make sense to check first if the file actually exists?
| @@ -372,30 +374,153 @@ | |||
| $page->output_footer(); | |||
| } | |||
|
|
|||
| // Activates or deactivates a specific plugin | |||
| if($mybb->input['action'] == "activate" || $mybb->input['action'] == "deactivate") | |||
| $is_upload = ($action == 'upload'); | |||
There was a problem hiding this comment.
in this day and age, shouldn't more strict comparissons be used? ( === instead of ==)
| $message = $lang->success_plugin_activated; | ||
| } else { | ||
| // Plugin is compatible with this version? | ||
| $plugininfo = read_json_file(MYBB_ROOT."inc/plugins/{$codename}/plugin.json"); |
There was a problem hiding this comment.
why do you read the json file twice in case of an upgrade? See line 549. It might make sense to add a new method getPluginInfo(string $codename): ?array which will perform the parsing of the json file and then return a cached version of this in case of subsequent calls.
| if(!empty($plugins_list)) | ||
| { | ||
| $a_plugins = $i_plugins = array(); | ||
|
|
||
| foreach($plugins_list as $plugin_file) | ||
| if(!empty($plugins_list)) |
There was a problem hiding this comment.
unless github is messing up the diff view, this condition is superfluous as it's literally done 4 lines above.
| $plugininfo = $infofunc(); | ||
| $plugininfo['codename'] = $codename; | ||
| $codename = str_replace('.php', '', $plugin_file); | ||
| $plugininfo = read_json_file(MYBB_ROOT."inc/plugins/{$codename}/plugin.json", $err_msg, /*$show_errs = */false); |
There was a problem hiding this comment.
I honestly would prefer if you would force the same structure for everything, including a certain folder structure inside the ZIP files. This a) would finally allow to script plugin installations (or ideally use composer for it) and b) simplify the whole plugin system and make things more maintainable. So if you already break the plugin system, take the chance and do it properly once instead of over and over again over the next couple releases.
Personally I would also implement a PluginInterface (along with either a trait or an abstract class for convenience) that defines initialize, enable, disable, install, uninstall etc methods and force the plugins to wrap their methods into a proper, ideally namespaced class. It's not really much more work for plugin authors to convert the currently global functions into a class, and on MyBB side you could get rid of all the if(function_exists()) stuff and simply call it.
This class could in the end also hold the metadata and what not.
| if(isset($active_plugins[$codename])) | ||
| { | ||
| // This is an active plugin | ||
| $plugininfo['is_active'] = 1; |
There was a problem hiding this comment.
abstracting the pluginInfo into a class with dedicated setters and strict return values would IMO be a better approach over an array with unknown indices if you are rewriting this part anyways atm (code completion in IDEs anyone?).
| } else if ($themeinfo['codename'] != $codename) { | ||
| $errors[] = $lang->sprintf($lang->error_codename_mismatch, htmlspecialchars_uni($themeinfo['codename']), htmlspecialchars_uni($codename)); | ||
| } else if (file_exists(MYBB_ROOT."inc/themes/{$codename}")) { | ||
| $infofile2 = MYBB_ROOT."inc/themes/{$codename}/theme.json"; |
There was a problem hiding this comment.
is it in general a good idea to have themes inside the inc folder which usually is protected from public access? Or are the theme assets at a different location? If so, I would suggest to keep them in one location and rather force their directory structure to have a public folder with assets that must be accessible from outside while the rest can be protected via .htaccess or nginx rules.
| // Find out if there was an error with the uploaded file | ||
| if ($_FILES['local_file']['error'] != 0) { | ||
| $errors[] = $lang->error_uploadfailed.$lang->error_uploadfailed_detail; | ||
| switch($_FILES['local_file']['error']) { |
There was a problem hiding this comment.
couldn't this entire block be simplified to
$errorNo = (int)$_FILES['local_file']['error'];
$errors[] = $lang->{'error_uploadfailed_php' . $errorNo} ?? $lang->sprintf($lang->error_uploadfailed_phpx, $errorNo);
|
Our plans are generally in line with your suggestions, @da-anda. Eventually, we want to adjust how MyBB stores data to improve clarity, safety, and deployment & management. These changes would include storage formats and locations (i.a. for metadata and language phrases). Accordingly, extension packages would be treated internal code, with front-end assets copied to exposed directories as needed (with SCSS transpilation, etc.). 1.9 focuses on the theme system, but we'll want to bring plugin metadata to the same format, and make it possible to install all extensions via Composer/CLI/API. Better support of manual, and theme-supplied language phrases - that would be loaded automatically - should make it in at some point. MyBB 1.9 introduces Laravel's Container in new code. |
This is a WIP but already with many features and commits.
See the individual commits for the implemented features.
I'm happy though (and hope) for it to be merged ASAP by anybody who thinks it's worth doing that.
The two major features lacking at time of initial PR are:
Related issue: #3689