Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.0] Add extensions uninstallation with optional parameter migration on update from 4.4 #40768

Merged

Conversation

richard67
Copy link
Member

@richard67 richard67 commented Jun 14, 2023

Pull Request for Issue # .

Summary of Changes

This pull request (PR) adds the capability to uninstall core extensions and optionally migrate their parameters to other core extensions when updating from the previous major version, i.e. here from 4.4.

Therefore a new function "uninstallExtensions" is added to script.php and called in the "update" function of the same file.

The call happens at the same place where the "uninstallEosPlugin" was called before it has been removed in 5.0-dev with my PR #40711 , and the new function basically works in the same way as the removed "uninstallEosPlugin" function, except that it is not limited to one plugin but goes through a list of extensions of any type and that it optionally calls a function e.g. to migrate parameters or save data before the extension is uninstalled.

The new function can be used to properly uninstall the demo task plugin as proposed with PR #40147 instead of doing that in an update SQL script, and it can be used for migrating the "System - Log Rotation" plugin to a scheduler task plugin as proposed with PR #40519 .

What this PR here does not do it to implement any parameter migration for extensions which are not uninstalled on update. This shall be done in the "postflight" method like it has been implemented for the migration of the TinyMCE plugin's paremeter with PR #40626 with function "migrateTinymceConfiguration".

Testing Instructions

For testing I have prepared code and packages which use the scheduler task demo plugin as example.

This will work as long as PR #40147 has not been merged, or when it has been merged without the update SQL scripts to delete the obsolete plugin.

Test 1: Uninstall extension without calling a migration function

  1. Modify script.php of this PR for uninstalling an extension as shown here https://github.com/richard67/joomla-cms/pull/37/files and build a package from that.
    Or download the following package which already contains these modifications: https://test5.richard-fath.de/Joomla_5.0.0-alpha2-dev-Development-Update_Package_pr40768-test-1.zip
  2. On a current 4.4-dev nightly build installation, switch on "Debug System" and set "Error Reporting" to "Maximum".
  3. Update the 4.4-dev to the modified package.
  4. Check that the extension which shall be uninstalled has properly been uninstalled and that no errors appeared during the update which could be related to the changes from this PR here.

Test 2: Uninstall extension with calling a migration function

  1. Modify script.php of this PR for uninstalling an extension with calling a migration function as shown here https://github.com/richard67/joomla-cms/pull/38/files and build a package from that.
    Or download the following package which already contains these modifications: https://test5.richard-fath.de/Joomla_5.0.0-alpha2-dev-Development-Update_Package_pr40768-test-2.zip
  2. On a current 4.4-dev nightly build installation, switch on "Debug System" and set "Error Reporting" to "Maximum".
  3. Make sure that PHP errors are logged into a log file, e.g. by using "log_errors = On" and "error_log = ..." in your php.ini file.
  4. Update the 4.4-dev to the modified package.
  5. Check that the extension which shall be uninstalled has properly been uninstalled and that no errors appeared during the update which could be related to the changes from this PR here.
  6. Check the PHP error log file if it shows the log from the testing migration function added in step 1.

Actual result BEFORE applying this Pull Request

Test 1: Uninstall extension without calling a migration function

Currently this is possible to delete the extension with an update SQL script, but this would not remove any assets or other things which would be removed on a regular uninstallation.

Test 2: Uninstall extension with calling a migration function

Currently this is not possible within update SQL scripts.

Expected result AFTER applying this Pull Request

Test 1: Uninstall extension without calling a migration function

The extension has been uninstalled after the update from 4.4-dev.

Test 2: Uninstall extension with calling a migration function

The extension has been uninstalled after the update from 4.4-dev, and the migration function has been called before uninstalling.

Link to documentations

Please select:

  • No documentation changes for docs.joomla.org needed

  • No documentation changes for manual.joomla.org needed

@HLeithner
Copy link
Member

thanks, looks good to me, will test later when drone is done

@richard67
Copy link
Member Author

richard67 commented Jun 14, 2023

thanks, looks good to me, will test later when drone is done

@HLeithner A test with the package built by drone will only show that nothing's broken, but it will not do anything because it doesn't contain any extension to be uninstalled. For this I have prepared 2 other branches and built update packages which are linked in the (to be completed) testing instructions.

@HLeithner
Copy link
Member

thanks, looks good to me, will test later when drone is done

