what am I supposed to see with this PR as I dont see any changed behaviour

Before change onExtensionBeforeSave() was not executed for plugins in brainforgeuk folder.
Always executed for plugins in system folder, in most cases not executed for other folders.

One outstanding issue - when a disabled plugin is enabled event_after_save is not executed.
To fix that would require a method of clearing static::$plugins in PluginHelper.php
That would allow PluginHelper::importPlugin() to be called again.

One outstanding issue - when a disabled plugin is enabled event_after_save is not executed.

That is why I didnt observe any changes

Gut feeling is that this feels like a potential security incident waiting to happen because we're loading plugin groups from user data. But I'm not quite sure if it's actually exploitable or not. @joomla/security thoughts?

No just realised this is only loading the plugin being modified. Maybe it's fine

in my opinion you are in charge to import your own plugins when you need them.

as george found out ;-) it make sense but please don't do it in the AdminModel move this code the the PluginModel we should have one.

Agree moving code to PluginModel makes more sense.
Bit more complex, but easier to resolve the caveat I mentioned about enabling disabled plugins.

How do I merge change to another file into this pull request?
The proposed change to AdminModel.php can be removed.
The PluginModel changes I propose is at line 342.

`
public function save($data)
{
// Setup type.
$data['type'] = 'plugin';

    // Make sure onExtensionBeforeSave(), if it exists, gets executed.
    // Note it is the responsibility of the plugin to only respond to relevent event triggers.
    // E.G. for system plugins onExtensionBeforeSave() and onExtensionAfterSave() get executed everywhere they exist.
    $pluginBeingSaved = PluginHelper::importPlugin($data['folder'], $data['element']);

    if ($result = parent::save($data)) {

        if (empty($pluginBeingSaved)) {
            if (!empty($data['enabled'])) {
                // Plugin was previously disabled
                $plugin = Factory::getApplication()->bootPlugin($data['element'], $data['folder']);

                // Execute, if it exists, onExtensionAfterSave() for this plugin ONLY.
                $plugin->onExtensionAfterSave('com_plugins.plugin', null, false, $data);
            }
        }
        else {
            // Nothing to do as onExtensionAfterSave(), if it exists, has already been executed
        }
    }

    return $result;
}

`

@BrainforgeUK You can go to your branch on GitHub https://github.com/BrainforgeUK/joomla-cms/tree/patch-15 and then edit any file on GitHub.

Thanks, PluginModel.php changed.

I rebased this for 5.0 since 4.x is more or less in feature freeze mode

Thanks

In 5.0.0-beta2-dev after applying the patch I get this:

An error has occurred.

    0 Class "Joomla\Component\Plugins\Administrator\Model\PluginHelper" not found 

So this is a test fail!

It seems the file "administrator/components/com_plugins/src/Model/PluginModel.php" is missing a use statement for the PluginHelper at the top. Done 2023-09-26.

Added use statement for PluginHelper.

I have looked at it again. And it still looks bad :)

Please trash it all, and use following code for save() method:

public function save($data)
{
  // Setup type.
  $data['type'] = 'plugin';

  PluginHelper::importPlugin($data['folder'], $data['element'], true, $this->getDispatcher());

  return parent::save($data);
}

Then all will work.

The simplified version from Fedik does not work!

Plugin enabled, change it to disabled OK.
Plugin disabled, change it to enabled onExtensionAfterSave(() is not called.
... this change would not resolve the initial plugin development issue which led to this PR being created.
... and I just happen to have a dev site with that plugin installed open and could quickly test it!

Hm, I see you trying to load disabled plugin also.
I not sure that we should allow that.
@HLeithner @wilsonge what is your opinion about triggering event for disabled plugin?

@BrainforgeUK a note, your code also does not work for disabled plugin.
The code $plugin = Factory::getApplication()->bootPlugin($data['element'], $data['folder']); will return incorrectly loaded plugin, without actual parameters etc.

The data returned by bootPlugin() does not matter - the saved data is passed to the event.
bootPlugin() ensures that the plugin has been loaded (loadExtension) and its constructor has been called.
Before the save the plugin was disabled and had not been loaded.

The data returned by bootPlugin() does not matter - the saved data is passed to the event.

Unfortunately it does matter.
The call inside event listener $this->getParams() will be different for enabled and disabled plugin, and probably other inconsistency.
It may lead to unexpected bugs.

Got solution to the bootplugin() / getParams() issue.
Now uses PluginHelper::reload(); to ensure the newly enabled plugin is in the right state.
Added reload function to PluginHelper.

This pull request has been automatically rebased to 5.1-dev.

This pull request has been automatically rebased to 5.2-dev.

This pull request has been automatically rebased to 5.3-dev.