Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Even though it's not a port, but a new module, it's a sub-issue for meta issue #1880976: [meta] Port examples (including submodules) to D9.4+
D8 all the things!
Proposed resolution
Make an example module for FAPI.
Remaining tasks
Even though it's marked fixed, this module needs some more attention.
Some major things:
Add a FAPI Example component to the project.- Follow up on our @todo about vertical tabs: #2686579: @todo in Drupal\fapi_example\Form\VerticalTabsDemo
- We don't have a .module file, which is where we do the sort of overview of the in-code documentation, and do the @defgroup for the module. #2686585: Ensure fapi_example module has @defgroup and @addtogroup
- The links on the description page don't match the child items in the menu. We really should just leave them off the description page and let the user navigate the menu.
- Not a lot of inline comments.
- Description page is lacking.
- Each of the form pages should have descriptive text at the top. For instance, I'm not sure what the Build Form Demo page is showing me.
Minor:
- The multi-select table example in the common elements page doesn't have a label below it.
- Vertical tabs doesn't add a summary to the tab.
- The tests could be consolidated into one test class with methods for each route to visit.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#82 | examples-fapi_example-2102659-82.patch | 53.17 KB | metzlerd |
| |||
#81 | examples-fapi_example-2102659-81.patch | 53.17 KB | metzlerd |
| |||
#78 | examples-fapi_example-2102659-78.patch | 53.01 KB | zerbash |
| |||
#77 | interdiff-2102659-72-77.txt | 5.3 KB | zerbash |
#77 | examples-fapi_example-2102659-77.patch | 53.02 KB | zerbash |
|
Comments
Comment #1
chrisjlee CreditAttribution: chrisjlee commentedComment #2
chrisjlee CreditAttribution: chrisjlee commentedStill working on this.
Comment #3
Sutharsan CreditAttribution: Sutharsan commented@chrisjlee, can you share some of the work. Even rough code will do.
Comment #4
chrisjlee CreditAttribution: chrisjlee commented...
Comment #5
chrisjlee CreditAttribution: chrisjlee commentedWIP patch; currently a mess :)
Comment #6
chrisjlee CreditAttribution: chrisjlee commentedThis is one of the first stable patches:
I Just need to figure out how to create local menu tasks in D8.
Comment #7
chrisjlee CreditAttribution: chrisjlee commentedCompleted porting over the tutorial examples steps 1-7.
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.
Comment #8
chrisjlee CreditAttribution: chrisjlee commentedComment #9
chrisjlee CreditAttribution: chrisjlee commentedAs one approach, looks like opportunity to extend base form multistep forms: #1886616: Multistep Form Wizard
Comment #10
chrisjlee CreditAttribution: chrisjlee commented- 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.
Comment #11
chrisjlee CreditAttribution: chrisjlee commentedUnassigning for now. Getting busy with some work. Going to come back to it if no one picks this up.
Comment #12
jjprieto CreditAttribution: jjprieto commentedHi, I will work on this
Comment #13
jjprieto CreditAttribution: jjprieto commentedI 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...
Comment #14
jjprieto CreditAttribution: jjprieto commentedI 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.
Comment #15
jjprieto CreditAttribution: jjprieto commentedComment #16
jjprieto CreditAttribution: jjprieto commentedComment #17
jjprieto CreditAttribution: jjprieto commentedChanges
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)
Comment #23
jjprieto CreditAttribution: jjprieto commentedComment #24
Sutharsan CreditAttribution: Sutharsan commentedWhen there is work to be done and the patch is not final, 'needs work' is the right status.
Comment #25
socketwench CreditAttribution: socketwench commentedGoing to work on this for the TCDrupal sprint today.
Comment #26
socketwench CreditAttribution: socketwench commentedFixed 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.
Comment #28
Sutharsan CreditAttribution: Sutharsan commentedI 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.
Comment #29
Sutharsan CreditAttribution: Sutharsan commentedComment #30
metzlerd CreditAttribution: metzlerd as a volunteer commentedI made the following comment on #2319591: [No patch] Rethink the form examples:
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.
Comment #31
metzlerd CreditAttribution: metzlerd as a volunteer commentedHere 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.
Comment #33
metzlerd CreditAttribution: metzlerd as a volunteer commentedWoops forgot to adjust tests. I'll submit another patch but would love comments on the approach.
Comment #34
Mile23The patch is 0 bytes long. :-)
Comment #35
metzlerd CreditAttribution: metzlerd as a volunteer commentedHow embarassing.... trying again.
Comment #37
metzlerd CreditAttribution: metzlerd as a volunteer commentedThis 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?
Comment #38
Mile23Actually, yes, those were branch blockers which have been fixed.
Restarting the test to see...
Comment #39
Mile23Comment #41
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedIn general a good start :) Thank you for getting this issue moving again. Do get your coding standards right though! Use Code Sniffer.
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?
Add an empty line at the of the file pls. Other files too. Highly recommended to use Code Sniffer to check your files.
$this->t() instead of t().
Move
outside of t string.
Form controllers should be in src/Form.
Standard Drupal @file thingy here.
Drupal\form_example\Form of course.
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.
Some comment on what the purpose of the formId. Tjhat is has to be unique side wide.
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.
Yes, a simple form. I like that.
Use $this->t() instead of t().
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.
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.
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.
Comment #42
metzlerd CreditAttribution: metzlerd as a volunteer commentedSo 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!
Comment #43
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedAll core modules do it this way. Perhaps it is a Drupalizm, but I'm not going to fight that battle here.
That is fine. Follow the other examples. Perhaps I should create an issue about it then.
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)
Comment #44
metzlerd CreditAttribution: metzlerd as a volunteer commentedThanks for the tip about code sniffer. I got it integrated to my PHP Storm environment. Here's the next stab.
Comment #46
metzlerd CreditAttribution: metzlerd as a volunteer commentedWoops missed a some class rename stuff. Back for another round.
Comment #47
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedMuch better already. A few comments based on the interdiff.
Use
Contains Drupal\form_example\Controller\Page.
in the @file.See any Core class for examples.
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.
Comment #48
metzlerd CreditAttribution: metzlerd as a volunteer commentedNext.
Comment #49
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedA few more...
Add a description for the minimum length.
Use a period to end the comment (code style).
Typo in message: The title must be ...
Comment #50
metzlerd CreditAttribution: metzlerd as a volunteer commentedok.
Comment #51
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedThank you!
Comment #52
Mile23Looking good... Just a few things. :-)
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).
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.
Comment #53
metzlerd CreditAttribution: metzlerd as a volunteer commentedWill 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:
Comment #54
metzlerd CreditAttribution: metzlerd as a volunteer commentedAs a side note we should probably create a separate issue to rethink example module security.
Comment #55
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedAgreed. Please do, Be bold ;)
Comment #56
metzlerd CreditAttribution: metzlerd as a volunteer commentedChanges per @Mile23.
Comment #57
metzlerd CreditAttribution: metzlerd as a volunteer commentedOk... Security issue created: #2567501: [meta] Add _permission: 'access content' to applicable routes
Comment #59
metzlerd CreditAttribution: metzlerd as a volunteer commentedHmmm... 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.
Comment #60
metzlerd CreditAttribution: metzlerd as a volunteer commentedOK, I refactored the tests so that they will pass. I sort of expect this patch to fail, but not in the form examples module.
Comment #62
metzlerd CreditAttribution: metzlerd as a volunteer commentedYep.. there are two branch blockers that are causing tests to fail. Will file an issue to that affect.
Comment #64
Mile23After 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.
Comment #65
danmuzyka CreditAttribution: danmuzyka as a volunteer and at Phase2 commentedWorking on a patch to include fapi_examples in the examples module, based on the work in https://github.com/metzlerdl/d8_examples .
Comment #66
danmuzyka CreditAttribution: danmuzyka as a volunteer and at Phase2 commentedNew 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.
Comment #67
metzlerd CreditAttribution: metzlerd as a volunteer commentedPatch completing all tests and including some refactors based on PNW Summit talk.
Comment #69
metzlerd CreditAttribution: metzlerd as a volunteer commentedOne more time for the test bot.
Comment #70
metzlerd CreditAttribution: metzlerd as a volunteer commentedUnsure why only some of the tests are being run here, but I think I've verified that these tests pass/work now.
Dave
Comment #71
Mile23Thanks. :-)
Some review of #69.
Wrap to 80 chars.
Needs a docblock. Explain what methods will use this property.
...and? :-)
Should be just {@inheritdoc}.
{@inheritdoc}
Same deal: This should only be {@inheritdoc}, and the special behavior should be documented as inline code.
Why are we generating a static and doing dsm and then returning a string?
Needs a one-line comment in the docblock.
Needs a one-line comment in the docblock.
Still not sure why we need this pattern.
Comment #72
metzlerd CreditAttribution: metzlerd as a volunteer commentedHere 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.
Comment #74
metzlerd CreditAttribution: metzlerd as a volunteer commentedWoops forgot to change tests after updating the output. This should succeed. Sorry.
Comment #75
Mile23Here's a partial review:
Not sure if it's us or them, but:
Whitespace.
{@inheritdoc} only.
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.
Explain what the controller shows.
Comment #76
PapaGrandeThe 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.
Comment #77
zerbash CreditAttribution: zerbash commentedFixed items 1-3 in comment #75.
Comment #78
zerbash CreditAttribution: zerbash commentedSeveral more stylistic changes and comments.
Comment #79
Mile23Thanks for all your work yesterday at the sprint.
Working off of #78...
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.
Comment #80
metzlerd CreditAttribution: metzlerd as a volunteer commentedSorry to have been away so long, but am back and starting to work on this.
Comment #81
metzlerd CreditAttribution: metzlerd as a volunteer commentedHere'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
Comment #82
metzlerd CreditAttribution: metzlerd as a volunteer commentedWoops this one has a couple of typo corrections in addition that I missed in my last code inspection run.
Comment #83
Mile23So 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.
Comment #84
Mile23Comment #85
Mile23Added links to follow-ups.
Comment #86
metzlerd CreditAttribution: metzlerd as a volunteer commentedThanks @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.
Comment #87
Mile23Yah, def make a new issue about changes. There are already a few child issues to this one. Check the sidebar. :-)