PR-5.0-dev Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
29 Apr 2023

Pull Request for Issue #40503 .

Summary of Changes

When installing Joomla on a different database server than localhost, a check is done to ensure that you are really the person having control over the server. This check writes a file to the filesystem and the content is empty. The code called File::write() with the string-literal, which is not valid, since you can't hand over a string-literal by reference. This PR first creates a variable, which then is handed over as the content for File::write().

Testing Instructions

Install Joomla 5.0-dev on a system and don't use localhost as your database server. Alternatively modify installation/src/Helper/DatabaseHelper.php in checkRemoteDbHost() to execute the check even though you are installing to localhost.

Actual result BEFORE applying this Pull Request

You get a red warning in the browser.

Expected result AFTER applying this Pull Request

Installation passes without error messages.

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 - 29 Apr 2023
Category Installation
avatar Hackwar Hackwar - open - 29 Apr 2023
avatar Hackwar Hackwar - change - 29 Apr 2023
Status New Pending
avatar Hackwar Hackwar - change - 29 Apr 2023
Labels Added: PR-5.0-dev
avatar HLeithner
HLeithner - comment - 29 Apr 2023

Should be fixed in 4.3 IR 4.4

avatar brianteeman
brianteeman - comment - 29 Apr 2023

This used to work. What broke it?

avatar sandewt
sandewt - comment - 30 Apr 2023

I cannot argue the following, but shouldn't the solution be found here? $buffer = ?

public static function write($file, $buffer, $useStreams = false)

avatar richard67
richard67 - comment - 30 Apr 2023

I cannot argue the following, but shouldn't the solution be found here? $buffer = ?

public static function write($file, $buffer, $useStreams = false)

@sandewt Your link points to the file of the filesystem library of the CMS, but the file modified by this PR uses the filesystem library from the framework, which cannot be found in the sources here but in the libraries/vendor folder of a running site.

avatar richard67
richard67 - comment - 30 Apr 2023

@sandewt P.S. to my previous comment: You can the source here https://github.com/joomla-framework/filesystem/blob/2.0-dev/src/File.php#L219 .

avatar richard67
richard67 - comment - 30 Apr 2023

This used to work. What broke it?

@brianteeman It was PR #40257 .

@Hackwar As your PR #40257 had been merged into the 4.4-dev branch and we have the same code there https://github.com/joomla/joomla-cms/blob/4.4-dev/installation/src/Helper/DatabaseHelper.php#L370 , this PR here needs to be rebased to 4.4-dev. Thanks in advance.

avatar brianteeman
brianteeman - comment - 30 Apr 2023

and thats why I keep banging on about establishing why something that did work now no longer works before writing a fix

avatar richard67
richard67 - comment - 30 Apr 2023

and thats why I keep banging on about establishing why something that did work now no longer works before writing a fix

@brianteeman Yes, you banged, and so I got curious and checked. That's what makes Joomla great, people work together.

avatar Hackwar
Hackwar - comment - 30 Apr 2023

The problem is the same as with the cookies in 4.3.0. people changed code without looking properly. Mostly it is the issue of the CMS having classes which we also have in the framework and those diverging from another. And yes, we want to use the framework classes, because they are generally of higher quality than the CMS ones. It's been the work of the last few weeks from me to remove the CMS classes from execution to prevent crap like this and yes, this showed some issues.

avatar richard67
richard67 - comment - 30 Apr 2023

@Hackwar I can reproduce the issue also on the 4.4-dev branch. It was introduced with your PR #40257 because that changed from the CMS lib, which uses the parameter by value, to the framework lib, which uses the parameter by reference. So this PR here definitely needs to be rebased to 4.4-dev (or replaced by a new one for that branch if that is easier).

avatar Hackwar
Hackwar - comment - 30 Apr 2023

We actually have to apply the fix which was done to the CMS class a long time ago also to the framework class, removing the &

avatar richard67
richard67 - comment - 30 Apr 2023

We actually have to apply the fix which was done to the CMS class a long time ago also to the framework class, removing the &

@Hackwar As that might be a b/c break it might go into framework 3 and so Joomla 5, so one reason more to make the fix from this PR here in the 4.4-dev branch.

avatar Hackwar
Hackwar - comment - 30 Apr 2023

If the removal of the & were a b/c break (which I would contest), we would have to revert the change which caused this issue, because it would mean a break in b/c for the CMS as well. This is a bugfix for the 2.x framework.

avatar richard67
richard67 - comment - 30 Apr 2023

If the removal of the & were a b/c break (which I would contest), we would have to revert the change which caused this issue, because it would mean a break in b/c for the CMS as well. This is a bugfix for the 2.x framework.

@Hackwar Still it doesn't make sense to have this PR here for 5.0-dev and not for 4.4-dev.

avatar ChristineWk
ChristineWk - comment - 30 Apr 2023

I tried it anyway:
I took the download package: .../artifacts/joomla/joomla-cms/5.0-dev/40504/downloads/65063/Joomla_5.0.0-dev+pr.40504-Development-Full_Package.zip
Loaded it and started installation.

The message given in the issue was no longer, but:
To verify ownership, the file "_Joomlaxyz......txt" just placed in the "installation" directory of Joomla! was created can be deleted again. Then “Joomla! Install” button to continue.

So how it "usually" works.
But due to the previous discussions, I didn't click on sucessful.
Because I don't know, of backwards compatibility (J 4.4-dev?) on new installations.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40504.
avatar Hackwar
Hackwar - comment - 30 Apr 2023

This fix is only half the work, so I'm closing this PR in favour of #40515 and #40514.

avatar Hackwar Hackwar - change - 30 Apr 2023
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2023-04-30 19:31:29
Closed_By Hackwar
avatar Hackwar Hackwar - close - 30 Apr 2023

Add a Comment

Login with GitHub to post a comment