Skip to content

Comments

[5.0] Make the includes check easier to read#41053

Merged
HLeithner merged 8 commits intojoomla:5.0-devfrom
Digital-Peak:includes/simplify
Aug 21, 2023
Merged

[5.0] Make the includes check easier to read#41053
HLeithner merged 8 commits intojoomla:5.0-devfrom
Digital-Peak:includes/simplify

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Jun 26, 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

laoneo and others added 3 commits June 26, 2023 13:51
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
@sandewt
Copy link
Contributor

sandewt commented Jun 26, 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')) {

@laoneo
Copy link
Member Author

laoneo commented Jun 26, 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.

@sandewt
Copy link
Contributor

sandewt commented Jun 26, 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.

@laoneo
Copy link
Member Author

laoneo commented Jun 26, 2023

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

@sandewt
Copy link
Contributor

sandewt commented Jun 26, 2023

issue-test

@laoneo
Copy link
Member Author

laoneo commented Jun 26, 2023

Can you try with the latest fix?

@wilsonge
Copy link
Contributor

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

@laoneo
Copy link
Member Author

laoneo commented Jun 27, 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.

@sandewt
Copy link
Contributor

sandewt commented Jun 27, 2023

Can you try with the latest fix?

Works, expected result.

@laoneo
Copy link
Member Author

laoneo commented Jun 28, 2023

Can you mark it as a successful test?

@wilsonge
Copy link
Contributor

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

@laoneo laoneo changed the title Make the includes check easier to read [5.0] Make the includes check easier to read Jun 28, 2023
@sandewt
Copy link
Contributor

sandewt commented Jun 28, 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)

@laoneo
Copy link
Member Author

laoneo commented Jun 28, 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.

@Quy
Copy link
Contributor

Quy commented Aug 14, 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.

@laoneo
Copy link
Member Author

laoneo commented Aug 17, 2023

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

@HLeithner HLeithner merged commit 47573d2 into joomla:5.0-dev Aug 21, 2023
@HLeithner
Copy link
Member

thanks

@HLeithner HLeithner deleted the includes/simplify branch August 21, 2023 06:36
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.

7 participants