Problem/Motivation

The logic in Registration::preSave() executes before hook_ENTITY_TYPE_presave(), which means if you set the state to 'complete' in a presave hook, then no completed timestamp will ever be saved, and RegisterForm::useAdvancedForm will throw an error when it tries to format a NULL datetime.

It's debatable whether the problem here is that it's too easy to end up with no completed timestamp, or that RegisterForm is not defensive enough.

I can see an argument that setting the completed time in Registration::preSave() is not optimal, and it would be better to have a Registration::setState() method and make that responsible for the completed time logic (the setter could be called from ::preSave() as a backup too).

Proposed resolution

Make RegisterForm more defensive.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

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:

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw’s picture

Status: Active » Needs review

john.oltman made their first commit to this issue’s fork.

john.oltman’s picture

Status: Needs review » Fixed

Thanks for the post, another good catch @jonathonshaw! I could not take your MR because it is possible for a canceled registration to have a completed timestamp if it was completed and then subsequently canceled. Also, it is not valid for a registration to be completed and not have a completed timestamp. As you point out in your issue description, entity Presave is not a good place for the timestamp logic because other modules may have presave hooks or presave subscribers that set the state after it. So I moved the logic into a low priority presave subscriber - this event is triggered by registration storage, right before the actual db save is going to occur, so it runs after any presave hooks. I added a test and confirmed it failed when the logic was in the entity preSave function.

I committed the fix to dev branch - since I just did a release recently, I'm going to see what other issues arise and then do a release in early February.

Status: Fixed » Closed (fixed)

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