Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
ConfirmFormInterface::getFormName() is only called by ConfirmFormBase. The return value, a string, is used to build a hidden form element:
$form[$this->getFormName()] = ['#type' => 'hidden', '#value' => 1];
The form value is not used by ConfirmFormBase's submit handler, although one subclass in core, UserMultipleCancelConfirm does check it.
However, because this form element uses the #value property, its value will ALWAYS be 1 according to the docs for Drupal\Core\Render\Element\Hidden:
* - #value: The value of the form element. The Form API ensures that this
* value remains unchanged by the browser.
Therefore, AFAICT it doesn't serve any purpose and both the element and the ConfirmFormInterface::getFormName() could be deprecated.
Steps to reproduce
Proposed resolution
Remaining tasks
- Remove ConfirmFormInterface::getFormName().
- Deprecate implementations in core form classes. See how to deprecate for what needs to be done to add the deprecation.
- Remove the calls to getFormName() in core form classes.
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3497021
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:
changes
Comments
Comment #2
godotislateCode archaeology seems to show this is cruft from this commit 19 years ago (#18329: Unify confirmation messages), and how confirm forms originally worked: https://git.drupalcode.org/project/drupal/-/blob/198ec98f756673da8c899bb0236a91808ed010ec/includes/theme.inc#L898
Comment #3
arunsahijpal commentedWorking on it.
Comment #5
joachim commentedLooks like a good start.
We also need to remove the condition check from UserMultipleCancelConfirm.
And I don't know what BC handling we need.
ConfirmFormInterface is what I call an inward-facing interface: 3rd parties are meant to implement it, not call it. So I don't think we need to deprecate getFormName() before removing it. But we might want to notify implementors that they can remove getFormName() from their class.
Comment #6
arunsahijpal commentedHi @joachim,
Yes, I think we can remove the
if ($form_state->getValue('confirm'))condition fromUserMultipleCancelConfirm::submitForm()as the confirm field is no longer relevant. SinceConfirmFormInterfaceis inward-facing, I agree thatgetFormName()can be removed without deprecation.I've done the changes pls check.
Thanks
Arun
Comment #7
joachim commentedRemoving the if() in core/modules/user/src/Form/UserMultipleCancelConfirm.php means that the indentation needs to be changed too!
Comment #8
arunsahijpal commentedCorrected the indentation.
Comment #9
joachim commentedNearly there -- needs a few coding style fixes.
Comment #10
arunsahijpal commentedComment #11
godotislate@arunsahijpal It may be helpful for you to make sure the MR build passes before setting to Needs Review. The build for the latest changes still fails.
See https://git.drupalcode.org/project/drupal/-/merge_requests/10785. You can review the build job log https://git.drupalcode.org/issue/drupal-3497021/-/jobs/3926956 to see why it is failing.
Comment #12
joachim commentedLooks like you've run it through a code formatter that's not configured for Drupal, as it's put all the { on new lines.
Comment #13
arunsahijpal commentedI've fixed the indentation issue but now a phpunit functional is failing
I think it is due to this Line of code as it should be this
if ($form_state->getValue('confirm')) {because Form submissions in Drupal are processed through$form_state.Should i change it?
Comment #14
godotislateWhen making the change, you'd need to make sure the functionality is not affected. If it isn't, then the test needs to be updated as well. There are two test failures, so you'd also need to make sure that the necessary changes can be made and are applied for both.
Comment #15
arunsahijpal commented@joachim, IDK why it is not finding ''confirm" key in
SearchConfigSettingsFormTestcould you pls check it, and the secondSearchMultilingualEntityTesterror is not due to the change made ingetFormName()function.Comment #16
joachim commentedYup, looks like we need to remove
> if ($form['confirm']
in ReindexConfirm.
Which is really weird code anyway -- that's ALWAYS going to be in the form array!
Comment #19
joachim commented@niharika.s You seem to have made a new branch and a new MR, neither of which are necessary. New branches are only needed when there's a completely different approach to fixing an issue.
If you want to work on this issue, you can make a commit to the existing branch.
Your change isn't quite right though, as it also needs the indentation fixing.
Comment #20
arunsahijpal commented@joachim,
I've updated the ReindexForm and the pipeline is also green now, pls check.
Comment #21
joachim commentedThere's stray whitespace being added in core/modules/search/tests/src/Functional/SearchMultilingualEntityTest.php.
Other than that, it's RTBC.
Comment #22
arunsahijpal commentedRemoved the whitespace.
Comment #23
joachim commentedI'm not sure you've removed the right line -- the diff in the MR for core/modules/search/tests/src/Functional/SearchMultilingualEntityTest.php now shows one removed and one added.
It's a good idea to do a `git diff 11.x` locally before you commit and push.
Comment #24
arunsahijpal commented@joachim , could you pls check now.
Comment #26
joachim commentedLooks good. Thanks!
Comment #27
quietone commentedThis looks fine except for BC, which @joachim mentioned in #5. The first thing I did was search for getFormName and there 3 pages of results. This change would break at lease one of those module (I don't recall which). I then asked in committer slack and catch confirmed that this needs to have deprecations to prevent fatals in at least some of the found modules.
Setting back to NW.
Comment #28
smustgrave commentedComment #29
arunsahijpal commentedHi @quietone and @joachim,
So should i revert my changes and add a deprecation comment instead?
Comment #30
joachim commentedWe need
Comment #31
joachim commentedComment #32
joachim commentedWe need to restore getFormName() in the base class, and instead, add to it a deprecation notice so that any calling code gets notified to stop calling it.
We should keep the removal of getFormName() in the interface though, as we want implementations to remove it.
I wonder whether we could/should give a deprecation notice to classes that have that method, or whether that's overkill.
Comment #33
joachim commented> I wonder whether we could/should give a deprecation notice to classes that have that method, or whether that's overkill.
Spoke to @longwave in Slack who says we don't need to do this.
Comment #35
joachim commentedWe don't need a new merge request or a new branch -- you can keep working on the existing one.
Comment #36
arunsahijpal commented@joachim
Could you please provide me the Change Record link so that I can use it.
Comment #37
joachim commentedThere isn't a Change Record yet. Use the link in the issue details to create a draft.
Comment #39
vladimirausAdded change record.
Comment #41
joachim commentedThanks!
I've rewritten it a bit as it was mostly the text of the issue summary. The CR has a different audience from the IS -- the IS says what's wrong and what we're going to fix and how, but the CR needs to tell people what's changed and how to update their code in response. If people reading the CR want the background, they can follow the link to the issue.
Comment #42
joachim commentedOverall looks good, but the deprecation docs and error messages need to follow the standard format at https://www.drupal.org/about/core/policies/core-change-policies/how-to-d....
Comment #44
arunsahijpal commented@joachim,
I've made the changes that you suggested, pls review.
Comment #45
joachim commentedYour new MR is missing all the changes from the original MR, such as removing the calls to this method in core!
Please can you make the new deprecation message changes on the *original* branch and MR, and close the new branch and MR.
A new branch/MR is only needed when we are trying a completely different approach. That is not the case here.
Comment #48
arunsahijpal commented@joachim
I've made the changes in the original MR and hide the new MR, please check.
Comment #49
smustgrave commentedCan summary be updated. The title mentions deprecating getFormNames() pretty straight forward. But the MR appears to be doing extra stuff, so think that should be marked so easier to review.
Comment #50
joachim commentedDone.
Comment #51
smustgrave commentedThanks left some comments on the MR.
Comment #52
godotislateChanges are in scope. See explanation in MR comments.
Comment #53
joachim commentedI think this is ready.
BTW @vladimiraus it's much clearer to rebase on 11.x rather than merge it in, as that way you don't get merge commits in the feature branch's history.
Comment #54
catchThis looks like good cleanup, but git apply is complaining about trailing whitespace errors - could that be cleaned up?
Comment #55
godotislatePHPCS wasn't picking up any white space issues, so I took a guess that it's the deprecation message string.
I also removed the
stringtypehints ongetFormName()in (unlikely, granted) the case that there are subclasses overriding the base class method.Comment #56
smustgrave commentedAppears to need a rebase, will keep an eye out on it though probably good to self RTBC
If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #57
arunsahijpal commented@smustgrave ,
pls check now
Comment #58
arunsahijpal commentedChanged the status by mistake to RTBC
moving back to NW.
Comment #59
arunsahijpal commented@joachim,
Could you please review it.
Comment #60
joachim commentedJust a couple of minor things to change.
Comment #61
arunsahijpal commentedPlease check now.
Comment #62
joachim commentedLooks good. Thanks!
Comment #63
godotislateFYI for future reference, I think using
__METHOD__can save having to typeConfirmFormBase::getFormName(), etc.Comment #64
alexpottThere are quite a few usages in in contrib - see http://codcontrib.hank.vps-private.net/search?text=-%3EgetValue%28%27con...
I discussed with @longwave - the thing that's tricky here is that we are only removing this because it is dead code but the value is still being used by contrib and these modules will break silently because the deprecation we're adding here is only for the method. The current implementation will break modules using
$form_state->getValue('confirm')immediately. Obviously form arrays are not part of the BC promise - see https://www.drupal.org/about/core/policies/core-change-policies/frontend... - but I think we can/should do better. I think we should consider adding a#deprecatedkey to the form arrays so we can issue a deprecation when calling$form_state->getValue()for a specific key. I think doing this would allow us to remove old stuff like this and get modules to update without breaking stuff.I think we should postpone this issue on adding this capability to the form system. Going to set this issue to needs work to open the issue to do this and then we can postpone this on that issue.
Comment #65
godotislateCreated #3519300: Provide a way to deprecate form API array value keys per #64.
Added a note that
FormStateInterface::getValue()can take nested keys as parameters, so that might need to be accounted for with the#deprecatedimplementation