Problem/Motivation

The current implementation of invoking a hook is incorrect for the use case. Instead of invoking several hook implementations to receive an array of usernames and use the first entry of the array, we should simply implement an alter hook and deprecate the old implementation.

Steps to reproduce

Proposed resolution

Deprecate the old implementation and remove the old hook implementation in the submodule.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork email_registration-3396028

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grevil created an issue. See original summary.

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented

Definitely yes. Should be deprecated in 2.x to reduce the risk of updaters and other contrib modules to miss the update!
https://drupalize.me/tutorial/what-deprecated-code

Grevil’s picture

Grevil CreditAttribution: Grevil at DROWL.de commented
Title: Change the main module's hook implementation to use an alter hook » Let the main module provide an alter hook, instead of a normal one
Issue summary: View changes

Grevil opened merge request !21

Grevil’s picture

Grevil CreditAttribution: Grevil at DROWL.de commented
Assigned: Grevil » Anybody
Status: Active » Needs review

Done, don't really like the removing of the presave hook in the submodule. Feels worse than before, but please review.

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented

Done, don't really like the removing of the presave hook in the submodule. Feels worse than before, but please review.

But makes order and logics more clear. Less magic, clear flow.

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented
Assigned: Anybody » Grevil
Status: Needs review » Needs work

@Grevil: commented, I think we had a misunderstanding regarding the new alter hooks use and logic. See the comments.

Grevil’s picture

Grevil CreditAttribution: Grevil at DROWL.de commented
Assigned: Grevil » Anybody
Status: Needs work » Needs review

All done, please review!

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented
Status: Needs review » Reviewed & tested by the community

All fine for 2.x! We should now add version information on the module page and inform about the hook deprecation there (and in readme?)
Super nice improvements!

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented
Assigned: Anybody » Grevil
Status: Reviewed & tested by the community » Needs work
Grevil’s picture

Grevil CreditAttribution: Grevil at DROWL.de commented
Assigned: Grevil » Unassigned
Status: Needs work » Needs review
Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented
Status: Needs review » Reviewed & tested by the community

Perfect!

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented
Status: Reviewed & tested by the community » Fixed

  • Anybody committed 7dfd66ce on 2.x authored by Grevil 
    Issue #3396028 by Grevil, Anybody: Let the main module provide an alter...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.