[#40504] - [5.0] Can't call File::write() with string-literal
- Closed
- 30 Apr 2023
- Medium
- Build: 5.0-dev
- # 40504
- Diff
- Hackwar:5.0-file-installer
User tests: Successful: Unsuccessful:
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
joomla-cms-bot
- | Category | ⇒ | Installation |
| Status | New | ⇒ | Pending |
| Labels |
Added:
PR-5.0-dev
|
||
This used to work. What broke it?
I cannot argue the following, but shouldn't the solution be found here? $buffer = ?
public static function write($file, $buffer, $useStreams = false)
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.
@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 .
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.
and thats why I keep banging on about establishing why something that did work now no longer works before writing a fix
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.
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.
@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).
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 &
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.
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.
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.
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.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-04-30 19:31:29 |
| Closed_By | ⇒ | Hackwar |
Should be fixed in 4.3 IR 4.4