Views forms can't be used multiple times on the same page

Let's ensure we won't forget tests ...

There is an update of the patch using Bojan's suggestions
adding some code for testing
- New Views form field plugin/hanlder
- New test view to be used, it uses this Views form field and Global null argument (fixed)
- New Route for the testing page using:
- New controller to build page with the defined view embeded twice
- New Views test to operate on the page built - incomplete.

It seems there is an issue to have Views forms in the page - the page is built without forms - content only.
So it needs work.
Maybe we need to update also the issue status.

Here's an interdiff between #1 and #5.

1. +++ b/core/modules/views/src/Form/ViewsForm.php
   @@ -66,6 +66,13 @@ class ViewsForm implements FormInterface, ContainerInjectionInterface {
   +  protected $viewArgs;
   

   Is it just me that $view_args instead of $view_arguments is a bit annoying?

2. +++ b/core/modules/views/tests/modules/views_test_data/src/Controller/ViewsTestFormMultipleController.php
   @@ -0,0 +1,44 @@
   +  public function testPage() {
   +    $build = [
   +      'view_arg1' => [
   +        '#prefix' => '<div class="view-test-form-multiple-1">',
   +        '#suffix' => '</div>',
   +        '#type' => 'view',
   +        '#name' => 'test_form_multiple',
   +        '#display_id' => 'default',
   +        '#arguments' => ['arg1'],
   +        '#embed' => TRUE,
   +      ],
   +      'view_arg2' => [
   +        '#prefix' => '<div class="view-test-form-multiple-2">',
   +        '#suffix' => '</div>',
   +        '#type' => 'view',
   +        '#name' => 'test_form_multiple',
   +        '#display_id' => 'default',
   +        '#arguments' => ['arg2'],
   +        '#embed' => TRUE,
   +      ],
   +    ];
   

   I'm curious whether we should just go with twice the same block on the same page? This would save a bunch of the code here

Working on this. Looks like we were missing the following from the test, so that the custom field plugin is discovered

  /**
   * {@inheritdoc}
   */
  protected function viewsData() {
    $data = parent::viewsData();
    $data['views_test_data']['job']['field']['id'] = 'field_form_button_test';
    $data['views_test_data']['job']['field']['click sortable'] = FALSE;
    $data['views_test_data']['id']['field']['click sortable'] = TRUE;
    return $data;
  }

The view is now rendering the test buttons on each row and a save button for each view. Just need to write interactions.

Here we go, test now runs and ensures each submit button runs its appropriate form.

Per #7, going to rename variable and instead of using custom page, place as blocks on homepage.

@mglaman i can confirm that the #11 patch works.
But the "confirmation" text should be the message provided by the button submission not a custom text.

But the "confirmation" text should be the message provided by the button submission not a custom text.

Could you elaborate? It is checking the message set by the button. The message says the view ID, display, and the row index. We then check for that.

Currently simpletest, as far as I could see, only allows specifying a form ID and not specific button. I added row index so we could hopefully actually test that in future.

dawehner, I started working on placing the blocks. However, it doesn't seem we can pass view arguments through a block's settings, only the items per page. I've left the page controller as it is since the block option does not seem feasible yet.

dawehner, I started working on placing the blocks. However, it doesn't seem we can pass view arguments through a block's settings, only the items per page. I've left the page controller as it is since the block option does not seem feasible yet.

Fair point.

? Bot says test fail but results show different ¿

This needs a change record for the constructor change to ViewsForm.

While it's a genuine bug fix, it's changing some @internal API surface, so moving to 8.1.x first. If this is blocking a stable Drupal Commerce release we'll need to think about a patch-release-safe way to do it in 8.0.x.

@bojanz pinged me in irc. In this case we don't expect anyone to call the constructor directly or override - this isn't a service. So we shouldn't do a change record because it's not something we're expecting people to have to update for. However the theoretical chance that someone might means it's still a default 8.1.x-only change unless we really, really can't wait (and then we might need to change the patch a bit).

Aren't form IDs changing doesn't this mean that potential custom code that altered one of this forms would have to change? Is that not worth a CR?

That's a good point.

Also it's a bit worse than that - hook_form_FORM_ID_alter() would have been usable before, but won't be now because the arguments change. Unless we can somehow make hook_form_BASE_FORM_ID_alter() work but this isn't using hook_forms() at all.

I'll work on the CR as noted in #24.

In regards to #25, I'll see how we can preserve the original form ID for hook_form_BASE_FORM_ID_alter . We just need to stuff it into the build info.

From \Drupal\Core\Form\FormBuilderInterface::prepareForm()

    // Invoke hook_form_alter(), hook_form_BASE_FORM_ID_alter(), and
    // hook_form_FORM_ID_alter() implementations.
    $hooks = array('form');
    if (isset($build_info['base_form_id'])) {
      $hooks[] = 'form_' . $build_info['base_form_id'];
    }
    $hooks[] = 'form_' . $form_id;

EDIT: By adding the form ID without arguments (as it is now) in the base form ID, no existing code should be broken.

Here is an updated patch. This adds a base form ID so that existing hooks work as expected, but the form ID is unique based on arguements.

Here is change record draft: https://www.drupal.org/node/2664936

Arg, forgot to change status!

Don't we need tests for that last change, and to ensure it fixes #25?

Yes I think we do, that would have been a proper API break and we very nearly missed it.

Here is a test that addresses #30 and #31

Not sure what I did to roll such a borked patch. Here is proper one.

Thanks a lot. We now have a test to ensure we don't break existing hook_form_alter() implementations.

That's great. Committed/pushed to 8.1.x, thanks!

published change record.