ConfirmFormInterface::getFormName() serves no purpose in ConfirmFormBase

Code archaeology seems to show this is cruft from this commit 19 years ago, and how confirm forms originally worked: https://git.drupalcode.org/project/drupal/-/blob/198ec98f756673da8c899bb0236a91808ed010ec/includes/theme.inc#L898

Working on it.

Looks 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.

Hi @joachim,
Yes, I think we can remove the if ($form_state->getValue('confirm')) condition from UserMultipleCancelConfirm::submitForm() as the confirm field is no longer relevant. Since ConfirmFormInterface is inward-facing, I agree that getFormName() can be removed without deprecation.

I've done the changes pls check.

Thanks
Arun

Removing the if() in core/modules/user/src/Form/UserMultipleCancelConfirm.php means that the indentation needs to be changed too!

Corrected the indentation.

Nearly there -- needs a few coding style fixes.

@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.

Looks like you've run it through a code formatter that's not configured for Drupal, as it's put all the { on new lines.

I'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?

When 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.

@joachim, IDK why it is not finding ''confirm" key in SearchConfigSettingsFormTest could you pls check it, and the second SearchMultilingualEntityTest error is not due to the change made in getFormName() function.

Yup, 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!

niharika.s → made their first commit to this issue’s fork.

@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.

@joachim,
I've updated the ReindexForm and the pipeline is also green now, pls check.

There's stray whitespace being added in core/modules/search/tests/src/Functional/SearchMultilingualEntityTest.php.

Other than that, it's RTBC.

Removed the whitespace.

I'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.

@joachim , could you pls check now.

joachim → changed the visibility of the branch 3497021- to hidden.

Looks good. Thanks!

This 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.

Hi @quietone and @joachim,
So should i revert my changes and add a deprecation comment instead?

We need

We 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.

> 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.

We don't need a new merge request or a new branch -- you can keep working on the existing one.

@joachim
Could you please provide me the Change Record link so that I can use it.

There isn't a Change Record yet. Use the link in the issue details to create a draft.

Added change record.

arunsahijpal → changed the visibility of the branch 3497021-confirmforminterfacegetformname-serves-no to hidden.

Thanks!

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.

Overall 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... → .

@joachim,
I've made the changes that you suggested, pls review.

Your 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.

arunsahijpal → changed the visibility of the branch 3497021-confirmforminterfacegetformname-serves-no to active.

arunsahijpal → changed the visibility of the branch 3497021-deprecate-getFormName to hidden.

@joachim
I've made the changes in the original MR and hide the new MR, please check.

Can 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.

Done.

Thanks left some comments on the MR.

Changes are in scope. See explanation in MR comments.

I 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.

This looks like good cleanup, but git apply is complaining about trailing whitespace errors - could that be cleaned up?

PHPCS wasn't picking up any white space issues, so I took a guess that it's the deprecation message string.

I also removed the string typehints on getFormName() in (unlikely, granted) the case that there are subclasses overriding the base class method.

Appears 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!

@smustgrave ,
pls check now

Changed the status by mistake to RTBC
moving back to NW.

@joachim,
Could you please review it.

Just a couple of minor things to change.

Please check now.

Looks good. Thanks!

FYI for future reference, I think using __METHOD__ can save having to type ConfirmFormBase::getFormName(), etc.

There 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 #deprecated key 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.

Created 📌 Provide a way to deprecate form API array value keys Active per #64.

Added a note that FormStateInterface::getValue() can take nested keys as parameters, so that might need to be accounted for with the #deprecated implementation