Skip to content

[4.0][CLI] Add a task to discover extensions#31524

Merged
HLeithner merged 23 commits intojoomla:4.0-devfrom
astridx:discover
Mar 18, 2021
Merged

[4.0][CLI] Add a task to discover extensions#31524
HLeithner merged 23 commits intojoomla:4.0-devfrom
astridx:discover

Conversation

@astridx
Copy link
Contributor

@astridx astridx commented Nov 29, 2020

Summary of Changes

i added a CLI task to discover extensions. I took #28666 as an example

Testing Instructions

  1. Apply this PR

  2. Copy or symlink an extension to your Joomla installation

  3. Run php ./cli/joomla.php extension:discover --eid=122 php ./cli/joomla.php extension:discoverinstall --eid=122 for discovering the extension with the ID 122
    or
    Run the command without a parameter (like this php ./cli/joomla.php extension:discover php ./cli/joomla.php extension:discoverinstall) for discovering all extensions, that are copied to Joomla but not installed.

  4. Run php ./cli/joomla.php list or only php ./cli/joomla.php and check the entry in the list.

  5. Run php ./cli/joomla.php help extension:discover php ./cli/joomla.php help extension:discoverinstallto check the help information.

Documentation Changes Required

I would add it here: https://docs.joomla.org/J4.x:CLI_Update

@richard67
Copy link
Member

@astridx Could you check and fix PHP code style errors reported by drone here https://ci.joomla.org/joomla/joomla-cms/37913/1/6? Thanks in advance

@astridx
Copy link
Contributor Author

astridx commented Nov 29, 2020

@laoneo @richard67 Thank you. I fixed the copy and pasted mistakes.

@richard67
Copy link
Member

@astridx Thanks. It might be that system tests are failing in drone for your PR, but this is very likely unrelated. But if you want to have it nice, you can update your branch to latest 4.0-dev of the CMS, then the system tests will pass and all will be green, which looks nicer ;-)

@astridx
Copy link
Contributor Author

astridx commented Nov 29, 2020

Now it is nice and green :)

Co-authored-by: Brian Teeman <brian@teeman.net>
@toivo
Copy link
Contributor

toivo commented Nov 29, 2020

I have tested this item ✅ successfully on 4a1e74f

Tested successfully in Beta6-dev of 29 November


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

@brianteeman
Copy link
Contributor

Does this find the extensions or does it install them?

@wilsonge
Copy link
Contributor

@brianteeman is right - can we name this to discover install - because we should add a separate discover task which covers the 'refresh' task in github https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/controllers/discover.php#L26-L33

@toivo
Copy link
Contributor

toivo commented Nov 30, 2020

I have tested this item ✅ successfully on b4ffab6

Tested successfully in Beta6-dev and PHP 8.0.

Copied the following free 3rd party extensions to modules and the PR detected and installed them:
Mini Frontpage from https://extensions.joomla.org/extension/news-display/articles-display/mini-frontpage/
Rapid Contact from https://extensions.joomla.org/extension/contacts-and-feedback/contact-forms/rapid-contact/


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

*/
public function processDiscover($eid): bool
{
$jInstaller = new Installer;
Copy link
Member

Choose a reason for hiding this comment

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

Can you also inject the installer the same way as the database.

Copy link
Contributor

Choose a reason for hiding this comment

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

Installer isn’t available in the container. So I think we should make it optionally injectable in the constructor for testing. But still fine to run the init in the constructor if nothing is passed in

Copy link
Member

Choose a reason for hiding this comment

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

You can create the instance in the serviceprovider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laoneo @wilsonge
I would like to leave it that way in this PR because it is implemented in the same way in other places.
For example here:

$jInstaller = Installer::getInstance();

I think it is confusing when things are different in a CMS. I agree with you that it should be changed. But then in all places. I would do this in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I can do then a followup pr.

@astridx
Copy link
Contributor Author

astridx commented Dec 1, 2020

@brianteeman is right - can we name this to discover install - because we should add a separate discover task which covers the 'refresh' task in github https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/controllers/discover.php#L26-L33

@wilsonge I don't understand what you mean with this.

@laoneo
Copy link
Member

laoneo commented Dec 12, 2020

I guess he means to rename the class to ExtensionDiscoverInstallCommand.

@laoneo
Copy link
Member

laoneo commented Dec 22, 2020

Renamed the classes, can be tested now.

@HLeithner
Copy link
Member

Joomla 4.0 is in feature freeze can you please rebase this on j4.1 or I can do this for you, thanks

@laoneo
Copy link
Member

laoneo commented Dec 22, 2020

This is not a feature, it is an enhancement. And I need it for testing extensions in j4. So no way to rebase to 4.1.

@HLeithner
Copy link
Member

And I need joomla 4 ready to ship

@laoneo
Copy link
Member

laoneo commented Dec 22, 2020

And postponing this to 4.1 will help?

@laoneo
Copy link
Member

laoneo commented Dec 22, 2020

Ok, I give up. Do whatever you want with it. I'm building it on my own

ExtensionDiscoverInstall renamed to ExtensionDiscoverInstallCommand
@alikon
Copy link
Contributor

alikon commented Dec 23, 2020

after the latest commit this is needed ExtensionDiscoverInstall renamed to ExtensionDiscoverInstallCommand
see astridx#3

@alikon
Copy link
Contributor

alikon commented Dec 23, 2020

I have tested this item ✅ successfully on 1381942


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

@HLeithner HLeithner merged commit d58de56 into joomla:4.0-dev Mar 18, 2021
@HLeithner
Copy link
Member

thanks and sorry

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.

10 participants