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

Modules Manager action hooks for module cleanup #7150

Merged
merged 12 commits into from Jan 15, 2024

Conversation

sjpadgett
Copy link
Sponsor Member

  • new class to include in modules directory root called by Laminas Module Mananger

Fixes #7147

- new class to include in modules directory root called by Laminas Module Mananger
@sjpadgett
Copy link
Sponsor Member Author

@adunsulag If you get a chance before you disappear tomorrow could you take a look at this concept.
I'd like to get merged early Monday.

@sjpadgett
Copy link
Sponsor Member Author

btw: not finished with cleanup

Comment on lines 142 to 143
if ($request->getPost('modAction') == "enable") {
$status = $this->EnableModule($request->getPost('modId'));
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the heck! This is backwards. enable actually disables the module and disable enables in the controller functions.
Appears interface is reporting the actions button name and not what it is supposed to do!
I guess while here I'll fix this but gesh all I wanted was to add the hooks and not go traipsing all over Laminas!!!

- document new listener class
Comment on lines +22 to +24
class ModuleManagerActionListener
{
// Prevent instantiation
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradymiller Do we have somewhere to put this file perhaps we have example module somewhere.

@sjpadgett
Copy link
Sponsor Member Author

It is always an adventure when I look into the Laminas module code!:)

…le_manager

* 'master' of https://github.com/openemr/openemr:
  fix: escaping custom templates used with ckeditor (openemr#7152)
- implement fax sms clean up methods
- add new method for listener save globals
@sjpadgett
Copy link
Sponsor Member Author

sjpadgett commented Jan 12, 2024

This button does the same as Default Install. I'm missing something. Looking for upgrade path maybe from disabled.
Looks like if have a 'sql' directory, either button looks for table/install.php there only. Had to fix path for Install SQL to account for custom_module.
I though billing takes you on a journey but Laminas is a marathon.

image

I need to reassign the Install SQL button to an upgrade sql script or patch maybe so we track version.

@zerai
Copy link
Contributor

zerai commented Jan 13, 2024

What do you think about using events?
A "fire and forget" approach can be more flexible and remove the risk of coupling with modules.

@sjpadgett
Copy link
Sponsor Member Author

Hi @zerai Thanks for the comment!
I actually considered the event route because on the face it, it seems the logical choice. My concern was bridging the two event systems and concerns for maintaining the two event dependencies where say a new symfony version may break the laminas version vica versa.
The current method is working well where if the class is in module root it is auto enabled and any unused methods are ignored if just the skeleton class is there.

Still I'm not married to current and may change my mind upon further research. I'm happy to take input on either.

@sjpadgett
Copy link
Sponsor Member Author

Figured out sql upgrade. Kind of a roundabout way to do this. I think we'll/I'll need to change how we version modules.

  • On install the manager gets version from interface/modules/custom_modules/oe-module-faxsms/version.php
  • On register if have sql/install.sql UI gives an Install and Install SQL button. After Install SQL then an Upgrade SQL button action if the following
  1. sql directory has an upgrade sql file with naming convention major_minor_patch-to-major_minor_patch.sql.
  2. the version.php version matches or lesser first version in sql file name.
  3. Final click Install button then enable.

The above is pretty close but Upgrade SQL doesn't currently render view after action plus other bugs. We still probably have a few custom module pathing issues.

Currently I trigger module listener action after the Laminas action however I wonder if it would be better to trigger listener action before Laminas action! Any feedback here?

image

@sjpadgett
Copy link
Sponsor Member Author

Restyled Module Manager to follow theme background and text colors. Also added hover making select correct buttons easier. Below is dark theme.
image
image

- fix sql action path for custom_module
- new fetch module setup method
- fix upgrade button to show when needed
- restyle Module Manager
- bump faxsms sql version to 3.1.0
- add faxsms sql upgrade sql
@sjpadgett
Copy link
Sponsor Member Author

merging soon. tired of this!:)

@sjpadgett
Copy link
Sponsor Member Author

Anyone have suggestions on workflow to do an auto upgrade on module bootstrap from ModuleLoader.
Or maybe develop a new function in sql_upgrade.php that scans modules looking for upgrade.

Develop and implement trait class for convientence functions like handling get, post, session plus more supers.
develop laminas get registry settings in abstract class
@sjpadgett
Copy link
Sponsor Member Author

Does it ever end!!! I still want to go ahead and style the other care modules to at least follow our themes bg and text colors.
install, upgrade and default sql actions work except upgrade needs to be fixed to update version in DB and refresh view after action.

It would have been nice to have some companionship for this project because it is more enjoyable doing these tedious projects where ideas can be bounced around and at minimum vane some interest.....

I don't know if I will do more or not. I hope this process is not too hard for people to catch onto but I may put a period on it for now and merge and test on dev demo later..

@sjpadgett sjpadgett merged commit 901fba1 into openemr:master Jan 15, 2024
24 checks passed
@sjpadgett sjpadgett deleted the module_manager branch January 15, 2024 03:18
@bradymiller
Copy link
Sponsor Member

tenor99

sjpadgett added a commit to sjpadgett/openemr that referenced this pull request Feb 21, 2024
* Modules Manager action hooks for module cleanup
- new class to include in modules directory root called by Laminas Module Mananger

* - refactor class name to be better decription
- document new listener class

* - refactor interface button action function arguement to reflect the action in the controller.

* - rename new class again so it is somewhat unique.
- implement fax sms clean up methods
- add new method for listener save globals

* - fix SMS notification cli adding run arguments

* - create and refactor abstract class to extend in module listener.
- fix sql action path for custom_module
- new fetch module setup method
- fix upgrade button to show when needed
- restyle Module Manager
- bump faxsms sql version to 3.1.0
- add faxsms sql upgrade sql

* - add column to install sql.

* Finish new abstract class for module manager actions
Develop and implement trait class for convientence functions like handling get, post, session plus more supers.
develop laminas get registry settings in abstract class

* style

* - more restyles

(cherry picked from commit 901fba1)
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.

Add module perform after actions functionality to Module Manager
3 participants