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

[5.0] Fixes the cli installer #42116

Merged
merged 4 commits into from
Oct 18, 2023
Merged

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Oct 10, 2023

Pull Request for Issue #42114 .

Summary of Changes

  • refactor the code so cli installation doesn't throw

Testing Instructions

Install joomla using cd installation and php joomla.php install.
Make sure that you set the public folder option --public-folder to a valid directory (or do not provide any options and follow the questions but provide a valid path for the public folder)

Actual result BEFORE applying this Pull Request

DB error

Expected result AFTER applying this Pull Request

No errors

Link to documentations

Please select:

  • No documentation changes for docs.joomla.org needed
  • No documentation changes for manual.joomla.org needed

@richard67 richard67 added the bug label Oct 10, 2023
@richard67
Copy link
Member

@dgrammatiko Shouldn't the testing instructions tell to use the public folder option --public-folder? As far as I understood the issue, the CLI installation broke only with using that option. Or was that wrong and it's broken also when being called without that opzion?

@dgrammatiko
Copy link
Contributor Author

Shouldn't the testing instructions tell to use the public folder option --public-folder?

Good call, fixed

@richard67
Copy link
Member

richard67 commented Oct 10, 2023

Shouldn't the testing instructions tell to use the public folder option --public-folder?

Good call, fixed

@dgrammatiko Where? I see no change in the description. Now it's there.

@dgrammatiko
Copy link
Contributor Author

@richard67 fwiw php joomla.php install without any options kicks an interactive set of questions, so the last question is about the public folder path. Leaving it empty skips the ceremony for the public folder...

@HLeithner HLeithner added this to the Joomla! 5.0.1 milestone Oct 10, 2023
@hans2103
Copy link
Contributor

@dgrammatiko
This is what I see on my local machine at the end of cd installation;php joomla.php install before applying the PR.
I do not have a DB error.

Relative or absolute path to the public folder []:
 > 

OK
Validating DB connection...OK
Creating and populating the database...OK
Writing configuration.php and additional setup ...OK

                                                                                                                        
 [OK] Joomla has been installed                                                                                         
                                                                                                                        

hans2103@192 installation % 

@rabidgrowth
Copy link

Thank for the PR! Confirm is work now! ✅

Hope this includes in 5.0 at next Tuesday. I will use this + Ansible. Make build server easy for me.

@HLeithner
Copy link
Member

this pr is scheduled for 5.0.1 thanks @rabidgrowth for the test

@ceford
Copy link
Contributor

ceford commented Oct 11, 2023

Clarification please: I unpacked the J5rc2 package and started from its installation folder. First try I answered all of the questions and entered the root folder:

/Users/ceford/Sites/j5rc2test

That ended in an error

OK
Validating DB connection...OK
Creating and populating the database...OK
Writing configuration.php and additional setup ...OK
Creating the public folder...
In PublicFolderGeneratorHelper.php line 112:
                                                                
  Unable to create the given folder, index.php already exists.                                                        

I did it again and left the public folder at the default [] and the installation went to completion with no errors. This is before applying the patch.

 Relative or absolute path to the public folder []:
 > 

OK
Validating DB connection...OK
Creating and populating the database...OK
Writing configuration.php and additional setup ...OK
                                                                                                                        
 [OK] Joomla has been installed

It has and works fine. So what error should I look for in the installed site?


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

@rabidgrowth
Copy link

@ceford You put all joomla files inside the folder /Users/ceford/Sites/j5rc2test, yes? Can not use same folder /Users/ceford/Sites/j5rc2test for public. Must use different folder. Example: /Users/ceford/Sites/rabidgrowth. After install ends the site is servered from /Users/ceford/Sites/rabidgrowth.

See my issue #42114 . I used folder outside from /var/www for joomla files, then folder inside /var/www for public.

Hope this is help for you!

@dgrammatiko
Copy link
Contributor Author

@hans2103 @ceford the CLI installation is broken only if the user provided a path for the public folder (and maybe the path needs to be outside of the root path) otherwise nothing is broken

@ceford
Copy link
Contributor

ceford commented Oct 11, 2023

I see now, the cli installer is used for creating a site within a site or multiple sites on the same server. That did not occur to me. Thanks for the clarification.

@ceford
Copy link
Contributor

ceford commented Oct 11, 2023

I have tested this item ✅ successfully on ec5c8d2

To test I unpacked the download in ~/tmp/joomla5 and followed the instructions. Without the patch I got the Database error and with the patch I got a working site in ~/Sites/j5testcli (I keep all my test sites in ~/Sites). I had not twigged this is a way to keep most of the php code outside the public folder.


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

@richard67
Copy link
Member

Thank for the PR! Confirm is work now! ✅

@rabidgrowth Could you mark your test result right? It's not enough just to add a comment with a green check mark. It needs to submit your test result in the issue tracker. For doing this, go to https://issues.joomla.org/tracker/joomla-cms/42116 , then use the blue "Test this" at the top left corner, then select your test result and finally submit. Thanks in advance.

@rabidgrowth
Copy link

I have tested this item ✅ successfully on ec5c8d2


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

@rabidgrowth
Copy link

Sorry, didn't know. I now marked test inside tracker.

@richard67 richard67 removed the bug label Oct 11, 2023
@joomla-cms-bot joomla-cms-bot removed this from the Joomla! 5.0.1 milestone Oct 11, 2023
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 11, 2023
@richard67 richard67 added the bug label Oct 11, 2023
@richard67 richard67 added this to the Joomla! 5.0.1 milestone Oct 11, 2023
@richard67
Copy link
Member

@ceford @rabidgrowth Thanks for testing.

@HLeithner HLeithner merged commit 3207bf6 into joomla:5.0-dev Oct 18, 2023
2 of 3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 18, 2023
@HLeithner
Copy link
Member

thanks, there is another problem I will create a PR for

@dgrammatiko dgrammatiko deleted the 5.0-dev-cli-inst branch October 18, 2023 18:53
@dgrammatiko
Copy link
Contributor Author

@HLeithner #42120 or another one?

@HLeithner
Copy link
Member

I talk about #42168 no fix for #42120 yet

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Oct 18, 2023

Nice but I think the error msg needs to be translatable

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

Successfully merging this pull request may close these issues.

7 participants