When looking at the API page information for the method processForm(), down below there's some inline docs about the condition that calls $form_state->isRebuilding() which doesn't make sense:
// If $form_state->isRebuilding() has been set and input has been
// processed without validation errors, we are in a multi-step workflow
// that is not yet complete. A new $form needs to be constructed based on
// the changes made to $form_state during this request. Normally, a submit
// handler sets $form_state->isRebuilding() if a fully executed form
// requires another step. However, for forms that have not been fully
// executed (e.g., Ajax submissions triggered by non-buttons), there is no
// submit handler to set $form_state->isRebuilding(). It would not make
// sense to redisplay the identical form without an error for the user to
// correct, so we also rebuild error-free non-executed forms, regardless
// of $form_state->isRebuilding().
// @todo Simplify this logic; considering Ajax and non-HTML front-ends,
// along with element-level #submit properties, it makes no sense to
// have divergent form execution based on whether the triggering element
// has #executes_submit_callback set to TRUE.
if (($form_state->isRebuilding() || !$form_state->isExecuted()) && !FormState::hasAnyErrors()) {
// Form building functions (e.g., self::handleInputElement()) may use
// $form_state->isRebuilding() to determine if they are running in the
// context of a rebuild, so ensure it is set.
$form_state->setRebuild();
$form = $this->rebuildForm($form_id, $form_state, $form);
}
}
Since you can't "set" a getter method with no parameters ($form_state->isRebuilding()), instead, the documentation needs to be changed to specify that if the form "rebuild" flag is set by calling the method $form_state->setRebuild(TRUE) and the input is processed without validation errors, then we are in a multi-step webform workflow that is not yet complete.
Head to the function FormBuilder::processForm
API page information and look for the inline docs before the condition if (($form_state->isRebuilding() || !$form_state->isExecuted()) && !FormState::hasAnyErrors()) { ...
You can also see the inline docs in the file itself: core/lib/Drupal/Core/Form/FormBuilder.php, line 605
Change the inline docs in the core/lib/Drupal/Core/Form/FormBuilder.php file to better reflect the multi-step webform validation process.
The API pages need to be changed as well: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Form%21Fo...
N/A
N/A
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #35 | 2776661-nr-bot.txt | 1.51 KB | needs-review-queue-bot |
Show commandsStart 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:
changes, plain diff MR !9319
Comments
Comment #2
pushpinderchauhan commentedComment #3
pushpinderchauhan commentedJust took care of line length and wrapping and updated the patch again. Please review.
Comment #14
quietone commentedIt doesn't seem right to say that a method is set. This is actually getting the value of the $rebuild parameter. Why not just say that,
"If $form_state->rebuild has been set and input has been"
If that is correct there are other places in the comment it would need to change.
Comment #17
ameymudras commentedChanges as per suggestion in #14
Comment #18
ameymudras commentedComment #20
kristen polThanks for the update.
Nitpick: The comments need to be adjusted to be as close to 80 characters as possible.
Comment #21
ameymudras commentedComment #22
vinmayiswamy commentedVerified and tested patch #21 in Drupal 10.1.x. Patch applied successfully.
Test Steps:
1. Fresh install Drupal 10.1.x.
2. Apply the given patch.
3. Navigate to /core/lib/Drupal/Core/Form/FormBuilder.php in the codebase to check '$form_state->rebuild' in processForm()
Test Result:
Thanks @ameymudras for the patch.
Comment #23
vinmayiswamy commentedSince the patch #21 is in green and has been addressed feedback from #14 and #20, changing issue status to RTBC.
Comment #24
catchI don't think this an improvement, we're taking an awkwardly phrased comment about an API method and making it about a protected property instead. What about 'If $form_state->isRebuilding() returns TRUE" at least for the first part of the comment.
Comment #27
quietone commentedChanged to use a phrase of the form, "'rebuild' has been set to TRUE", which is from the docs for \Drupal\Core\Form\FormState::$rebuild.
Comment #28
joachim commented> I don't think this an improvement, we're taking an awkwardly phrased comment about an API method and making it about a protected property instead.
Agreed -- the protected property is internal.
> If 'rebuild' has been set to TRUE
This doesn't help either. How do I find out more about what 'rebuild' is and how to set it or check for it?
> What about 'If $form_state->isRebuilding() returns TRUE" at least for the first part of the comment.
Maybe. It depends whether the reader of this documentation is trying to know
A. 'how do I know which behaviour I will get here' --> isRebuilding() is the key thing because that's how you find out
B. 'how do I influence which behaviour I will get here' --> isRebuilding() is one step removed from this AND its docs don't tell you how to change that status, and what you actually need to know about is setRebuild()
I'd lean towards B.
Comment #30
brandonlira commentedHi,
I’ve updated the inline documentation in processForm() (in FormBuilder.php) to clarify the usage of isRebuilding() and setRebuild(). The changes now specify that isRebuilding() is a getter that returns TRUE when the rebuild flag is set (via setRebuild(TRUE)), rather than implying that it can be directly set.
I hope this addresses the feedback from earlier comments. Please review my changes in MR ![!9319] and let me know if further adjustments are needed.
Comment #31
sivaji_ganesh_jojodae commentedComment #32
joachim commented> // If $form_state->isRebuilding() returns TRUE and input has been processed
This is correct and accurate, but from the POV of DX, it doesn't fix the problem.
The original docs approach this from answering the question, 'what do you need to do to the form to get this behaviour?'
> If $form_state->isRebuilding() has been set
It's just that the technical detail is wrong.
We need to change the detail, not the meaning being conveyed.
Comment #33
charlliequadros commentedHi @joachim,
it's not clear what the comment is meant to convey. To me, it should include something like:
This would help clarify when and why the rebuild happens. What do you think?
Comment #34
charlliequadros commentedComment #35
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.
Comment #36
brandonlira commentedHi @joachim,
I’ve updated the comment to reflect that setRebuild(TRUE) is what triggers the behavior, as you suggested, while preserving the original intention of explaining how to get this behavior from a DX perspective.
I also rebased the branch onto the latest 11.x since it was many commits behind, to ensure the pipeline runs accurately.
Let me know if anything else needs adjusting!
Comment #37
smustgrave commentedFor good practice issue summary should be complete using standard template.
Comment #38
danrodComment #39
danrodI changed the summary above using the standard template, hope it makes sense.
Comment #40
danrodComment #41
smustgrave commentedFeedback believe to be addressed
Comment #44
catchCommitted/pushed to 11.x and cherry-picked to 11.2.x, thanks!