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

Rewrite installation plugins and add them to Hathor #10450

Merged
merged 4 commits into from
Jun 8, 2016

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented May 13, 2016

With 3.6.0, the installer tabs are generated by plugins similar how the install from web tab is done.
However this wasn't done for Hathor.

The way the plugins generated the output, it wasn't also possible to do proper overwrites for Hathor. The tabs and fieldsets were generated by the plugins, however Hathor doesn't use tabs.

Summary of Changes

This PR rewrites the plugins so they return the data instead of directly outputting the tab. The tab is then generated by the Isis layout file.
This PR also adds the functionality (plugin trigger) to Hathor.

Testing Instructions

  • Test in Isis and Hathor that the various extension install ways still work.
  • Try enabling/disabling the plugins and check that they properly show/hide in the installer view
  • Try rearranging the plugins and see that they follow the ordering set in the plugin manager (except the install from web one).

@810
Copy link
Contributor

810 commented May 13, 2016

I have tested this item ✅ successfully on 15e1931


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@@ -18,58 +18,39 @@
class PlgInstallerFolderInstaller extends JPlugin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bakual Can you fix the double space here as well to a single space?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing 😄

@roland-d
Copy link
Contributor

I have tested this item ✅ successfully on 15e1931

Choosing Hathor, the installation plugins show up and work.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@brianteeman
Copy link
Contributor

Almost but not quite.
If you disable all the install plugins and use Isis you will get a warning
No installation plugin has been enabled. At least one must be enabled to be able to use the installer. Go to the Plugin Manager to enable the plugins.

In Hathor you dont get any notice

(Otherwise everything else is good)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@roland-d
Copy link
Contributor

@brianteeman Good point, I actually did want to check that but must have looked at the wrong place. I checked again but it is the JED Installer that I saw. Thanks for pointing this out.

@roland-d
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 15e1931

The message is not shown when no plugins are enabled.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @810, @roland-d


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @810, @roland-d


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@Bakual
Copy link
Contributor Author

Bakual commented May 13, 2016

If you disable all the install plugins and use Isis you will get a warning
No installation plugin has been enabled. At least one must be enabled to be able to use the installer. Go to the Plugin Manager to enable the plugins.
In Hathor you dont get any notice

Good catch. Added that message now as well to Hathor.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 49e416a


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@izharaazmi
Copy link
Contributor

I have few doubts regarding this:

  • Why aren't we using JForm to load forms in the installer view and use the plugin event onContentPrepareForm($form, $data) to extend the form.
  • Why are the plugin events fired in the layout? Aren't we supposed to separate logic from output as much as possible!

@roland-d
Copy link
Contributor

@izharaazmi JForm isn't used because I don't see the added value here as all we need is an input field. With what would you want to extend the form? You are better off writing your own installation plugin. I think it is really overkill.

You are a little late to the party as to why the plugin is triggered there. It has been there since the introduction of Install from web. I would even argue, how is this different from doing for example $form->renderInput('fieldname');

@Bakual
Copy link
Contributor Author

Bakual commented May 14, 2016

The way the plugins currently work (including the install from web) it wouldn't be possible to move the call to the view. The plugins directly generate output.
With my proposed change here it would be possible since they return the data instead of echoing it. However only the new trigger could be moved to the view, the old triggers still need to stay in the layout and thus we don't gain much (the check for the message would also have to stay in the layout).

@izharaazmi
Copy link
Contributor

izharaazmi commented May 14, 2016

It has been there since the introduction of Install from web.

Fine if it was so. But you and I and we are working on improvements, right? After all we are going to have a major release.

I also want to implement the ability to the installer plugins to perform their own install routine. I understand that this is a gradual process. I am just trying a kickstart. I am also willing to work towards this but not sure if it will be breaking anything. Experts' advice needed.

Less significant stuff below:

You are a little late to the party as to why the plugin is triggered there. It has been there since the introduction of Install from web.

I and my friend had a discussion on having a direct installation from JED mechanism long before Install from Web came. But ah, we did not have the access to the JED api or alike. We never know then that we can ask/discuss for. Gave up! 😄

@roland-d
Copy link
Contributor

@izharaazmi Yes we are working on improvements but with the B/C promise our hands are tied. As Thomas said, at this point there isn't much we can do.

I also want to implement the ability to the installer plugins to perform their own install routine.

@mbabker Would be more than qualified to respond to that as he has been fixing our installer. I am not sure we should be moving the installation routines into a plugin.

@izharaazmi
Copy link
Contributor

izharaazmi commented May 14, 2016

with the B/C promise our hands are tied.

