Skip to content

Comments

[5.0] Can't call File::write() with string-literal#40504

Closed
Hackwar wants to merge 2 commits intojoomla:5.0-devfrom
Hackwar:5.0-file-installer
Closed

[5.0] Can't call File::write() with string-literal#40504
Hackwar wants to merge 2 commits intojoomla:5.0-devfrom
Hackwar:5.0-file-installer

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Apr 29, 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

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@HLeithner
Copy link
Member

Should be fixed in 4.3 IR 4.4

@brianteeman
Copy link
Contributor

This used to work. What broke it?

@sandewt
Copy link
Contributor

sandewt commented Apr 30, 2023

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

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

@richard67
Copy link
Member

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.

@richard67
Copy link
Member

@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 .

@richard67
Copy link
Member

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.

@brianteeman
Copy link
Contributor

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

@richard67
Copy link
Member

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.

@Hackwar
Copy link
Member Author

Hackwar commented Apr 30, 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.

@richard67
Copy link
Member

@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).

@Hackwar
Copy link
Member Author

Hackwar commented Apr 30, 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 &

@richard67
Copy link
Member

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.

@Hackwar
Copy link
Member Author

Hackwar commented Apr 30, 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.

@richard67
Copy link
Member

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.

@ChristineWk
Copy link

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.

@Hackwar
Copy link
Member Author

Hackwar commented Apr 30, 2023

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

@Hackwar Hackwar closed this Apr 30, 2023
@Hackwar Hackwar deleted the 5.0-file-installer branch April 30, 2023 19:31
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