PR-4.4-dev Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
11 May 2023

Pull Request for Issue #40430.

Summary of Changes

In #40430 an issue came up where updating failed because the framework class File wasn't available in /administrator/components/com_admin/script.php. This PR reverts that change from #40257. We will eventually have to switch over the whole update process from File/Folder/etc to the framework classes and update the finalisation.php as well.

Testing Instructions

Codereview.

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 - 11 May 2023
Category Administration com_admin
avatar Hackwar Hackwar - open - 11 May 2023
avatar Hackwar Hackwar - change - 11 May 2023
Status New Pending
avatar richard67 richard67 - test_item - 11 May 2023 - Tested successfully
avatar richard67
richard67 - comment - 11 May 2023

I have tested this item successfully on 91e63b8

Code review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40569.

avatar laoneo
laoneo - comment - 12 May 2023

Thanks @Hackwar. Waiting here for @MacJoom as he is currently testing the upgrades as well to see if we can reproduce the issue on our end.

avatar richard67
richard67 - comment - 12 May 2023

@laoneo @MacJoom This PR here is one of the few rare cases where a review is sufficient and clear. Just check what is in file extract.php of the update component. Check the namespace used there for the mocked file system libs, and check that the change in script.php fits to that. When doing updates it can happen on the same environment that one time the real filesystem libs or the stuff in extract.php is used, and another time the real lib. So it might be hard to catch the issue with a real test.

avatar laoneo
laoneo - comment - 12 May 2023

On some point we want to make the switch to the framework classes. We will face then the same issue again. So it makes sense to invest a bit of resources as we are aware of the issue right now and do some testing on what to do to properly fix it. Because in my opinion all is needed is to change the namespace here. This basically what Nik suggested in the comment #40430 (comment). So I would prefer to see if that fix would work, but first we need to be able to reproduce it.

avatar richard67
richard67 - comment - 12 May 2023

@laoneo No it is not sufficient to change the namespace at that one place in extract.php because this namespace contains the mocks for the file and for the folder class and the CMS has not been changed yet to move to the framework for the folder class, so we would need 2 namespaces in extract.php as long as that is not done.

avatar Hackwar
Hackwar - comment - 12 May 2023

Yes, @richard67, thanks for explaining this. Indeed, we will have to switch over at one point, but that would happen when we have switched all other code over. At that point we would also have to check the classes which are included in the finalisation.php and update that code where necessary. With this, we actually don't have one or two, but three different implementations of the File and Folder class and I'm pretty sure that they are all not in sync to each other. So there is more work to do than just to change the namespace of the filesystem package in the finalisation.php.

avatar richard67
richard67 - comment - 12 May 2023

@Hackwar If you like to have some co-worker for the script.php and extract.php files I volunteer because I know these 2 files well.

avatar laoneo
laoneo - comment - 12 May 2023

Can we then not just load composer in the finalization script and get completely rid of the fake namespaces?

avatar richard67
richard67 - comment - 12 May 2023

For now I would say this PR here is the quick fix so people can test updating with nightlies, and the bigger clean-up we do soon after that and have it hopefully ready for 4.4-alpha1.

avatar richard67
richard67 - comment - 12 May 2023

Can we then not just load composer in the finalization script and get completely rid of the fake namespaces?

I don‘t know, but would be cool of course if we could get rid of the fake namespace.

avatar laoneo laoneo - change - 12 May 2023
Labels Added: PR-4.4-dev
avatar laoneo
laoneo - comment - 12 May 2023

Agree that we should first accept here and then have a look if we can deal something with the framework package.

avatar laoneo laoneo - change - 12 May 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-05-12 08:10:05
Closed_By laoneo
avatar laoneo laoneo - close - 12 May 2023
avatar laoneo laoneo - merge - 12 May 2023
avatar laoneo
laoneo - comment - 12 May 2023

Thanks @Hackwar for stepping up so quickly!

Add a Comment

Login with GitHub to post a comment