Skip to content

[4.0] CLI commands#28666

Merged
wilsonge merged 34 commits intojoomla:4.0-devfrom
alikon:patch-105
May 2, 2020
Merged

[4.0] CLI commands#28666
wilsonge merged 34 commits intojoomla:4.0-devfrom
alikon:patch-105

Conversation

@alikon
Copy link
Contributor

@alikon alikon commented Apr 12, 2020

Pull Request for Issue #21452 (comment)
all credit to @bosunski

Summary of Changes

porting from #21452
Added Commands for:

  • Checking Updates
  • Core Update
  • Listing Extensions with type specification
  • Setting Configurations with setting multiple Configuration support
  • Listing Configurations (includes listing configurations as a group)
  • Setting website to offline
  • Bringing website online
  • Installing Extension
  • Removing Extension

Testing Instructions

All testing instruction can be found in the documentation https://docs.joomla.org/J4.x:CLI_Update

Expected result

Actual result

N/A

Documentation Changes Required

remove the install part

@richard67
Copy link
Member

@alikon At a first look it seems to me that the database section of the get configuration command doesn't include the options for database connection encryption.

@alikon
Copy link
Contributor Author

alikon commented Apr 12, 2020

added php cli/joomla.php config:get --group db

@richard67
Copy link
Member

@alikon I've started to test a bit and it goes well up to now. But there is a mismatch between help text and how it really works for the config:get, and maybe we have that elswehere, too.

The help says Usage: php ./cli/joomla.php config:set <option> <value>, but you have to specify in fact <option>=<value>.

j4-pr-28666_error-config-set

@richard67
Copy link
Member

richard67 commented Apr 12, 2020

Hmm, php ./cli/joomla.php config:set dbencryption=1 seems not to work. I don't get any error message, but php ./cli/joomla.php config:get still shows dbencryption 0 after that.

Update: Same with other parameters like e.g. frontediting.

