Skip to content

Properly handle disabled hooks & includes#7666

Merged
jdarwood007 merged 4 commits intoSimpleMachines:release-2.1from
sbulen:hook_fixes
Mar 12, 2023
Merged

Properly handle disabled hooks & includes#7666
jdarwood007 merged 4 commits intoSimpleMachines:release-2.1from
sbulen:hook_fixes

Conversation

@sbulen
Copy link
Copy Markdown
Contributor

@sbulen sbulen commented Jan 6, 2023

Fixes #7634

This addresses a number of problems where adding/removing mods did not properly deal with disabled hooks. It also addresses numerous issues with the Integration Hooks admin function when dealing with disabled hooks & also when dealing with includes.

Changes:

  • When adding a hook, if a disabled entry previously exists, it should get rid of the old disabled entry, & avoid creating a dupe.
  • When removing a hook, it should look for disabled entries as well, otherwise they will be left in place, e.g., even after a mod de-install.
  • Some file operations (file exists, etc.) were including the disabled indicator (!) in the file/function name, which would make it look like the file/function did not exist when it did. This prevented some operations in the Integration Hooks maintenance function from working.
  • Use the path when it's available. Some add/remove hook operations did not check for the path. In particular, enable/disable operations use a remove/add approach, so removing an entry resulted in no change (due to no hit, since the path wasn't specified), but the add resulted in adding a partial entry (no path). The result was an erroneous partial entry that could not be fixed.
  • Eat dupes, to help when cleaning things up. There should never be duplicates.
  • Allow for enabling/disabling of includes. Needed for when they are disabled elsewhere, e.g., in repair_settings.php.

At this point, adding mods no longer creates dupes, and results in one clean set of integration hooks, even when cleaning up after a broken install/uninstall. Removing mods removes all the hooks, no matter what the status.

Also, as intended, you can now use the Integration Hooks function to do most hook cleanup activity. Even if you were to accidentally disable all hooks in repair_settings.php.

sbulen added 3 commits January 4, 2023 23:10
Signed by Shawn Bulen, bulens@pacbell.net
Signed by Shawn Bulen, bulens@pacbell.net
 Signed by Shawn Bulen, bulens@pacbell.net
@sbulen sbulen added the Hooks label Jan 6, 2023
@sbulen sbulen added this to the 2.1.4 milestone Jan 6, 2023
@sbulen sbulen marked this pull request as draft January 6, 2023 04:19
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jan 6, 2023

Some before/after tests...

Test #1 - disable one hook:

Before - mutant partial hook entry (no path specified):
before_disable_one

After - hook properly disabled:
after_disable_one

Test #2 - uninstall with a disabled hook:

Before - hook fragment left behind:
before_uninstall_w_disabled

After - all hooks removed:
after_uninstall_w_disabled

Test #3 - disable all via repair_settings.php:

Before - note it thinks the include is missing:
before_disable_all

After - it no longer thinks the include is missing, so it can be re-enabled:
after_disable_all

@sbulen sbulen marked this pull request as ready for review January 6, 2023 04:59
Signed by Shawn Bulen, bulens@pacbell.net
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jan 6, 2023

Some more "before" tests to demonstrate the problems:

Before - deinstall when disabled hooks exist leave them in place (this PR properly deletes them):
before_disabled_not_uninstalled

Before - install when disabled hooks exist result in dupes (this PR removes the dupes):
before_disabled_dupes on install

@jdarwood007 jdarwood007 merged commit eac3724 into SimpleMachines:release-2.1 Mar 12, 2023
@pr-triage pr-triage Bot added the PR: merged label Mar 12, 2023
@jdarwood007
Copy link
Copy Markdown
Member

I tested this with a few of my mods, including dev tools. Nothing seemed to break and the hooks did seem to remain clean.

@sbulen sbulen deleted the hook_fixes branch March 13, 2023 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hooks - squirrely behavior upon disable/uninstall/reinstall

2 participants