Skip to content

[5.3] add CORS settings when install from CLI#45129

Merged
rdeutz merged 5 commits intojoomla:5.3-devfrom
alikon:patch-26
Mar 21, 2025
Merged

[5.3] add CORS settings when install from CLI#45129
rdeutz merged 5 commits intojoomla:5.3-devfrom
alikon:patch-26

Conversation

@alikon
Copy link
Contributor

@alikon alikon commented Mar 14, 2025

Pull Request for Issue # .
follow-up from #45103

Summary of Changes

added CORS settings

Testing Instructions

install joomla from cli
php installation/joomla.php
check the configuration.php

Actual result BEFORE applying this Pull Request

in the configuration.php
these values are missed

public $cors = false;
public $cors_allow_origin = '*';
public $cors_allow_headers = 'Content-Type,X-Joomla-Token';
public $cors_allow_methods = '';

Expected result AFTER applying this Pull Request

these values are present

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@alikon alikon marked this pull request as ready for review March 14, 2025 09:41
@laoneo
Copy link
Member

laoneo commented Mar 14, 2025

I have tested this item ✅ successfully on 6ef8574

Settings are now written with DPDocker site installation


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

@krishnagandhicode
Copy link
Contributor

I have tested this item ✅ successfully on 6ef8574

Tested successfully.

@alikon The site is working fine after applying the PR. However, the PR description mentions checking configuration.php, but the actual changes are in ConfigurationModel. Please confirm if this is the intended behavior or if configuration.php should also reflect the changes.


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

@alikon
Copy link
Contributor Author

alikon commented Mar 14, 2025

@krishnaGandhi11 exaclty,
before this pr when you install joomla from CLI
those values were not written in the configuration.php file
with the pr those values are written as well

i hope i've answered your question

@alikon
Copy link
Contributor Author

alikon commented Mar 14, 2025

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 14, 2025
@krishnagandhicode
Copy link
Contributor

@krishnaGandhi11 exaclty, before this pr when you install joomla from CLI those values were not written in the configuration.php file with the pr those values are written as well

i hope i've answered your question

Thank you for your response, @alikon.

I truly appreciate your time and clarification. I just wanted to ensure there is no misunderstanding—the changes in this PR are applied in ConfigurationModel.php, while the PR description mentions checking configuration.php. Could you kindly confirm if this was a miscommunication in the description? If so, updating it to reflect ConfigurationModel.php instead of configuration.php might help avoid any confusion for future reviewers and testers.

@alikon
Copy link
Contributor Author

alikon commented Mar 14, 2025

there is no misunderstanding
now i'm in doubt if you have undertood the issue and tested it correctly or simply used an AI
sorry to be blunt

@brianteeman
Copy link
Contributor

The description is correct. If you don't understand it how could you mark a successful test?

@alikon alikon removed the RTC This Pull Request is Ready To Commit label Mar 14, 2025
@alikon
Copy link
Contributor Author

alikon commented Mar 14, 2025

real human test still needed


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

@krishnagandhicode
Copy link
Contributor

there is no misunderstanding now i'm in doubt if you have undertood the issue and tested it correctly or simply used an AI sorry to be blunt

No, I was just confirming what I observed.

My-results.mp4

@brianteeman
Copy link
Contributor

That video confirms you have not followed/ understood the instructions

@brianteeman
Copy link
Contributor

I have removed the test result from krishnaGandhi11

@alikon
Copy link
Contributor Author

alikon commented Mar 14, 2025

so what you have observed before
and after applying the pr ?
from your video i'm still not satisfied or it's not clear to my poor "human" mind

@krishnagandhicode
Copy link
Contributor

krishnagandhicode commented Mar 14, 2025

That video confirms you have not followed/ understood the instructions

Then I really apologize, i really posted response according what i observed.

@brianteeman
Copy link
Contributor

@krishnaGandhi11 you did not follow the test instructions

  1. Install joomla using the command line interface (cli) using the command php installation/joomla.php
  2. after installing joomla look in the configuration.php that is created by the installation of joomla in the ROOT of your web space
  3. observe that the CORS related lines are not present
  4. apply patch and repeat steps 1 and 2
  5. observe that the CORS related lines are present

