Remove usage of drupal_get_form()

This is not a real tim.plunkett patch ... sorry 50KB, this is disappointing.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -46,7 +46,7 @@ public function getFormId($form_arg, &$form_state);
   +   *   \Drupal::formBuilder()->getForm(), including the unique form constructor function. For
   

   80

2. +++ b/core/modules/action/lib/Drupal/action/ActionListController.php
   @@ -122,7 +122,7 @@ public function render() {
   -    $build['action_admin_manage_form'] = drupal_get_form('Drupal\action\Form\ActionAdminManageForm');
   +    $build['action_admin_manage_form'] = \Drupal::formBuilder()->getForm('Drupal\action\Form\ActionAdminManageForm');
   

   Injecting?

3. +++ b/core/modules/book/lib/Drupal/book/Form/BookForm.php
   @@ -20,7 +20,7 @@ class BookForm {
   -    return drupal_get_form('book_remove_form', $node);
   +    return \Drupal::formBuilder()->getForm('book_remove_form', $node);
   
   +++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/ContentTranslationForm.php
   @@ -23,7 +23,7 @@ public function deleteTranslation(Request $request, $language) {
   -    return drupal_get_form('content_translation_delete_confirm', $entity, $language);
   +    return \Drupal::formBuilder()->getForm('content_translation_delete_confirm', $entity, $language);
   

   Inject or not inject, this is the question.

It would be kind of cool if you would have posted a patch which removes drupal_get_form() to see that really nothing uses it anymore.
Otherwise the patch looks great.

Great patch! Unfortunately, no longer applies.

I'd rather not remove drupal_get_form(), and leave it as a deprecated, one-line wrapper, personally. It doesn't hurt anyone and it makes porting modules easier.

Nice patch! It applies now, so RTBC.

10: form-2121175-10.patch queued for re-testing.

Re-roll. Some occurrences had already been converted.

I've confirmed that 15 is a reroll of 10, so would RTBC it if it applied... but it doesn't. This one should (one more example gone).

I tried to apply this patch, but it needed a reroll. ianthomas_uk can you review and if it passes RTBC? It looked clean to me.

Will do. I noticed there were some docblock lines >80 characters, we should wrap those.

Sorry, that went way too fast. Here's a proper patch that also removes the drupal_get_form function. I searched for docblock issues and fixed all the ones I found.

We could think about injecting the form builder into some of the classes in the lower part of the the patch.

Can't apply patch so I just do re-roll.

What point to change /Drupal::formBuilder()->getForm(); to $this->formBuilder()->getForm(); in controllers.
Any way I test patch and there need re-roll conflict in /core/includes/install.core.inc

So I found conflicts from 2121175_18.patch
1. core/includes/form.inc
2. core/includes/install.core.inc
3. core/modules/language/language.admin.inc
4. core/modules/locale/lib/Drupal/locale/Form/LocaleForm.php

20: 2121175_18.patch queued for re-testing.

Ok so rerolled patch there

@Codenator Looking at the size difference between #20 39k, and the patch in #34 718k, I think that the new patch probably has all the core changes since then.

That is not exactly what we want.

adding back the needs reroll tag.
the reroll should be tried again, starting from #20.

follow exactly the instructions in https://drupal.org/patch/reroll
and we should get what we need.
ask questions in #drupal-contribute while going through the steps if any questions arise.

Thanks for response but I do exactly as in instructions there https://drupal.org/patch/reroll

So last patch got 3 conflicts
1. Conflict in LocaleForm.php. The patch changed a line in export(), but issue #1978924: Convert locale_translate_export_form to a Controller removed the export(). So resolved the conflict by .. keeping what was in head (no export, nothing to change).
2. Conflict in installer.core.inc (nothing to changed) from Issue #2213357: Use a proper kernel in early installer by sun: Use a proper kernel in early installer.
which were in installer.core.inc.
3. Next one is in form.inc
#22336: Move all core Drupal files under a /core folder to improve usability and upgrades and #2192649: Remove drupal_set_title() from installation and update process so I remove function drupal_get_form() as need do for this issue :)).
see comment #20 in this issue

Thanks YesCT for nice guide and patience :)).

+++ b/core/includes/form.inc
@@ -105,18 +105,6 @@
-function drupal_get_form($form_arg) {

The policy is not to remove the function itself in these patches, as that has caused problems in the past. It can then be removed in a simple followup.

#39 is a reroll of #20 with a few hunks removed, as detailed:

3. Next one is in form.inc
#22336: Move all core Drupal files under a /core folder to improve usability and upgrades and #2192649: Remove drupal_set_title() from installation and update process so I remove function drupal_get_form() as need do for this issue :)).

Are you sure you mean #22336? That predates #20 and shouldn't have changed that line anyway (although it will have moved it). #2192649 is the sort of thing you need to be careful with when rerolling - it has removed a call to drupal_get_form(), but has also added a new one, so you need to be careful that's dealt with too. In this case it looks like we're fine though, as HEAD doesn't call it any more (it looks like another issue changed it to use install_get_form() instead).

#23 still needs to be addressed.

HOWEVER...

Much, if not all, of this code is due to be refactored by #1971384: [META] Convert page callbacks to controllers (see the Form builder section), so there seems to be little benefit to cleaning it up now. Resolve those issues, then we can come back and see if anything still calls drupal_get_form().

@tim.plunkett pointed out on irc that drupal_get_form() is blocking a change to buildForm(), so it would be good to get this done now rather than waiting for the page callbacks to be done. As this would only be a stopgap change, I don't think we need to worry about injecting the dependencies.

#39 still needs a proper review, but one problem it does have is that it removes the function itself (as mentioned in #43).

I you check patch from #20 so you can see there function is removed as well by cosmicdreams.

@codenator just because an earlier patch did something a particular way, doesn't mean it's the right thing to do. Reviews are generally performed on the most recent patch, even if the problem is from an earlier patch.

Even when we're sure that we want to remove a function, we're typically doing the actual removal in a separate patch to the one that removes everything that uses it. That reduces the chance of a patch causing issues, and if the removal patch does cause an issue (breaks HEAD) then it's much easier to resolve.

See #2204143: Remove deprecated drupal_explode_tags() and drupal_implode_tags() for an example where a much simplier patch was rejected because it did both in a single patch.

So I return function drupal_get_form() as note ianthomas_uk

I'll miss you drupal_get_form :(

+++ b/core/includes/form.inc
@@ -113,7 +110,7 @@
-  return call_user_func_array(array(\Drupal::formBuilder(), 'getForm'), func_get_args());
+      return call_user_func_array(array(\Drupal::formBuilder(), 'getForm'), func_get_args());

not needed.

Ok I fix from #50 and add right interdiff

Didn't review the whole patch. (Just noticed this so far.)

+++ b/core/includes/form.inc
@@ -102,16 +102,13 @@
  *
  * See drupal_build_form() for documentation of $form_state keys.
- */
-
-/**
  * Returns a renderable form array for a given form ID.
  *
  * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
  *   Use \Drupal::formBuilder()->getForm().
  *
  * @see \Drupal\Core\Form\FormBuilderInterface::getForm().
- */
+  */
 function drupal_get_form($form_arg) {
   return call_user_func_array(array(\Drupal::formBuilder(), 'getForm'), func_get_args());
 }

these look like unintentional changes.

So clean more from comment #52

@Codenator Thanks!

@grom358 was that from the script? (a link to the script here might help those who do not know about it)
Did you do the script and then adjust the comments by hand?
How does it differ from the patch made "by hand"?

Doing that comparison would be a good kind of review here.

So clean more from comment #52 with interdiff.

Sorry same windows shells produce bud patch this is right one

I have been having a look at this.

Unfortunately within a couple of hours of the latest patch being posted it no longer applied. (Downside of having sprints is that HEAD moves pretty quick) so we will need to reroll. I am going to review your patch against a6ffb282610 when it last applied.

There is a bit of confusion going on with the patches 53 -> 55 -> 58, but I think I can see what's going on now.

Did you do the script and then adjust the comments by hand?
How does it differ from the patch made "by hand"?

I agree with @YesCT, can we get a link to the script and also work out which patch was posted as a result of the script and then what changes needed to be made manually.

#55 is almost certainly from the script, see #2028607: form_set_error issue. A review of the scripted version would be useful, as if the script can do everything then that makes for a much easier reroll.

If we're having trouble keeping up with HEAD then this is not a good patch to commit this week, as it will just break other, more important, patches. But that doesn't mean we can't get it RTBC-ready, with just a quick reroll (hopefully scripted) when we want to commit.

I have spoken to webchick and she suggested that she would be available to review/commit this next Monday, 31st March. If there's a patch here on Monday that applies then I'll review it.

Thanks @ianthomas_uk

Other than the reroll, the patch in #58 looks good to me.

@yanniboi, @ianthomas_uk thanks for review
So leave it for Monday 31h March.
Is script work well?

Worked with @heddn in corementoring on reroll of #58.

All occurrences have been correctly replaced.

Committed and pushed to 8.x. Thanks!