Add new Form API example module for Drupal 8

Still working on this.

@chrisjlee, can you share some of the work. Even rough code will do.

...

WIP patch; currently a mess :)

This is one of the first stable patches:

• Creates the first form tutorial example
• Creates a page controller for the initial form welcome screen
• Created a stub yml file for local menu tasks

I Just need to figure out how to create local menu tasks in D8.

Completed porting over the tutorial examples steps 1-7.

• Currently need to work on figuring out how to do multi-step forms. I'm considering using the config system; but that seems like overkill for a multistep form.

So i still need to find some decent examples in core for multistep forms and complete tutorial forms 8-10, complete the form wizard, etc.

As one approach, looks like opportunity to extend base form multistep forms: #1886616: Multistep Form Wizard

- Added multiform wizard example for form example number 8; fields swap out as you use the next button

Currently, trying to figure out how to store form field data from one step to the other. Might use the \Drupal\user\TempStore Class. Open to ideas.

Unassigning for now. Getting busy with some work. Going to come back to it if no one picks this up.

Hi, I will work on this

I finished the 10 tutorials about Forms and i am going to work on test now.

The tutorial number 10 was a bit difficult because file_move changed [1] and the first parameter is a FileInterface.

I continue ;)

[1] https://api.drupal.org/api/drupal/core!modules!file!file.module/function...

I upload my progress because it is a long work.

I have a problem at test 10 and it is taking me a long time

I continue.

Changes
https://www.drupal.org/node/2315253
in implementations of the buildForm(), validateForm() and submitForm() methods.

- public function validateForm(array &$form, array &$form_state) {
+ public function validateForm(array &$form, FormStateInterface $form_state)

When there is work to be done and the patch is not final, 'needs work' is the right status.

Going to work on this for the TCDrupal sprint today.

Fixed the form state and form set errors due to https://www.drupal.org/node/2310411.

I rather strongly dislike simply numbering the examples. I would rather have a descriptive name for each.

Examples 1 though 6 also have no useful submit or validate handlers. I think we need to rethink which examples we have since I think the complexity in forms isn't in building the form, but handling the submit and validation.

I agree that we should rethink the form examples. My first experience with the first example of this module was: WTF does this example want to explain. I suggest to start with a simple, but working example form. One you can copy past and have working code in your module. But let's not derail this issue now and get it working first. We can draft a new example outline in a separate issue.

I made the following comment on #2319591: Rethink the form examples:

I am slowly but surely working on rewriting form elements api documentation so that it is complete and documents all the new elements available in D8. See #2486967: [meta] Move/Create Form Element Documentation. I am busy putting sample code snippets in every Element's implementation. I have been working in a sandbox module to prove my code samples before including them in core documentation patches, so it seems like there may be some overlap here.

It seems to me that it would be a good idea to create simple examples and then consider expanding on them in subsequent patches, but in the interest of "working code wins", would it be ok to make a really simple version of the Form Example module and put in working examples of these controls? I'd be willing to host a sandbox module to provide an easier way of collaborating on this.

I am starting to take the tact of simple form examples. I've assigned myself to this issue just because I am working hard on the new forms api docs and am going to start submitting patches based on that work. I agree that there are a lot of oddities in the examples here, and that starting with simple examples makes more sense. There's also a lot of interesting topics like modal forms etc, that might be interesting to cover in small incremental patches.

Here is a the first draft at a fresh start on a simple form example for drupal 8. I'd recommend that we consider creating child issues or new issues for new form examples? This is a simple example to get the discussion started. It is a fresh start, so no interdiff provided.

Woops forgot to adjust tests. I'll submit another patch but would love comments on the approach.

The patch is 0 bytes long. :-)

How embarassing.... trying again.

This patch is failing in a module I didn't modify. Going to need some time to figure out fields_api to correct this problem. Is the normal protocol you follow here to create a separate issue for the test that's failing and try and solve that one before committing this one?

Actually, yes, those were branch blockers which have been fixed.

Restarting the test to see...

In general a good start :) Thank you for getting this issue moving again. Do get your coding standards right though! Use Code Sniffer.

1. +++ b/form_example/form_example.routing.yml
   @@ -0,0 +1,14 @@
   +form_example.simple_form:
   +  path: 'examples/form_example/simple_form'
   +  defaults:
   +    _form:  '\Drupal\form_example\Controller\SimpleForm'
   +    _title: 'Simple Form Example'
   +  requirements:
   

   Could use some comment here about using '_form' for form controllers. And about '_access: TRUE' giving access to anonymous.

   Is it a good example to give anonymous access? What does D7 form_example do?

2. +++ b/form_example/form_example.routing.yml
   @@ -0,0 +1,14 @@
   +  requirements:
   +    _access: 'TRUE'
   \ No newline at end of file
   diff --git a/form_example/src/Controller/Page.php b/form_example/src/Controller/Page.php
   

   Add an empty line at the of the file pls. Other files too. Highly recommended to use Code Sniffer to check your files.

3. +++ b/form_example/src/Controller/Page.php
   @@ -0,0 +1,29 @@
   +      '#markup' => t('<p>Form examples to demonstrate commen UI solutions using the Drupal Form API.</p>'),
   +    );
   

   $this->t() instead of t().
   Move

   outside of t string.

4. +++ b/form_example/src/Controller/Page.php
   @@ -0,0 +1,29 @@
   diff --git a/form_example/src/Controller/SimpleForm.php b/form_example/src/Controller/SimpleForm.php
   
   diff --git a/form_example/src/Controller/SimpleForm.php b/form_example/src/Controller/SimpleForm.php
   new file mode 100644
   
   new file mode 100644
   index 0000000..079ab5e
   

   Form controllers should be in src/Form.

5. +++ b/form_example/src/Controller/SimpleForm.php
   @@ -0,0 +1,65 @@
   +/**
   + * Created by PhpStorm.
   + * User: metzlerd
   + * Date: 6/9/15
   + * Time: 1:45 PM
   + */
   +
   

   Standard Drupal @file thingy here.

6. +++ b/form_example/src/Controller/SimpleForm.php
   @@ -0,0 +1,65 @@
   +namespace Drupal\form_example\Controller;
   +
   

   Drupal\form_example\Form of course.

7. +++ b/form_example/src/Controller/SimpleForm.php
   @@ -0,0 +1,65 @@
   +class SimpleForm extends FormBase {
   

   Add some comments about extending base forms. There are other base forms too. How to select? Why this one?

   Perhaps a line of comments on where to place form controller file.

8. +++ b/form_example/src/Controller/SimpleForm.php
   @@ -0,0 +1,65 @@
   +  public function getFormId() {
   +    return 'form_example_simple_form';
   +  }
   +
   

   Some comment on what the purpose of the formId. Tjhat is has to be unique side wide.

9. +++ b/form_example/src/Controller/SimpleForm.php
   @@ -0,0 +1,65 @@
   +  /**
   +   * @param array $form
   +   * @param FormStateInterface $form_state
   +   * @param null $extra
   +   * @return array
   +   *
   +   * Build the simple form.
   +   */
   +  public function buildForm(array $form, FormStateInterface $form_state, $extra = NULL) {
   +
   

   If we want this to be a (by the book) example, we should use {@inheritdoc}. But we want to add descriptive comments too. Need to discuss this.

10. +++ b/form_example/src/Controller/SimpleForm.php
    @@ -0,0 +1,65 @@
    +    $form['title'] = array(
    +      '#type' => 'textfield',
    +      '#title' => t('Title'),
    +      '#required' => TRUE,
    +    );
    +
    +    $form['actions'] = array(
    +      '#type' => 'actions',
    +    );
    +
    +    $form['actions']['submit'] = array(
    +      '#type' => 'submit',
    +      '#value' => t('Submit'),
    +    );
    +
    +    return $form;
    

    Yes, a simple form. I like that.

    Use $this->t() instead of t().

11. +++ b/form_example/src/Controller/SimpleForm.php
    @@ -0,0 +1,65 @@
    +  /**
    +   * @param array $form
    +   * @param FormStateInterface $form_state
    +   * Implements a form submit handler.
    +   *
    +   * The submitForm method is the default method called for any submit elements
    +   */
    +  public function submitForm(array &$form, FormStateInterface $form_state) {
    

    If you are listing the @param-s then do it properly pls. Missing description on $form, no indent on "Implements ..."
    End a comment sentence ("The submitForm ...") with a dot.

12. +++ b/form_example/src/Controller/SimpleForm.php
    @@ -0,0 +1,65 @@
    +    /*
    +     * This would normally be replaced by code that actually does something
    +     * with the title.
    +     */
    +    $title = $form_state->getValue('title');
    

    We don't need to excuse ourselves. We should tell that this is where the submit processing takes place. ::getValue and d_s_m() are valid and usefull examples though.

13. +++ b/form_example/src/Controller/SimpleForm.php
    @@ -0,0 +1,65 @@
    +
    

    Perhaps add a validation too, it is common enough to have validation and it makes it a nice copy-paste-start-my-own-form example.

So I have a couple of questions....

1.) Why should all form controllers be in src/Form? Is there some common reason for this or standard that I'm missing. It seems generally inconsistent with MVC and IMHO, it confuses new developers. Doing so makes Form Controllers seem very different form page controllers (In some ways they are and in some ways they aren't.) There are lots of different types of controllers (some of which are even forms). I'll just follow along if that's what the group wants, but looking at the code tree as a newbie, I read "there are Controllers and Forms" which almost implies that a form is not a special case of a controller but rather something different than a contoller.

2.) Regarding access. I borrowed the "everyone can see it" from other D8 examples modules in the examples projects. What privileges should we be checking? We could use "access content" or we could invent one in the examples module? Should we create a separate issue to discuss examples permission strategy or have that discussion here? By the way... most of the examples in the old code are doing form_alter rather than actually making a form, so they dodge the permissions issue.

3.) Excuse my ignorance but what does {@inheritdoc} actually do? I've never used it before, but have seen it in doc code I've been working on, so it's hard for me to weigh in on how it should be used properly.

I'll look into code sniffer and work on fleshing out more comments.

Finally is this generally the direction we want to go? It seems right to me, but I don't want to put in a ton of work to find in the end that we don't want examples to cover the basics, but rather only advanced cases, or that I need to port all the old code instead? That's a task I don't really have the time or interest for.

Oh and I definitely should have said... thanks for the review!

1.) Why should all form controllers be in src/Form?

All core modules do it this way. Perhaps it is a Drupalizm, but I'm not going to fight that battle here.

2.) Regarding access. borrowed the "everyone can see it" from other D8 examples modules in the examples projects.

That is fine. Follow the other examples. Perhaps I should create an issue about it then.

3.) Excuse my ignorance but what does {@inheritdoc} actually do?

See http://manual.phpdoc.org/HTMLSmartyConverter/PHP/phpDocumentor/tutorial_...

As far as I'm concerned, I am fine with this direction. The examples should be usable for 1. copy-past and 'run'; 2. An isolated working piece of code to learn and understand how it works.

Some more basic example I would like to see:
* Basic form with some of the most used field types (incl. #markup)
* Configuration form with default configuration and loading and saving of configuration
* A confirmation form

Some advanced forms:
* Fields with states (form_example_states)
* Multi-step form (tutorial step 8 and/or for_example_wizard)
* Form with ajax that add new elements (tutorial step 9)
* Form with file upload (tutorial step 10)

Thanks for the tip about code sniffer. I got it integrated to my PHP Storm environment. Here's the next stab.

Woops missed a some class rename stuff. Back for another round.

Much better already. A few comments based on the interdiff.

1. +++ b/form_example/form_example.routing.yml
   @@ -11,4 +22,4 @@
   diff -u b/form_example/src/Controller/Page.php b/form_example/src/Controller/Page.php
   
   diff -u b/form_example/src/Controller/Page.php b/form_example/src/Controller/Page.php
   --- b/form_example/src/Controller/Page.php
   
   --- b/form_example/src/Controller/Page.php
   +++ b/form_example/src/Controller/Page.php
   
   +++ b/form_example/src/Controller/Page.php
   +++ b/form_example/src/Controller/Page.php
   @@ -2,21 +2,23 @@
   
   @@ -2,21 +2,23 @@
    
    /**
     * @file
   - * Implement simple page controller for simple pages
   + * Implement simple page controller for simple pages.
     */
   +
   

   Use Contains Drupal\form_example\Controller\Page. in the @file.
   See any Core class for examples.

2. +++ b/form_example/src/Tests/SimpleFormTest.php
   @@ -51,7 +52,7 @@
   --- /dev/null
   
   --- /dev/null
   +++ b/form_example/src/Form/SimpleForm.php
   
   +++ b/form_example/src/Form/SimpleForm.php
   +++ b/form_example/src/Form/SimpleForm.php
   @@ -0,0 +1,114 @@
   
   @@ -0,0 +1,114 @@
   +<?php
   +/**
   + * @file
   + * Provides simple form example.
   

   Perhaps not defined in code style, but Core uses an empty line between <?php and the first docblock.

And @todo the remaining items of #41.

Next.

A few more...

1. +++ b/form_example/src/Form/SimpleForm.php
   @@ -0,0 +1,115 @@
   +    $form['title'] = array(
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Title'),
   +      '#required' => TRUE,
   +    );
   

   Add a description for the minimum length.

2. +++ b/form_example/src/Form/SimpleForm.php
   @@ -0,0 +1,115 @@
   +  public function validateForm(array &$form, FormStateInterface $form_state) {
   +    $title = $form_state->getValue('title');
   +    if (strlen($title) < 5) {
   +      // Set an error for the form element with a key of "title"
   +      $form_state->setErrorByName('title', $this->t('The must be at least 5 characters long'));
   +    }
   +  }
   

   Use a period to end the comment (code style).
   Typo in message: The title must be ...

ok.

Thank you!

Looking good... Just a few things. :-)

1. +++ b/form_example/form_example.routing.yml
   @@ -0,0 +1,26 @@
   +form_example.description:
   +  path: 'examples/form_example'
   ...
   +form_example.simple_form:
   +  path: 'examples/form_example/simple_form'
   

   These routes show up as separate links in the tool menu. Let's make the description a parent of the form item (with the parent: key).

2. +++ b/form_example/form_example.routing.yml
   @@ -0,0 +1,26 @@
   +# TODO: Determine an appropriate right to check for this example.
   +  requirements:
   +    _access: 'TRUE'
   

   I think we don't want to demonstrate permissions for pages here. Let's just give access to everyone. We'll demo permissions in a routing example.

Will work on these changes, but wouldn't it be better to test against the 'access content' right rather than having copied demo code be inherently insecure? I don't mind letting everyone view it, but for sites that require auth, we'd be technically bypassing security? I know it's only examples, but we could/should model at least the minimal expected security? It still just one property so I don't see how that's a big deal. Anyway the TODO came as a recommendation from @suthersan so he might want to weigh in as well.

Possible strategies:

• Leave the TODO in as a marker for any developer who copies this code to make sure they implement form security.
• Change all the examples routes to use the "access content" right so that minimal security is modeled properly
• Change all the examples modules code to use a "view code examples" right advertised by the examples module.

As a side note we should probably create a separate issue to rethink example module security.

we should probably create a separate issue to rethink example module security

Agreed. Please do, Be bold ;)

Changes per @Mile23.

Ok... Security issue created: #2567501: [meta] Add _permission: 'access content' to applicable routes

Hmmm... The link checking for the all example modules is failing testing (not just form_example). Current version of this in d8 fails testing locally as well. I could use some help figuring out what's going on here.

OK, I refactored the tests so that they will pass. I sort of expect this patch to fail, but not in the form examples module.

Yep.. there are two branch blockers that are causing tests to fail. Will file an issue to that affect.

After our talk at PNWDS I think the whole module at https://github.com/metzlerdl/d8_examples would make a great addition.

It's mostly ready, it looks like.

Just some more inline comments would be great, with some tests for navigation and so forth.

Working on a patch to include fapi_examples in the examples module, based on the work in https://github.com/metzlerdl/d8_examples .

New patch file and interdiff attached. I didn't have time to write the tests for all of the form examples provided in https://github.com/metzlerdl/d8_examples . I also made a few small adjustment to the submit handler functions in the forms for which I did write tests.

Patch completing all tests and including some refactors based on PNW Summit talk.

One more time for the test bot.

Unsure why only some of the tests are being run here, but I think I've verified that these tests pass/work now.

Dave

Thanks. :-)

Some review of #69.

1. +++ b/fapi_example/fapi_example.routing.yml
   @@ -0,0 +1,84 @@
   +# Access to these paths is restricted to users with the permission 'access content'.
   +# This is notated as _permission: 'access content'.
   

   Wrap to 80 chars.

2. +++ b/fapi_example/src/Form/AjaxDemo.php
   @@ -0,0 +1,100 @@
   +  private $colors = [
   

   Needs a docblock. Explain what methods will use this property.

3. +++ b/fapi_example/src/Form/AjaxDemo.php
   @@ -0,0 +1,100 @@
   +
   +    //
   +    $form['color_wrapper'] = [
   

   ...and? :-)

4. +++ b/fapi_example/src/Form/AjaxDemo.php
   @@ -0,0 +1,100 @@
   +  /**
   +   * Getter method for Form ID.
   +   *
   +   * @inheritdoc
   +   */
   

   Should be just {@inheritdoc}.

5. +++ b/fapi_example/src/Form/BuildDemo.php
   @@ -0,0 +1,172 @@
   +  public function __construct() {
   

   {@inheritdoc}

6. +++ b/fapi_example/src/Form/BuildDemo.php
   @@ -0,0 +1,172 @@
   +  /**
   +   * Getter method for Form ID.
   +   *
   +   * The form ID is used in implementations of hook_form_alter() to allow other
   +   * modules to alter the render array built by this form controller.  it must
   +   * be unique site wide. It normally starts with the providing module's name.
   +   *
   +   * @return string
   +   *   The unique ID of the form defined by this class.
   +   */
   +  public function getFormId() {
   

   Same deal: This should only be {@inheritdoc}, and the special behavior should be documented as inline code.

7. +++ b/fapi_example/src/Form/BuildDemo.php
   @@ -0,0 +1,172 @@
   +  public function getFormId() {
   +    static $i = 0;
   +    $i++;
   +    drupal_set_message("getFormId $i");
   +    return 'fapi_example_simple_form';
   +  }
   

   Why are we generating a static and doing dsm and then returning a string?

8. +++ b/fapi_example/src/Form/BuildDemo.php
   @@ -0,0 +1,172 @@
   +  /**
   +   * @param array $form
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   * Demonstrates ajax button submit.
   +   */
   +  public function ajaxSubmit(array &$form, FormStateInterface $form_state) {
   

   Needs a one-line comment in the docblock.

9. +++ b/fapi_example/src/Form/BuildDemo.php
   @@ -0,0 +1,172 @@
   +  /**
   +   * @param array $form
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   * Test a submit that causes form state rebuild
   +   */
   +  public function rebuildFormSubmit(array &$form, FormStateInterface $form_state) {
   

   Needs a one-line comment in the docblock.

10. +++ b/fapi_example/src/Form/BuildDemo.php
    @@ -0,0 +1,172 @@
    +    drupal_set_message("rebuildFormSubmit $i");
    

    Still not sure why we need this pattern.

Here is another stab trying to address the feedback. I refactored the build demo, but the basic idea behind the buildDemo from was to demonstrate the firing order of the various events that you implement in a form controller. I refactored the display into a method so that static variables weren't needed and/or duplicated. I used this example in my summit presentation to illustrate the order of events firing, but I'm ok if you think we should remove this from the examples module. Just let me know what you'd prefer to do here.

Woops forgot to change tests after updating the output. This should succeed. Sorry.

Here's a partial review:

Not sure if it's us or them, but:

1. +++ b/fapi_example/src/Form/BuildDemo.php
   @@ -0,0 +1,156 @@
   +   *   The method being invoked. ¶
   

   Whitespace.

2. +++ b/fapi_example/src/Form/ContainerDemo.php
   @@ -0,0 +1,99 @@
   +  /**
   +   * Getter method for Form ID.
   +   *
   +   * @inheritdoc
   +   */
   

   {@inheritdoc} only.

3. +++ b/fapi_example/src/Form/DemoBase.php
   @@ -0,0 +1,52 @@
   +/**
   + * Implements common submit handler used in fapi_example demo forms.
   + *
   + * This extends FormBase which is the simplest form base class used in Drupal.
   + * The class is primarily used to provide a common SubmitForm method for other
   + * form controllers to use.
   + *
   + * @see \Drupal\Core\Form\FormBase
   + * @see \Drupal\Core\Form\ConfigFormBase
   + */
   +abstract class DemoBase extends FormBase {
   

   Convert to the 'we' form, so that it says "We extend FormBase..."

   It's also unclear which class you mean when you say "The class is primarily used..."

   Pretty sure we don't need @see for ConfigFormBase but we do need @see for all the forms that subclass it.

   A lot of the form class docblocks don't say a lot about what the class does.

4. +++ b/fapi_example/src/Form/StateDemo.php
   @@ -0,0 +1,85 @@
   +/**
   + * Implements the state demo form controller.
   + *
   + * The submit handler for this form is implemented by the DemoBase class.
   + *
   + * @see \Drupal\Core\Form\FormBase
   + * @see \Drupal\Core\Form\ConfigFormBase
   + */
   +class StateDemo extends DemoBase {
   

   Explain what the controller shows.

The vertical tabs spacing issue will get fixed by #2541252: Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs.

Fixed items 1-3 in comment #75.

Several more stylistic changes and comments.

Thanks for all your work yesterday at the sprint.

Working off of #78...

+++ b/fapi_example/src/Form/VerticalTabsDemo.php
@@ -0,0 +1,77 @@
+/**
+ * Implements the vertical tabs demo form controller.
+ *
+ * This class extends FormBase which is the simplest form base class used in
+ * Drupal.
+ *
+ * @see \Drupal\Core\Form\FormBase
+ * @see \Drupal\Core\Form\ConfigFormBase
+ */
+class VerticalTabsDemo extends DemoBase {

Let's add a @todo here, saying something like: Verify this form works after https://www.drupal.org/node/2541252

So that people can look at the code and find out why their vertical tabs look weird.

There are also a lot of coding standards errors, but we can do a follow-up on that.

Sorry to have been away so long, but am back and starting to work on this.

Here's the next stab. Unfortunatley, examples drifted too much for me to be able to get an interdiff. What's changed:

* I made sure there were comments on all the form classes.
* I changed the use of $this->l to Link->createFromRoute as $this->l() is now deprecated.
* I did a code sniffer review of the project and made corrections.
* Several typos were corrected.

Hopefully we can get this added to examples soon.

Dave

Woops this one has a couple of typo corrections in addition that I missed in my last code inspection run.

So I tellya what I'm gonna do...

I think this is usable and informative enough to be committed and pushed, so people can refer to it to lean FAPI. I'll do coding standards work before committing.

I have some specific review stuff to deal with, but they can all be follow-ups. List in the issue summary.

Added links to follow-ups.

Thanks @Mile23,

FYI: I made a copy based on the last patch that I'm using to continue efforts on updating the core documentation. Particularly what I was working on was adding elements to the InputDemo form, but wanted to make sure we got the main example committed. If we think it's a good idea, I can submit separate issues regarding those changes after the documentation effort is complete. See #2599742: Document several form elements that have no docs for a more detailed list of other form elements that may want to be added in a future update.

Feel free to make issues about any changes you'd like to see on this and I'll watch for them and work on them as time permits.

Yah, def make a new issue about changes. There are already a few child issues to this one. Check the sidebar. :-)