Skip to content
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

Plugin Id is not available to Plugin in CMSPlugin.php #38094

Closed
Irata opened this issue Jun 19, 2022 · 9 comments
Closed

Plugin Id is not available to Plugin in CMSPlugin.php #38094

Irata opened this issue Jun 19, 2022 · 9 comments

Comments

@Irata
Copy link
Contributor

Irata commented Jun 19, 2022

While in the execution of a plugin the id of the plugin in #__extensions is not available.

The id is retrieved as part of PluginHelper.php and passed as part of $config to the __construct of CMSPlugin along with three other values, type, name and params and are assigned to $this->_type, $this->_name and $this-params respectively however id is not assigned to $this->_id as you might expect.

Steps to reproduce the issue

$fred = $this->_id

Expected result

$fred = 1

Actual result

$this->_id not found

System information (as much as possible)

If this code was added to the __construct of CMSPlugin.php at around line #114 then the problem would be resolved.

		// Get the plugin id.
		if (isset($config['id']))
		{
			$this->_id = $config['id'];
		}

Additional comments

@brianteeman
Copy link
Contributor

What would you use the ID for?

@Irata
Copy link
Contributor Author

Irata commented Jun 19, 2022

To change the value(s) of a plugin, in particular the params, via my code.

As it stands at the moment I have to do another call to the DB to determine the id within my plugin to find the correct record to update in #__extensions.

In the sample of code pasted below that I am using at the moment, the first three lines would not be required if the id was available in the same way the other three values were set in the __construct.

public function updateParams($key, $value)
{
    $table = new  Extension(Factory::getDbo());
    $table->load(array('element' => $this->_name, 'type' => 'plugin', 'folder' => $this->_type));
    $data['extension_id'] = $table->extension_id;

    $this->params->set($key, $value);
    $data['params'] = $this->params->toString();

    $plugin_model = new PluginModel();
    $plugin_model->save($data);
}

@alikon
Copy link
Contributor

alikon commented Jun 20, 2022

$plugin = PluginHelper::getPlugin($type, $name);
echo $plugin->id;

should do

@Irata
Copy link
Contributor Author

Irata commented Jun 21, 2022

PluginHelper::getPlugin() has already been executed and the four columns retrieved, including extension_id, and the results passed to the __construct of CMSPluign but for some reason the id was not set in the same way the other three properties are set.

So the issue is not how to get the plugin id but for consistency and ease of use it should already be available and not require another call to getPlugin() or other methods.

Once _id is available then updating params looks a lot simpler.

        $this->params->set($key, $value);
        $plugin_model = new PluginModel();
        $plugin_model->save(array('extension_id' => $this->_id, 'params' => $this->params->toString()));

The same should also apply to the new column custom_data in the #__extensions table, it could or should be available in the same way params are now, in a _custom_data property, however I may log that as a separate issue or enhancement.

@laoneo
Copy link
Member

laoneo commented Jun 21, 2022

You can safe it in your own class when you need it. Just override the constructor.

@joomdonation
Copy link
Contributor

We do not need this in our core plugins and when it is needed like in your case, you can always override plugin constructor to store data for your own usage as pointed out by @laoneo above. Therefore, I'm closing this issue.

@Irata
Copy link
Contributor Author

Irata commented Nov 13, 2022

This issue is/was opened to highlight a code quality or consistency concern and is not about asking for assistance on how to get the extension_id of the plugin.

In libraries/src/Plugin/PluginHelper.php within the Load() function, lines 251-315, four fields are retrieved from the Extension table including extension_id. When an instance of a Plugin is created the __construct processes the other three fields that were retrieved, the extension_id is not used.

It seems a bit like getting four new tyres(tires) for you car and they only put air in three of them and expect you put air in the fourth for no apparent reason other than that you can.

If the 'decision' is that extension_id is not to be made available at the start of their plugins then load() in libraries/src/Plugin/PluginHelper.php should be updated to remove the SQL for the retrieval of the extension_id field. However that would then be inconsistent with libraries/src/Component/ComponentHelper.php load() function and libraries/src/Helper/LibraryHelper.php function loadLibrary() that also retrieve the extension_id, I haven't looked to see if it used along with the other fields they retrieve.

In my opinion to close this issue then some lines of code should to be added or other lines of redundant code should be removed. For ease of use, less code code and a potentially repetitive call to the DB for a value already retrieved then making the extension_id available by default should be reconsidered.

If you disagree then leave this issue closed.

@joomdonation
Copy link
Contributor

Yes. Look like extension_id is not used here and could be removed. However, we could not remove it now (at least until Joomla 6) because it would cause backward incompatible changes (in case some third party plugins like yours use it)

We also could not add _id property now as you suggested because:

  • It is not needed (in core plugins)
  • It could cause potential backward incompatible change. Imagine that we add this as a protected property, and someone add some property in his plugin as private, his plugin will be broken

So I will still keep the issue closed for now. If you still disagree, I will ask maintainers for final decision :).

@Irata
Copy link
Contributor Author

Irata commented Nov 14, 2022

@joomdonation , I don't really see the relevance that because the 'core' doesn't use it then it isn't a worthwhile thing to make available to the extension developer, I am sure there are plenty of functions and features available to developers that the core doesn't use.

If there was a potential collision with someone's own plugins use of id then it would actually support the argument for including it in the __construct and not leave it for extension developers to fill in the missing piece. If we could scan every plugin extending CMSPlugin for the use of getPlugin() to get the id/extension_id then we might have a better idea of how widespread its use is and therefore requirement might be.

There is also the more recent inclusion of the custom_data column in the #__extensions table that would further increase the need for id to be available to get and set values in the correct row for each plugin.

It looks to me that whenever the four fields were added to PluginHelper and CMSPlugin it was incomplete, one field got missed or left out on purpose, so if this isn't an issue then should I resubmit it as a new Feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants