Problem/Motivation

Let's add dependency injection and split the classes.
We can also consolidate some helper functions.

Steps to reproduce

N/A

Proposed resolution

Move hooks to *.api.php with exceptions below.
Move _update_notify to cron
Add DI
Exceptions
module / theme install / uninstall are in one hook class since they call the same function
Isolate following hooks since they have a bunch of dependencies that would crowd other classes.

  • page_top
  • cron
  • mail

Remaining tasks

Review

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#7 3545284-nr-bot_z3_41vyt.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3545284

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Title: Modernize Announcements Feed Hook implementations » Modernize Update Hook implementations
nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !13177

nicxvan’s picture

nicxvan commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Parent issue: » #3493453: [meta] Standardize and clean up hook classes in core
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3545284-nr-bot_z3_41vyt.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

dww’s picture

dww
we/he/they
commented
Related issues: +#3125013: Refactor update.fetch.inc into a MailHandler

#3125013: Refactor update.fetch.inc into a MailHandler is already handling parts of this.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

sivaji_ganesh_jojodae made their first commit to this issue’s fork.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs work » Needs review

I have rebased the branch and resolved the pipeline issues.

dww’s picture

dww
we/he/they
commented
Title: Modernize Update Hook implementations » [pp-1] Modernize Update Hook implementations
Status: Needs review » Postponed

Per #8, this MR is duplicating effort from #3125013: Refactor update.fetch.inc into a MailHandler. Let's stop here and wait for that to land (since it's already RTBC, and more important for the effort to remove procedural .module and .inc files). Thanks!

dww’s picture

dww
we/he/they
commented
Title: [pp-1] Modernize Update Hook implementations » Modernize Update Hook implementations
Status: Postponed » Needs work

#3125013: Refactor update.fetch.inc into a MailHandler is in. This needs to be rebased vs. main and conflicts resolved.

Thanks,
-Derek