* @since 4.0
*/
const DB_GROUP = ['name' => 'db', 'options' => ['dbtype', 'host', 'user', 'password', 'dbprefix', 'db']];
const DB_GROUP = [
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Class constants should have visibility modifiers, this isn't PHP 5 compatible code anymore.

  2. The group concept really should be a pluggable thing. As is this feature is only available for first party code with no way for third party to use it, and last I checked the global configuration form was built the same way as every other form meaning it offers events for plugins to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't get point 2

Copy link
Member

@richard67 richard67 Apr 13, 2020

Choose a reason for hiding this comment

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

@alikon I think with point 2 @mbabker meant that the list of configuration groups should not be hardcoded only, there should be a way for 3rd party components to register their group somehow so it can be used with the CLI here, too. I.e. like you now can list the options for the "db" you could list e.g. "akeebabackup" group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it may be done later....
anyway the branch is open...

*
* @since 4.0
*/
public function retrieveOptionsFromInput($options)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. This method has no reason to be public.

  2. This method has no reason to exit, that makes this entire command untestable (well, a lot of Joomla is the same, nothing new there)

  3. The registry API was updated in 4.0 to use type safe values (i.e. a boolean for a boolean parameter), this command (and probably others) is blissfully ignorant of the form definition and the types that each option should have, that is a concern because public $offline = 'true'; !== public $offline = true;.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be addressed now i hope

@alikon
Copy link
Contributor Author

alikon commented Apr 12, 2020

@richard67
there is not such a setting like frontediting in configuration.php
Screenshot from 2020-04-12 20-29-46

for the config:set dbencryption=1 command
i'm unable to reproduce your findings

Screenshot from 2020-04-12 20-31-50

Screenshot from 2020-04-12 20-34-51

@richard67
Copy link
Member

richard67 commented Apr 12, 2020

@alikon You will not get this parameter frontediting on a newly installed J4. But if you login to backend and just save the Global Configuration, the parameter will be there;

richard@vmkubu02:~/lamp/public_html/joomla-cms-4.0-dev$ php ./cli/joomla.php config:get frontediting
 -------------- ------- 
  Option         Value  
 -------------- ------- 
  frontediting   1      
 -------------- ------- 

richard@vmkubu02:~/lamp/public_html/joomla-cms-4.0-dev

It’s one of these parameters obviously missing in the default configuration.php.

@richard67
Copy link
Member

@alikon I again tested and it is still the same for php ./cli/joomla.php config:set dbencryption=1 here: I don't get the green OK message, and the value isn't changed.

@richard67
Copy link
Member

Maybe it’s because I’m on Linux and you’re on Windows?

@alikon alikon requested a review from richard67 April 13, 2020 10:19
@richard67
Copy link
Member

richard67 commented Apr 13, 2020

@alikon Are you sure you want my review? Would Michael not be better? My PHP skills are limited. Anyway I'll have a look later and will test.

@alikon
Copy link
Contributor Author

alikon commented Apr 13, 2020

i've made a full rewrite of the SetConfigurationCommand
reusing existing core code where possible and added the encrypted db connection settings
@richard67 i need you for the encrypted db connection part

P.S @mbabker can you have another look please

@richard67
Copy link
Member

@alikon How can I help you with the dB encryption part? Just review? Or is there something else to be done? Or you need some testing instructions for that? Let me know and I’ll try to help.

Beside this, as my posts above for the frontediting parameter have shown, we have configuration parameters not going into the configuration.php until you save global configuration in backend for the first time. Not an issue of this PR. Question is shall this PR handle that somehow, and shall someone open an issue about that?

@alikon
Copy link
Contributor Author

alikon commented Apr 13, 2020

How can I help you with the dB encryption part?

reviewing and testing as you were the writers for that, who better than you ? 😄

consider that at the GSoC 2018 time that part wasn't in the core yet

as my posts above for the frontediting parameter have shown, we have configuration parameters not going into the configuration.php until you save global configuration in backend

should be addressed now with the full rewrite

@astridx
Copy link
Contributor

astridx commented Apr 13, 2020

I just pulled this branch and typed the command
php ./cli/joomla.php config:set dbencryption=1.

Is this the same message you see @richard67
Screenshot from 2020-04-13 14-30-09
?
The command
php ./cli/joomla.php config: set list_limit = 120
was OK, expect the permissions of the configuration.php are changed.

Before: -rw-r--r-- 1 astrid astrid 3433 Apr 13 13:11 configuration.php
After: -r--r--r-- 1 astrid astrid 2158 Apr 13 14:33 configuration.php

@richard67
Copy link
Member

@astridx No, that's not what I had, and it has nothing to do with this PR. It is desired behavior that you can't use database connection encryption when using localhost, because localhost defaults to a socket connection for the mysql clients, and database connection encryption doesn't work when using a socket connection. Therefore you will receive the same error message when trying to do that configuration in backend or when doing a new installation. The solution: Use "127.0.0.1" (or "::1" on an IP-V6 only network) to enforce a TCP connection, then you can also use encrypted connections to the local host ;-)

@richard67
Copy link
Member

@astridx So for the reason stated in my previous comment, this is a useful good test: The implementation in this PR handles that attempt for a malconfiguration in the right way and shows the right error messages.

@richard67
Copy link
Member

site:down and site:up are working now.

@richard67
Copy link
Member

richard67 commented Apr 19, 2020

@alikon It seems the help text and how it works again became inconsistent, at least for the config:get with the group option:
pr-28666-config-groups

Help text tells to use =, but it needs to use a space.

P.S.: For config:set it is ok, i.e. there documentation and function are consistent with = to be used.

@richard67
Copy link
Member

richard67 commented Apr 19, 2020

@alikon Setting database connection encryption options works well here.

Hint for other testers: Beside the default (dbencryption=0), anything else than dbencryption=1 and dbsslverifyservercert=false is tricky to test due to the requirements of the particular database server and client regarding e.g. server name matching the name in the server certificate or ownership and permissions of certificate and key files. There is no documentation for that feature on the Joomla Docs website, but information on how database connection encryption options can be tested and links to documentation sites of the particular database types can be found in PR #27351 .

I've tested all options here with success. Spefiyfing empty values where possible clears that option to an empty string, e.g. for the dbsslcipher to change a previously entered value to empty.

@richard67
Copy link
Member

@alikon For the help text inconsistency I've mentioned 2 comments above see alikon#68.

@richard67
Copy link
Member

Another hint for testers: If you successfully have tested a core update to nightly build (with having patched libraries/src/Version.php before, if necessary, so that an update was found), then after the update the changes from this pull request (PR) are lost of course, so to continue to test it needs to apply the PR again after the update.

@toivo
Copy link
Contributor

toivo commented Apr 19, 2020

In my test of config on Wampserver group=mail works all right. The only issue I found was with smtpsecure=tls when Global Configuration has STARTTLS. I am using Gmail as SMTP server. All the other functions were working perfectly.


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

@richard67
Copy link
Member

richard67 commented Apr 19, 2020

@toivo Maybe tls doesn't work with self-signed certificate? Sorry, I misunderstood.

@toivo
Copy link
Contributor

toivo commented Apr 19, 2020

I was trying to update the documentation but may have messed it up because nothing changed. Will still investigate how to do that properly.


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

@richard67
Copy link
Member

One thing I am missing: There is a function to check for extension updates, but not for doing the extension update after an update for an extension has been found. This is not a mistake in this PR, it is also not in the specification for the original PR. If possible this function should be added with a new, future PR after this one here has been tested and finally been merged.

Fix help text of GetConfigurationCommand for group parameter
Copy link
Member

@richard67 richard67 left a comment

Choose a reason for hiding this comment

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

LGTM

@richard67
Copy link
Member

@toivo Can we consider the results you have posted above as a good test?

If so, could you mark your test result on the issue tracker here https://issues.joomla.org/tracker/joomla-cms/28666?

Just select the "Test this" button in the top left area of that page, then select the appropriate test result, e.g. "Tested successfully", with the check box for that result, and then finally use the "Submitt tes result" button a bit below.

Thanks for your contribution.

@richard67
Copy link
Member

P.S. The last change was just a change of the help text, so previous tests are still valid.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 953c70f

Tested all commands listed in the description.

Tested in detail database configuration parameter changes, especially for connection encryption.

Hints for other testers about how I tested, e.g. about patching CMS version so that an update is found, you can find in my previous comments.


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

@toivo
Copy link
Contributor

toivo commented Apr 19, 2020

I have tested this item ✅ successfully on 953c70f

Tested ok


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 19, 2020
@richard67
Copy link
Member

@alikon Is the revision comment by @wilsonge above resolved #28666 (comment)? if not: Could that be done with a future PR, or should I remove RTC and wait for changes?

@toivo
Copy link
Contributor

toivo commented Apr 20, 2020

BTW, the last versions in Nightly Builds are from 14 April. Are they not generated daily or is the daemon broken?


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

@richard67
Copy link
Member

BTW, the last versions in Nightly Builds are from 14 April. Are they not generated daily or is the daemon broken?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28666.

@toivo They should be generated every night. I can confirm that the nightly build of tonight doesn't contain changes from yesterday which it should contain. I'll inform the CMS maintenance team.

@alikon
Copy link
Contributor Author

alikon commented Apr 20, 2020

@alikon Is the revision comment by @wilsonge above resolved #28666 (comment)? if not: Could that be done with a future PR, or should I remove RTC and wait for changes?

i think it could be done in a future PR, but i'll left to the RL the last word

@richard67
Copy link
Member

i think it could be done in a future PR, but i'll left to the RL the last word

I think too it can be done with a future PR.

@wilsonge wilsonge merged commit a0d4888 into joomla:4.0-dev May 2, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 2, 2020
@wilsonge
Copy link
Contributor

wilsonge commented May 2, 2020

This will do as a starting point. Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone May 2, 2020
@alikon alikon deleted the patch-105 branch May 2, 2020 13:32
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
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.

8 participants