Yeah, I understand this pain.

If you have few minutes to spare can you put some light on the B/C concerns regarding the installer component/plugin. I am quite not able to foresee them. Like not everyone is creating installer plugins. And if someone is indeed creating one, then won't it break as...

This PR rewrites the plugins so they return the data instead of directly outputting the tab. The tab is then generated by the Isis layout file.

Those would still try to directly output the tab and just return true and not any data!

@Bakual
Copy link
Contributor Author

Bakual commented May 14, 2016

After all we are going to have a major release.

3.6.0 is called a minor release. With the next major (J4) we can talk about it again 😄

I also want to implement the ability to the installer plugins to perform their own install routine.

Using com_ajax, it should be possible with the current plugins already,

Those would still try to directly output the tab and just return true and not any data!

The backward compatibility only is relevant for stuff that is already released. Since those plugins together with the new plugin event were merged only a few days ago and not yet released, we can still adjust to our liking. It's another case for the existing plugin events (they need to be properly deprecated and can be removed with J4.0.0) and the install from web plugin.

@izharaazmi
Copy link
Contributor

I also want to implement the ability to the installer plugins to perform their own install routine.

Using com_ajax, it should be possible with the current plugins already,

Rather I see that it is already possible with the Installer plugin events onInstallerBeforeInstallation, onInstallerBeforeInstaller and onInstallerAfterInstaller. 👍

@roland-d
Copy link
Contributor

@izharaazmi

Like not everyone is creating installer plugins. And if someone is indeed creating one, then won't it break as...

That we don't know and removing the triggers for before and after will then break their installation because they are no longer called if we remove them.

@izharaazmi
Copy link
Contributor

@roland-d In my last comment I didn't mean to remove those triggers. Rather I was happy to see what I wanted is already possible. 👍

@mbabker
Copy link
Contributor

mbabker commented May 14, 2016

There are already onExtension(Before|After)(Install|Update|Uninstall) events dispatched by JInstaller. IMO there shouldn't be events in the middle of the install process for general system use. The hooks for the installed extension to listen on are more than enough there.

@izharaazmi
Copy link
Contributor

izharaazmi commented May 14, 2016

onExtension(Before|After)(Install|Update|Uninstall) events are to notify the plugins that the extension is ready for / has completed the installation, where the installation is anyway being processed in the normal way.

My concern, which IMO is solved by the triggers onInstallerBeforeInstallation, onInstallerBeforeInstaller fired from the InstallerModelInstall::install().

To elaborate my concern (likely I am not clear enough and causing confusion) –
Assume I want to implement a new installtype apart from existing 'upload', 'url', 'folder' types, then I should be able to do that using installer plugins. That I see is already possible with above triggers fired from inside the model.

@mbabker Can you please explain a bit the "general system use"

@mbabker
Copy link
Contributor

mbabker commented May 14, 2016

That's a different thing altogether then. Make the PR if you want to see that happen.

Can you please explain a bit the "general system use"

Something like the preflight script hook shouldn't be a plugin event that anything can manipulate at any point ever. Just leave that to whatever extension is being installed.

@Bakual
Copy link
Contributor Author

Bakual commented May 14, 2016

Keep in mind that the installer plugins we talk about here are just different ways to get the extension zip file to the installer (uploading, specifying a server directory, from an URL or from JED). The install routine itself is a whole different thing and is the same for all currently four plugins.
As an example you could add a custom plugin (installer tab) which pulls the files from an own corporate server where you host your most used extensions, providing a dropdown list where you can choose which from the available extensions you want to install.

@izharaazmi
Copy link
Contributor

Thanks for the tip @Bakual 👍
I understand that the routine is ultimately same for all installtypes. And its hard to find anything that is not already covered by Joomla so far. I just assume that someone may have something in mind. I'll do the PR soon so that we can discuss more specific.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @810, @brianteeman, @roland-d


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@Bakual
Copy link
Contributor Author

Bakual commented May 26, 2016

I've updated this PR and moved the HTML/JS to a layout file. This allows templates to override the plugins output.
Based on a discussion in #10616 (comment)

@wilsonge
Copy link
Contributor

wilsonge commented Jun 8, 2016

I have tested this item ✅ successfully on 4abda6a

This now allows me to use the plugins as expected in hathor. Nice work :)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10450.

@wilsonge wilsonge merged commit 5190862 into joomla:staging Jun 8, 2016
@wilsonge wilsonge added this to the Joomla 3.6.0 milestone Jun 8, 2016
@Bakual Bakual deleted the BringInstallerPluginsToHathor branch June 9, 2016 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants