conditional_fields_form_after_build() is way too long.
My IDE is telling me that:
Method length is 94 lines (20 allowed)
It's so long, in fact, that I can't even figure out what's going on, which is exactly why functions and methods should do one thing (as per Clean Code).
Typically, my hook implementations consist of no more than a few lines, just enough to get whatever needs to be done in OO, where all of the work is handled. Why? Because you can make small methods allowing for understandability, maintainability, ease of development.
Having said all of that, I'd rather not make this function any longer, which would otherwise be required by a couple of issues, notably #2891276: checked, !checked don't work because states set on checkbox, radiobutton wrapper divs, not HTML input elements and #2859667: Conditional Required Field not Evaluating on Save with Autocomplete widget. We need to refactor this in a cleaner way with objects and methods before adding anything else.
Help with this would be greatly appreciated (especially from those that already understand the code).
Continuing as before would make this module much more challenging to work with in the long run.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | interdiff-8-9.patch | 8.8 KB | marvil07 |
| #9 | 2926132-9-steps.patch | 84.9 KB | marvil07 |
| #9 | 2926132-9.patch | 56.78 KB | marvil07 |
Comments
Comment #2
colanComment #3
mparker17Blocks #2891276: checked, !checked don't work because states set on checkbox, radiobutton wrapper divs, not HTML input elements
Comment #4
colanComment #5
colanAfter taking another look, probably the best way to go here would be to delete that entire file and move just about everything to
src/. conditional_fields_evaluate_dependency() is actually longer.A minimal
#after_buildfunction can move back into the.modulefile, just to get things going.Comment #6
dillix commentedAny news on it?
Comment #7
colanI don't have time to work on it myself, but it needs to get done. Patches welcome!
Comment #8
marvil07 commentedI have followed colan suggestion.
I have started by just moving things around, and changing related code to support the change.
I have used static methods on a helper class, we may want to convert that into an actual instantiatable class, but this patch is not trying to do that yet.
Next step would be to actually re-organize the content of the after build callback.
Moving to needs review for feedback.
Maybe the change is worth to be added and we can later do a follow-up, or a new commit here.
Two patches are attached, the first is a simple one, the second is step by step patches that could be imported via
git amif someone want to review the steps.Comment #9
marvil07 commentedNew changes since last patch:
Comment #11
colanThanks for all of your work on this. I'm working in a branch because this is conflicting with #3068112: The tests are deprecated and some conditionals are not working., which was unfortunately committed before this one. The above commit handles everything except for
conditional_fields.api.incchanges that were applied in the other issue. I'm working on that now so there should be another commit soon (same branch, other issue), which applies those changes to the new class.I'm happy either way, but for now, I'd like to get this stuff committed before anything else does. But yes, we'll still need to make the new class instantiable (i.e. switch to non-static methods) in a follow-up issue or in here.
Comment #13
colanDone. See #3077803: Make ConditionalFieldsFormHelper class instantiable for the follow-up.
Please reopen this if there are any problems.
Comment #14
colan