No Code Attached Yet
avatar nikosdion
nikosdion
22 Aug 2021

After namespacing the JFile and JFolder classes the administrator/components/com_joomlaupdate/restore_finalisation.php was attempted to be updated to reflect that change... “attempted” being the operative word. What it does and what it think it does are two different things :)

First, let's see what's broken.

Line 90 currently reads

if (!class_exists('File'))

but it should read

if (!class_exists('\Joomla\CMS\Filesystem\File'))

See the problem? You need to provide an FQN to class_exists. It won't do in-scope class resolution. The original class_exists('File') will always return false.

The other two problems on that file are of the exact same nature.

Line 131 currently reads

if (!class_exists('Folder'))

but it should read

if (!class_exists('\Joomla\CMS\Filesystem\Folder'))

Likewise for namespacting JText.

Line 174 reads

if (!class_exists('Text'))

but it should read

if (!class_exists('\Joomla\CMS\Language\Text'))

In normal operation of the updater over the web you will never experience it.

However, now that we have a CLI core updater it is possible that the respective File, Folder or Text class has already been loaded in the course of applying an update over CLI, e.g. when downloading the update file and/or displaying information to the user. As a result, when Joomla reaches the finalisation step it will fail with a PHP error about the class having already been defined. The consequence of that is that among other things leftover files are not deleted (which can be addressed from the CLI) and that the database schema is not updated. Okay, this is the important bit.

Within the 4.0 release you won't notice any major issues. However, as soon as you change anything substantial in the database you might end up with a prepared statement failing before you have the chance of logging in, preventing you from visiting your site's backend to fix the schema. Remember that there's no way to do that from CLI right now. That's a bit of a pickle to find yourselves in.

This actually did happen to me by adapting the Joomla 4 CLI updater to run under Joomla 3 so I could do an unattended update from Joomla 3.10 to Joomla 4.0.

So, while you cannot CURRENTLY reproduce a fatal error using just core code it's only a matter of time before you do. Please consider this an advance warning of a very nasty bug ready to strike at an unsuspecting time and a great opportunity for fixing what is in retrospect an obvious bug that went undetected for three years. The fix is literally three lines of code. If you want me to, I can send a PR as well.

avatar nikosdion nikosdion - open - 22 Aug 2021
avatar joomla-cms-bot joomla-cms-bot - change - 22 Aug 2021
Labels Added: No Code Attached Yet
avatar joomla-cms-bot joomla-cms-bot - labeled - 22 Aug 2021
avatar PhilETaylor
PhilETaylor - comment - 22 Aug 2021

The fix is literally three lines of code. If you want me to, I can send a PR as well.

Please can you do that :) I'll test it and Im sure @richard67 will too.

/me hopes I never broke it 3 years ago haha

avatar nikosdion
nikosdion - comment - 22 Aug 2021

According to GitHub showing me a Git blame it was @mbabker BUT do take this with a grain of salt. I will do a PR as soon as mini-me is asleep.

avatar PhilETaylor
PhilETaylor - comment - 22 Aug 2021

Let blame him, he cant run after us and catch us (with his knee poor sod) ;-)

avatar HLeithner
HLeithner - comment - 22 Aug 2021

just for confirmation a short poc https://3v4l.org/mlAm4

avatar brianteeman
brianteeman - comment - 22 Aug 2021

It was @mbabker code that he posted as a gist and others made into a pull request https://gist.github.com/mbabker/5e171b1df99f71a30290125e0292b1d9

avatar wilsonge
wilsonge - comment - 22 Aug 2021

I'd love a PR @nikosdion - will make sure it gets merged in time for any 4.0.1's and thankyou for reporting!!

avatar wilsonge wilsonge - change - 22 Aug 2021
Labels Added: ?
avatar wilsonge wilsonge - labeled - 22 Aug 2021
avatar richard67
richard67 - comment - 23 Aug 2021

@nikosdion If you don't have the time for making a PR then let us know that soon so someone else (maybe me) can do it before we release 4.0.1, which is planned for tomorrow.

avatar nikosdion
nikosdion - comment - 23 Aug 2021

I'll do it now.

Sorry, my back gave up last night, I couldn't do much but sit in a comfy chair. This morning I was doing releases and running some errands. FINALLY I have some time. Let me set up a branch and I'll do the PR promptly.

avatar richard67
richard67 - comment - 23 Aug 2021

@nikosdion Thanks, really much appreciated, and I hope your back gets better. I have back pain too sometimes, so I know how it is.

avatar nikosdion
nikosdion - comment - 23 Aug 2021

Yeah, I've had back pain since I was 24. A little souvenir from my days in the airforce ?

I have submitted the PR.

I believe, without having had the time to test it, that a CLI update to 4.0.1 would fail before the PR and work after the PR. You'd need to set up a custom update source, though, to test.

avatar richard67 richard67 - change - 23 Aug 2021
Status New Closed
Closed_Date 0000-00-00 00:00:00 2021-08-23 09:16:33
Closed_By richard67
Labels Added: ?
Removed: ?
avatar richard67 richard67 - close - 23 Aug 2021
avatar richard67
richard67 - comment - 23 Aug 2021

Closing as having a pull request. Thanks.

avatar richard67 richard67 - change - 23 Aug 2021
Labels Removed: ?
avatar richard67 richard67 - unlabeled - 23 Aug 2021

Add a Comment

Login with GitHub to post a comment