PR-5.0-dev Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
26 Jun 2023

Summary of Changes

Makes the check if a configuration exists in the include files easier to read as it returns early when a condition doesn't exist.

Testing Instructions

  • Remove configuration.php
  • Go to the root page of the joomla installation
  • Remove installation folder
  • Go to the root page of the joomla installation

Actual result BEFORE applying this Pull Request

  • The installation screen is shown
  • "No configuration file found and no installation code available. Exiting..." message is shown

Expected result AFTER applying this Pull Request

  • The installation screen is shown
  • "No configuration file found and no installation code available. Exiting..." message is shown

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

avatar joomla-cms-bot joomla-cms-bot - change - 26 Jun 2023
Category Administration
avatar laoneo laoneo - open - 26 Jun 2023
avatar laoneo laoneo - change - 26 Jun 2023
Status New Pending
avatar laoneo laoneo - change - 26 Jun 2023
Labels Added: PR-5.0-dev
avatar sandewt
sandewt - comment - 26 Jun 2023

Proposal: If you change the following code, both files are identical except for line 4 (administator / site).

Change this code

define('JDEBUG', $config->debug);

to that of

if (!defined('JDEBUG')) {

avatar laoneo
laoneo - comment - 26 Jun 2023

I guess this is done on purpose as sites can include the index.php in an own script and then define that variable. If you want to change this, then please in another pr, but I'm not sure about the consequences, perhaps @wilsonge can shed some light into this.

avatar sandewt sandewt - test_item - 26 Jun 2023 - Tested unsuccessfully
avatar sandewt
sandewt - comment - 26 Jun 2023

I have tested this item ? unsuccessfully on b129c8d

Remove configuration.php - done
Go to the root page of the joomla installation - done
Error message appears: The requested URL was not found on this server.


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

avatar laoneo
laoneo - comment - 26 Jun 2023

Can you post a screenshot of the error message and the url you have opened and the url you get redirected?

avatar sandewt
sandewt - comment - 26 Jun 2023

issue-test

avatar laoneo
laoneo - comment - 26 Jun 2023

Can you try with the latest fix?

avatar wilsonge
wilsonge - comment - 26 Jun 2023

I guess this is done on purpose as sites can include the index.php in an own script and then define that variable. If you want to change this, then please in another pr, but I'm not sure about the consequences, perhaps @wilsonge can shed some light into this.

I can't shed any proper light. I mean it's always been intended that you can override defines.php rather than index.php at the top level of each app (https://github.com/joomla/joomla-cms/blob/4.3-dev/includes/app.php#L16-L23) so I assume this allows setting JDEBUG there. No clue why you'd choose to set it there. But making it consistent also makes sense to me.

API should be included in this PR as it's based on administrator https://github.com/joomla/joomla-cms/blob/4.3-dev/api/includes/framework.php

avatar laoneo
laoneo - comment - 27 Jun 2023

Should then API get the same logic as admin and site got in #40509? If yes, can you comment on the original pr? I will then update this one when the rest is done.

avatar sandewt
sandewt - comment - 27 Jun 2023

Can you try with the latest fix?

Works, expected result.

avatar laoneo
laoneo - comment - 28 Jun 2023

Can you mark it as a successful test?

avatar wilsonge
wilsonge - comment - 28 Jun 2023

Should then API get the same logic as admin and site got in #40509? If yes, can you comment on the original pr? I will then update this one when the rest is done.

Ahh we error out instead of redirecting - one day I'll read code correctly the first time. Yeah just ignore me and I'll get back into my box

avatar laoneo laoneo - change - 28 Jun 2023
Title
Make the includes check easier to read
[5.0] Make the includes check easier to read
avatar laoneo laoneo - edited - 28 Jun 2023
avatar sandewt
sandewt - comment - 28 Jun 2023

Can you mark it as a successful test?

Why these two files? Only file 1 seems to be addressed in this test. I access the root page of the joomla installation through the administrator (administrator/index.php?option=com_joomlaupdate). Or am I not testing it correctly?

C:\xampp\htdocs\bugtesting5\joomla\administrator\includes\framework.php (file 1)

C:\xampp\htdocs\bugtesting5\joomla\includes\framework.php (file 2)

avatar laoneo
laoneo - comment - 28 Jun 2023

YOu can also test the second file when you go to the administrator subfolder and do the same. You can do it either from the joomla root or the administrator subfolder.

avatar Quy Quy - test_item - 14 Aug 2023 - Tested successfully
avatar Quy
Quy - comment - 14 Aug 2023

I have tested this item ✅ successfully on 76d6a17


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

avatar laoneo
laoneo - comment - 17 Aug 2023

@sandewt can you give this one a test as well?

avatar HLeithner HLeithner - change - 21 Aug 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-08-21 06:35:56
Closed_By HLeithner
avatar HLeithner HLeithner - close - 21 Aug 2023
avatar HLeithner HLeithner - merge - 21 Aug 2023
avatar HLeithner
HLeithner - comment - 21 Aug 2023

thanks

Add a Comment

Login with GitHub to post a comment