-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
[REF] ScheduleReminders - Cleanup "sublimely stupid" form code #26876
Conversation
Lots more refactoring to do, this fixes the easy-to-fix things that were obviously wrong.
Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷 Introduction for new contributors
Quick links for reviewers |
@@ -138,17 +146,12 @@ public function buildQuickForm(): void { | |||
} | |||
} | |||
else { | |||
// Dig deeper - this code is sublimely stupid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is easily my favorite comment in our codebase. I'm sorry to see it go @totten
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess that code had to wander 7 years through the wilderness, but now... it's sublimely smart... I hope...
Jenkins re test this please |
@seamuslee001 was that just a drive-by comment or were you gonna merge this? |
ok this seems ok |
@totten yea, that's because the form has to do its own deserialization of the values from |
Overview
Initial cleanup of the ScheduleReminders form... toward sanity.
Lots more refactoring to do, this fixes the easy-to-fix things that were obviously wrong.