@HLeithner A test with the package built by drone will only show that nothing's broken, but it will not do anything because it doesn't contain any extension to be uninstalled. For this I have prepared 2 other branches and built update packages which are linked in the (to be completed) testing instructions.

Yes I know, I have read the source, you don't need complex test instructions, just that the update should work without error and that the extensions should no longer exist.

@richard67
Copy link
Member Author

Yes I know, I have read the source, you don't need complex test instructions, just that the update should work without error and that the extensions should no longer exist.

@HLeithner As said, the "extensions should no longer exist" can not be tested with the package of this PR, it needs a modified one with an example. I have prepared 2 tests:

And created packages with that:

For the 2nd test I might have to modify that testing dummy function because the "echo" result is not really visible for long enough time. Maybe it has to do an "error_log" or something like that.

@richard67 richard67 changed the title [5.0] [WiP] Add extensions uninstallation with optional parameter migration on update from 4.4 [5.0] Add extensions uninstallation with optional parameter migration on update from 4.4 Jun 14, 2023
@richard67 richard67 marked this pull request as ready for review June 14, 2023 16:14
@richard67
Copy link
Member Author

Ready for testing.

@richard67
Copy link
Member Author

richard67 commented Jun 14, 2023

@HLeithner One more question, hopefully the last one: Shall I remove the condition here so it runs also when updating from 5.0.0-alpha1 where we also have the obsolete demo task plugin? https://github.com/joomla/joomla-cms/pull/40768/files#diff-db7eb77540ff419bd7e6557d2779f4cf1e31158c20cb4b63dbbe8d6c426385adR217-R220

If we remove the condition, the function will work on every update, also e.g. when updating a 5.0.1 to a 5.0.2.

If we leave it as it is, all if fine except if people started with a 5.0.0-alpha1 or updated to that version and plan to continue to use that site later, because then it will contain the obsolete plugin forever.

I think we can leave it as it is and maybe create an FAQ doc for 5.0.0-alpha1 (or alpha2).

What do you think?

@HLeithner
Copy link
Member

I think the check is not needed or set it to 5.0.1 this would allow us to use it for all alpha, beta and rc versions too, without the need to do a real semver check.

@richard67
Copy link
Member Author

richard67 commented Jun 14, 2023

I think the check is not needed or set it to 5.0.1 this would allow us to use it for all alpha, beta and rc versions too, without the need to do a real semver check.

@HLeithner I've decided to do the 5.0.1 suggestion. Other idea could be to run always but to have a version number in the array with the extensions so it could be decided individually for every extension until which version we do the migration and uninstallation. But I don't think that is necessary because our policy says we are feature complete with beta 1.

And when it turns out that this will be necessary, then it still can be added later in future.

@richard67
Copy link
Member Author

@HLeithner I think it should even work when using version_compare($this->fromVersion, '5.0.0-alpha2', 'ge'), or maybe better with 5.0.0-beta1 as that's the point where we shall be feature complete. Shall I do that? If yes, which one?

@HLeithner
Copy link
Member

Use rc1 and please test it for our usual version stages up to 5.4.0-rc1

@richard67
Copy link
Member Author

Use rc1 and please test it for our usual version stages up to 5.4.0-rc1

@HLeithner 5.4.0-rc1? Or did you mean 4.4.0-rc1? We are checking the version from which we update. I'm using https://onlinephp.io/ for testing with PHP 7.2, 7.4, 8.0, 8.1 and 8.2. Is that sufficient?

@HLeithner HLeithner merged commit b901a19 into joomla:5.0-dev Jun 15, 2023
@HLeithner
Copy link
Member

I merge this now so we can use it in real extension uninstall tests. Thanks for your work.

@richard67
Copy link
Member Author

I merge this now so we can use it in real extension uninstall tests. Thanks for your work.

@HLeithner Thanks for advice and suggestions. I will soon make a PR for Allon's branch for PR #40147 for uninstalling the demo task plugin.

@richard67 richard67 deleted the 5.0-dev-extension-uninstallation-on-update branch June 15, 2023 09:56
@richard67
Copy link
Member Author

@HLeithner I just see that we will need the enabled status of the extension in the $row parameter for any migration function because the migration might depend on if the extension is enabled or not. E.g. for the logrotation this will be the case. The old system plugin could be disabled, so the migration to a task should disable the new task in that case. I will make a follow up PR soon.

@richard67
Copy link
Member Author

Follow-up PR for the above comment is #40775 .

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.

3 participants