Skip to content

Comments

[4.4] Reverting parts of #40257#40569

Merged
laoneo merged 2 commits intojoomla:4.4-devfrom
Hackwar:4.4-filesystem-revert
May 12, 2023
Merged

[4.4] Reverting parts of #40257#40569
laoneo merged 2 commits intojoomla:4.4-devfrom
Hackwar:4.4-filesystem-revert

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented May 11, 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

@richard67
Copy link
Member

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.

@laoneo
Copy link
Member

laoneo commented May 12, 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.

@richard67
Copy link
Member

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

@laoneo
Copy link
Member

laoneo commented May 12, 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.

@richard67
Copy link
Member

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

@Hackwar
Copy link
Member Author

Hackwar commented May 12, 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.

@richard67
Copy link
Member

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

@laoneo
Copy link
Member

laoneo commented May 12, 2023

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

@richard67
Copy link
Member

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.

@richard67
Copy link
Member

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.

@laoneo laoneo assigned laoneo and unassigned MacJoom May 12, 2023
@laoneo
Copy link
Member

laoneo commented May 12, 2023

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

@laoneo laoneo merged commit 2aeb1d4 into joomla:4.4-dev May 12, 2023
@laoneo
Copy link
Member

laoneo commented May 12, 2023

Thanks @Hackwar for stepping up so quickly!

@laoneo laoneo added this to the Joomla! 4.4.0 milestone May 12, 2023
@Hackwar Hackwar deleted the 4.4-filesystem-revert branch March 22, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants