Move views_ajax_form_wrapper() to ViewsFormBase

Um. If this is a novice task, we will need more information added to the issue about what needs to be added to the documentation. Looking at https://api.drupal.org/api/drupal/core!modules!views!includes!ajax.inc/f... ... I am clueless as to what is needed.

Also... Is this an internal-use function for within Views, or something developers need to know about? If it's for developers, perhaps it needs an @ingroup line to link it up to a topic? If it's internal use... it only seems to be called in one place -- shouldn't it just be part of that class instead of being a stand-alone function?

Assigning back to dawehner for more information to be added. Please. :)

I don't know of a "nearly novice" tag, but if you put explicit instructions in the issue summary, one of our very capable "repeat novice" people will probably grab the issue and take care of it. Don't underestimate the novices -- we also have mentoring. :)

I've taken a stab at an issue summary.

But ... again, shouldn't this really be moved into the Views class that uses it? Why is it still in the module?

Attached you'll find a patch adding a documentation block for this function. Please let me know if I can make any improvements on the doc style for this file!

Thanks for the patch! Great start! A few things to fix:

a)

+ /**
+ * Wrapper for handling AJAX forms.

The indentation here is wrong. /** should be all the way to the left, and then in the other lines, * should line up with the first * in the /** line.

b)

+ * @param $form_id
+ *   The name of the form arg ('Drupal/...FooForm') which will be wrapped.

In the function signature, the parameter here is actually $form_class... However, probably we should change the argument to be $form_arg (will require the code to be changed, but that is OK because changing local variable names is considered a documentation change).

Also, the instructions in the issue summary said to copy some documentation; that documentation was not copied, so we don't really have all the information we need and the punctuation is not really OK.

c)

+ *   An array that stores information about the form.
+ *   See \Drupal\Core\Form\FormBuilderInterface::buildForm() for details.

Needs line wrapping attention. "See" should be on the previous line.

d)

+ *   See \Drupal\Core\Form\FormBuilderInterface::buildForm() for details.
+ * @return

There should be a blank line between the @param section and the @return, and the @return line should include a type. See https://www.drupal.org/node/1354#types for how to document types.

e) The first parameter needs a type on it too.

f)

+ *   Returns one of three possible values

Needs : at end of line before a list. Also each list item needs to end in . See
https://www.drupal.org/node/1354#lists

g)

+ *   - An AjaxResponse object, for an Ajax response: Drupal\Core\Ajax\AjaxResponse

How about "A \Drupal\Core\Ajax\AjaxResponse object." instead?

h)

+ *   - A render array with the form title and the rendered form in it.

Looking at this, I realize we probably should be more explicit. The code seems to be putting the title in #title and the form in #markup array elements, so let's mention this.

Thanks for that. Could you clarify a bit on this correction?

b) Also, the instructions in the issue summary said to copy some documentation; that documentation was not copied, so we don't really have all the information we need and the punctuation is not really OK.

Quite a bit of documentation there! I looked at both the documentation on the FormBuilderInterface function in and then the FormBuilder.php file referenced on that page.

Sorry for not being clear!

For the $form_arg parameter (well, it's currently called $form_id but we would like to rename it $form_arg), copy the docs from $form_arg on https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Form!FormBuilderI...

Here's an updated patch with changes. I realized after changing everything I failed to do an interdiff (sorry!) so I hope it's ok just submitting a new patch with changes. (Did the original patch using a different machine).

Most of the notes were pretty straightforward, but a few changes that might need attention:

-For $form_arg, I'm pretty sure I pulled the right documentation snippet but please double check.

-For the @return type, I designated as mixed and based that decision on phpDocumentor standards: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...

-Clarified the render array return for #title and #markup. Does that look ok?

Thanks for working on this!

A couple more notes.

1. +++ b/core/modules/views/includes/ajax.inc
   @@ -10,11 +10,25 @@
   - * Wrapper around \Drupal\Core\Form\FormBuilderInterface::buildForm() to handle
   ...
   + * Wrapper around drupal_build_form to handle some AJAX stuff automatically.
   

   drupal_build_form() is gone, this should reference \Drupal\Core\Form\FormBuilderInterface::buildForm() as it did before.

2. +++ b/core/modules/views/includes/ajax.inc
   @@ -10,11 +10,25 @@
   + * @param \Drupal\Core\Form\FormInterface|string $form_arg
   + *   The value must be one of the following:
   + *   - The name of a class that implements \Drupal\Core\Form\FormInterface.
   + *   - An instance of a class that implements \Drupal\Core\Form\FormInterface.
   

   This is perfect now!

3. +++ b/core/modules/views/includes/ajax.inc
   @@ -10,11 +10,25 @@
   + * @param array $form_state
   + *  An array that stores information about the form. See \Drupal\Core\Form\FormBuilderInterface::buildForm() for details.
   

   The second line needs to be indented one more space (should line up under the first "A" in @param).

   You'll also need to wrap under 80 characters, so right after the "See".

4. +++ b/core/modules/views/includes/ajax.inc
   @@ -10,11 +10,25 @@
   + * @return mixed
   

   I'd probably replace this mixed with \Drupal\Core\Ajax\AjaxResponse|string|array

5. +++ b/core/modules/views/includes/ajax.inc
   @@ -10,11 +10,25 @@
   + *  Returns one of three possible values:
   + *  - A \Drupal\Core\Ajax\AjaxResponse object.
   + *  - The rendered form, as a string.
   + *  - A render array with the title in #title and the rendered form in the #markup array.
   

   Each of these lines should be indented one more space

Updated patch with #9 changes. Please review.

I see the changes and tests passed. Thanks for the assist, this was my first patch!

Yay! Congrats @jessi_pantheon, great work. Thanks @er.pushpinderrana for the last push.

I believe following line should split into 2 lines so that 80 characters limit can be maintained,

+ * - A render array with the title in #title and the rendered form in the #markup array.

Please review the updated patch for the same.

Close enough ;)
Thanks @amitgoyal.

Um... This bit in the @return isn't quite right:

+ *   - A render array with the title in #title and the rendered form in the
+ *     #markup array.
 

Last word should be "property".

Can we fix up the top of this documentation too? Sorry for not mentioning this sooner...

/**
+ * Wrapper for handling AJAX forms.
+ *
  * Wrapper around \Drupal\Core\Form\FormBuilderInterface::buildForm() to handle
  * some AJAX stuff automatically.
  * This makes some assumptions about the client.

- The first line should start with a verb, and tell what the function does, like "Handles Ajax forms for the Views module.".
- AJAX should be Ajax.
- The next paragraph... "automatically"? "some Ajax stuff"? "some assumptions"? Really this is not very good documentation. But at a minimum, it should be wrapped better.

I took a look at what this function actually does, and would like to propose replacing the 2nd paragraph with:

Builds the form and either creates an Ajax response that displays it as a dialog, or returns the rendered form.

Also, can I just ask again: Why is this function not a method on the Views class that is the only place it is called, rather than being a stand-alone function?

Because "document" generally doesn't include refactoring. Expanding the scope.

It would help to call the new method.

hah! Now we're talking:

    // @todo Remove the need for this.
     \Drupal::moduleHandler()->loadInclude('views_ui', 'inc', 'admin');
-    \Drupal::moduleHandler()->loadInclude('views', 'inc', 'includes/ajax');

!!

I am definitely +1 on this patch.

I suppose we should make sure this is working though? This is used in the Views UI module for all the places with Ajax forms, I think. So we should test and make sure that it's working. I do not think that the automated tests will catch possible problems with this patch, because as far as I know they are not able to test Ajax/JavaScript behavior.

Tagging for Amsterdam.

Suggestion:
- start with updating the issue summary with the task you need to do, which is what jhodgdon says.
- do it :)
- comment on what you have tested manually.

Updated the issue summary with remaining tasks.

As this latest patch is an API change, we also need a brief draft change notice written.

Will do the manual testing.

Manual testing failed, since $form_state is a classed object now. I'll try to update the code.

Replaced the code in ajaxFormWrapper with the one in ajax.inc.

Where's the rest of the patch? It's missing ajax.inc changes. Also please provide an interdiff against #19.

My bad. I'll provide the complete patch later today.

Complete patch. Somehow patchutils keeps giving errors on creating the interdiff.

Thanks!

This is not the right syntax:

+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -1,7 +1,9 @@
 <?php
 
 /**
+ *
  * @file
+ *

The blank lines here should not be added. Reference for how we do @tags is
https://drupal.org/node/1354

Also, this has too much indentation:

   * @param string $type
-   *   The type of the item this form is manipulating.
+    *    The type of the item this form is manipulating.
    */

Please fix up the patch; I stopped looking there. The changes you made to existing code were wrong...

Rerolling with a fresh git pull and from patch #19 :D

Wrote the Change Record(its my first one). https://www.drupal.org/node/2383927
Let me know the improvements if needed.

The formatting for the documentation header for the new method is still not right -- mostly indentation and wrapping issues. Please look at the sections at
https://www.drupal.org/node/1354#functions
https://www.drupal.org/node/1354#lists
for examples and standards.

The change record draft looks pretty good, although I think maybe it should say somewhere that your class would need to extend ViewsFormBase (and give the full namespace of this class!) rather than just saying it changed to using $this->ajaxFormWrapper() without context of what $this is?

@jhodgdon,
Updated the CR as specified.Although I think there is no other place where views_ajax_form_wrapper is using through out the code in D8. Similarly in D7 views_ajax_form_wrapper . Do we need to specify in the CR how do we use this function in contrib module with example ?

I edited the change record.

The patch still needs work, see #36.

Corrected indentation and wrapping. Please correct me if I'm wrong.
Sorry, I can't create an interdiff because the previous file doesn't apply.

+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -180,6 +180,74 @@ public function getForm(ViewStorageInterface $view, $display_id, $js) {
+      $form['#attached']['css'][] = drupal_get_path('module', 'views_ui') . '/css/views_ui.admin.css';

@dawehner it's already there from @tim.plunkett's patch in #19 But do you think that I should remove it?

Since I think I made a mistake on indenting some part of the docs I'm attaching a patch to fix that.

Added a draft change records here https://www.drupal.org/node/2398961 ,added beta evaluation, and changed the status.

After discussions with @catch - My first thought is to punt to 8.1 but implementing a BC layer would onerous. Given that views has been brought in from contrib and has plenty of legacy code like this and the clean up has no disruption on core I'm going to commit under the committer discretion provision in the beta policy.

+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -178,6 +178,78 @@ public function getForm(ViewStorageInterface $view, $display_id, $js) {
+    if (!$ajax && $title) {
+      $form['#attached']['css'][] = drupal_get_path('module', 'views_ui') . '/css/views_ui.admin.css';
+    }

As per #42 this does not need to be here.

working on this

Okay, fixed that.

Sorry, forgot to change the status

Committed 7b83db3 and pushed to 8.0.x. Thanks!

Thanks for including the beta evaluation in the issue summary.

publish the change record.