Skip to content

[6.0] Github Codespaces Patch#45961

Open
mahmoudmagdy1-1 wants to merge 3 commits intojoomla:6.0-devfrom
mahmoudmagdy1-1:gh-codespaces-patch
Open

[6.0] Github Codespaces Patch#45961
mahmoudmagdy1-1 wants to merge 3 commits intojoomla:6.0-devfrom
mahmoudmagdy1-1:gh-codespaces-patch

Conversation

@mahmoudmagdy1-1
Copy link

@mahmoudmagdy1-1 mahmoudmagdy1-1 commented Aug 21, 2025

Pull Request for Issue # .

Summary of Changes

This PR is a quick patch for github codespaces feature, #45719

  1. it provides https on port 443, which is needed for some joomla features and it caused Mfa.cy.js tests to fail
image 2. readds `rm configuration.php` to rebuild container without any errors 3. Adds error logging for php

Testing Instructions

see joomla/Manual#496

Actual result BEFORE applying this Pull Request

Mfa.cy.js tests fail, error when rebuilding the container (it says configuation.php already exists), no error logs for php

Expected result AFTER applying this Pull Request

Mfa.cy.js tests pass and https support is added, no errors when rebuilding the container, error logs available for php

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: Github Codespaces Patch Manual#505

  • No documentation changes for manual.joomla.org needed

@laoneo
Copy link
Member

laoneo commented Aug 22, 2025

Would this mean that it works only on https? Does it also need an update of the manual?

@mahmoudmagdy1-1
Copy link
Author

Would this mean that it works only on https?

It was working only on http (port 80), with this pr it works on both http (port 80) and https (port 443)

Does it also need an update of the manual?

I guess so, just a few lines to clarify.
I'll make a pr for that later today

@alikon
Copy link
Contributor

alikon commented Aug 22, 2025

the Mfa.cy.js test now doesn't fail anymore
there are still 2 failure about Router.cy.js see comment #45719 (comment)

what i've noticed is that the configuration.php is not writable so you cannot change some settings in Global configuration
image

@bembelimen bembelimen added the bug label Aug 22, 2025
@LadySolveig
Copy link
Contributor

LadySolveig commented Aug 22, 2025

Do we need port 3306 for MySQL to the outside? I think it is only needed inside the docker network at the moment. Personally, I see it as best practice to only expose the ports to the outside that are really needed from the outside. Not a bug but maybe a consideration

@mahmoudmagdy1-1
Copy link
Author

mahmoudmagdy1-1 commented Aug 22, 2025

Just added a couple of lines in the docs for this, joomla/Manual#505

there are still 2 failure about Router.cy.js

I changed them like your comment said, but this change makes the CI checks fail on this repo, so I might need to revert those changes and just ignore Router.cy.js for now?

image

now we got all Cypress tests passing successfully

what i've noticed is that the configuration.php is not writable so you cannot change some settings in Global configuration

After some debugging, I found out it was caused by SiteUpDown.cy.js and cli/joomla.php site commands

@mahmoudmagdy1-1
Copy link
Author

Do we need port 3306 for MySQL to the outside?

Yes, otherwise users can't execute commands from the codespace terminal like in the image below, and in any way I don't think there is a down side from having them exposed, as a github codespace is only accessible to the github account who made the codespace and also because it's a testing environment

image

Thank you for the suggestion

@alikon
Copy link
Contributor

alikon commented Aug 25, 2025

what i've noticed is that the configuration.php is not writable so you cannot change some settings in Global configuration

After some debugging, I found out it was caused by SiteUpDown.cy.js and cli/joomla.php site commands

yes without running the test suite i'm able to write the configuration.

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.

6 participants