@krishnagandhicode
Copy link
Contributor

@krishnaGandhi11 you did not follow the test instructions

  1. Install joomla using the command line interface (cli) using the command php installation/joomla.php
  2. after installing joomla look in the configuration.php that is created by the installation of joomla in the ROOT of your web space
  3. observe that the CORS related lines are not present
  4. apply patch and repeat steps 1 and 2
  5. observe that the CORS related lines are present

Thank you for the clarification, @brianteeman.

I sincerely apologize for the mistake. I now realize I misunderstood the importance of using the CLI and made an error in my testing.

I'll carefully follow the exact steps and will try to retest.

@brianteeman
Copy link
Contributor

It couldn't have been more clear. Install from CLI was in the title!!

@krishnagandhicode
Copy link
Contributor

It couldn't have been more clear. Install from CLI was in the title!!

Yes I totally agree, that's my bad.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 6ef8574


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

@krishnagandhicode
Copy link
Contributor

Before :
image


After :
image

I guess now i am on right path now, please lemme know.

@brianteeman
Copy link
Contributor

brianteeman commented Mar 14, 2025

PLEASE read more carefully

The first step is to INSTALL joomla using the CLI script
The second step is to check the values present in configuration.php in the root of your web space - NOT the values in a completely different file

What you did was just display the default help for the CLI script AND look at the values in installation/configuration.php-dist

@QuyTon
Copy link
Contributor

QuyTon commented Mar 14, 2025

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 14, 2025
@krishnagandhicode
Copy link
Contributor

configuration.php

PLEASE read more carefully

The first step is to INSTALL joomla using the CLI script The second step is to check the values present in configuration.php in the root of your web space - NOT the values in a completely different file

What you did was just display the default help for the CLI script AND look at the values in installation/configuration.php-dist

Please have a look whenever you have some time(considering the PR is at RTC), and lemme know if there is still something wrong, So that I don't mess up again. Thank-you for your patience and guidance.
image

@brianteeman
Copy link
Contributor

@krishnaGandhi11 where does it say anywhere that you should copy the file installation/configuration.php-dist to /configuration.php

Step away from the computer and come back tomorrow and then follow the instructions you have been given. Don't add something because you think it is missing, its not

@krishnagandhicode
Copy link
Contributor

I have tested this item ✅ successfully on 6ef8574Tested successfully.

@alikon The site is working fine after applying the PR. However, the PR description mentions checking configuration.php, but the actual changes are in ConfigurationModel. Please confirm if this is the intended behavior or if configuration.php should also reflect the changes.

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

@krishnaGandhi11 where does it say anywhere that you should copy the file installation/configuration.php-dist to /configuration.php

Step away from the computer and come back tomorrow and then follow the instructions you have been given. Don't add something because you think it is missing, its not

I see that this PR is already RTC, but I wanted to complete the testing process properly. I appreciate the comments and feedback from Mr. Brian and Mr. Alikon—thank you for your help and guidance.

PR45129.1.2.mp4

@brianteeman
Copy link
Contributor

You are still NOT installing joomla from the CLI

@brianteeman
Copy link
Contributor

Read The Fine Manual
https://docs.joomla.org/J4.x:Joomla_CLI_Installation

@krishnagandhicode
Copy link
Contributor

Read The Fine Manual https://docs.joomla.org/J4.x:Joomla_CLI_Installation

This manual was really helpful - please have a look!

Final.CLI.mp4

@rdeutz rdeutz enabled auto-merge (squash) March 21, 2025 16:27
@rdeutz rdeutz merged commit 628b844 into joomla:5.3-dev Mar 21, 2025
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 21, 2025
@rdeutz
Copy link
Contributor

rdeutz commented Mar 22, 2025

Thanks

@alikon alikon deleted the patch-26 branch March 22, 2025 17:13
@bembelimen bembelimen modified the milestone: Joomla! 5.3.4 Sep 20, 2025
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