Do not disable plugins during bugfixes updates

[cedric-anne]

Checklist before requesting a review

• I have read the CONTRIBUTING document.
• I have performed a self-review of my code.
• I have added tests that prove my fix is effective or that my feature works.
• This change requires a documentation update.

Description

It make sense to disable all plugins when GLPI is upgraded to a new intermediate version (e.g. 9.4 -> 9.5) or a new major version (9.5 -> 10.0), because we can introduce BC breaks in the code that would result in a complete GLPI crash when an incompatible plugin is loaded. But it does not seems required to do it when the update does not contains more than bugfixes. Indeed, in bugfixes version, we are not suppose to introduce BC-breaks (see https://glpi-developer-documentation.readthedocs.io/en/develop/sourcecode.html#backward-compatibility).

I therefore propose to disable the plugins only when an update is made to a new intermediate or major version.

Please note that it will also prevent the tester plugin to be disabled on the test database everytime an update is made to update the DB to the latest dev version.

[AdrienClairembault]

Plugins can define that they only support specific minors versions.
Couldn't this lead to a plugin being active on a version that it does not support ?

[cedric-anne]

Plugins can define that they only support specific minors versions. Couldn't this lead to a plugin being active on a version that it does not support ?

No, the plugin loading logic will anyway check the compatibility of the plugin everytime GLPI is booted, and the plugin will be disabled in this case. See

glpi/src/Plugin.php

Lines 790 to 812 in a71aa6f

	// Check prerequisites
	if ($usage_ok) {
	$function = 'plugin_' . $plugin_key . '_check_prerequisites';
	if (function_exists($function)) {
	ob_start();
	if (!$function()) {
	$usage_ok = false;
	}
	ob_end_clean();
	}
	}
	if (!$usage_ok) {
	// Deactivate if not usable
	trigger_error(
	sprintf(
	'Plugin "%s" prerequisites are not matched. It has been deactivated.',
	$plugin_key
	),
	E_USER_WARNING
	);
	$this->unactivate($plugin->fields['id']);
	}

[cconard96]

Plugin loading in general should be more resilient now. Any exception thrown from an init function should disable that plugin now. I wonder if it's needed to limit this just to bugfix versions or if it is OK globally.

[cedric-anne]

Plugin loading in general should be more resilient now. Any exception thrown from an init function should disable that plugin now. I wonder if it's needed to limit this just to bugfix versions or if it is OK globally.

If there is compile error, the error will not be caught (see #18695), so we cannot remove this for now for all updates. Something was started to handle this, see #18765, but we did not had time yet to finish it.

[orthagh]

I agree with this proposal only if you can assure me the plugins are not called (no hooks) during updates.
So, it is like they are temporarily disabled.

[cedric-anne]

I agree with this proposal only if you can assure me the plugins are not called (no hooks) during updates. So, it is like they are temporarily disabled.

Our migrations are not suppose to use something else than hardoded values, and are supposed to only do direct DB queries that cannot be hooked by plugins.
Maybe there are some old migrations that contains some calls to CommonDBTM::add()/CommonDBTM::update()/.... If it cause issues, I guess we will be able to detect and fix them quickly. As long as this change is targetting an major version, I consider it as an acceptable risk.

[trasher]

I would say the same as @orthagh: I'm OK if plugins are not loaded during the installation phase - "hooks" are probably not only the only possible issue.

[cedric-anne]

Unless on specific circumstances, the plugins are not loaded during the update process, see

glpi/src/Glpi/Kernel/Listener/InitializePlugins.php

Lines 61 to 64 in 8cf551b

	if (!DBConnection::isDbAvailable() || (!defined('SKIP_UPDATES') && !Update::isDbUpToDate())) {
	// Requires the database to be available.
	return;
	}

The are loaded only in CLI context, if the update is "replayed", or if the SKIP_UPDATES constant is defined. This is true for either the exiting code and the proposed patch.

Disabling the plugins will anyway only disable the hooks execution in the current update context. We could add a unloadAllPlugins that would have the same effect for the update process, and would permit to not have them disabled after the update process.

[orthagh]

Like I said IRL, I'm not strongly against the proposed changes.
I'm nervous about plugins interfering with GLPI during the update process.
They shouldn't, regarding the piece of code mentioned above, but the question I asked, Is there any possibility a plugin page can be called externally by a user or script and so interfere with the database.

We quickly mention the public router may disable its legacy routes when a plugin is disabled (update or not)
But can this be a solution for the current concern also?

[cedric-anne]

We quickly mention the public router may disable its legacy routes when a plugin is disabled (update or not)
But can this be a solution for the current concern also?

It is a partial solution to this, and has been done in #19271.

I'm nervous about plugins interfering with GLPI during the update process.

With the current state of the code, it is not possible, but unrelated changes may change this behaviour. The best solution would be to implement the following behaviour:

• suspend all plugins;
• do the GLPI update;
• optionally unsuspend all plugins if the update corresponds to a bugfixes update only.

It is probably easy to implement, and I would like to do it prior to the GLPI 11.0 RC1 release, but I have to focus on most important topics for the moment.