Problem/Motivation
The D7-style component API has too many limitations and side-effects to write about.
Proposed resolution
Convert components to annotated, classes plugins that also implement PluginFormInterface to render the actual form elements. Use separate classes (as configured in the plugins' annotations) for rendering values or administrative configuration elements (don't put too many things in one class, basically).
All this makes for easier testing, and PluginFormInterface aides in component validation and submission. I can understand if you want to look at how Field API does this, but please don't. It isn't designed too well, mostly because we didn't get many useful features until after the API was redesigned for Drupal 8, even the maintainers agree to this.
Remaining tasks
T.B.D.
User interface changes
None.
API changes
Many. Yay!
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | interdiff-2293945-27-28-do-not-test.diff | 45.59 KB | tedbow |
| #28 | webform-components_to_plugins-2293945-28.patch | 34.83 KB | tedbow |
Comments
Comment #1
twistor commentedPoint of clarification, shouldn't the PluginFormInterface be for plugin configuration? We would use a separate class, WidgetSomethingInterface, for rendering user-facing forms.
Comment #2
paulmckibbenChecking on the status of this task. Has any progress been made toward a plugin architecture for components? @fenstrat/@DanChadwicik, is this something @ttamniwdoog and I can help with? Hate to jump in if you're just about to release a solution. Thanks!
Comment #3
danchadwick commentedI am only passively working on the D8 branch -- specifically applying D7 patches and managing issues so that D7 changes don't get lost.
Comment #4
fenstratNo, nothings been done here @paulmckibben, if you and @ttamniwdoog want to jump in here that would be seriously appreciated! Even if it's just a WIP post a patch here, this is the exciting stuff of moving to D8!
Comment #5
danchadwick commentedIs there an OOP roadmap for this? Do you imagine using inheritance for components (such as abstract class WebformComponent) or are you thinking of an interface (such as WebfromComponentable)?
Should there be some architecture discussion before the coding begins?
Comment #6
fenstratNo there's no roadmap here as of yet Dan, I've simply not had the time. Going to take a very rough first pass at it, working on that now.
Comment #7
fenstratOk here's a very early WIP, but amazingly it actually works.
I've not got any more time for this now. Whoever wants to pick it up just make sure you assigning the issue to yourself before you start work so we don't step on each others toes.
Comment #8
ttamniwdoog commentedI've tested the patch and I have not seen an error related to this WIP. I'm going to spend some time over the next few days trying to add to what you've done here. Thanks for getting this started, which was the hard part :-)
Comment #9
fenstratGreat, thanks @ttamniwdoog!
Comment #10
ttamniwdoog commentedAdded the remaining shells for plugins following fenstrat's lead there.
I'd like to know what you envision as the next steps are here.
Since we cannot adequately test these components as plugins without having a route to view them.
I think DanChadwick inquired about a roadmap and I think that would be great but I think that might be a tall order.
I'm happy to continue to help but I'll need a little direction.
Thanks again!!
Comment #11
michelleJust noting that I've picked this up from Matt. Not a lot of progress so far. Still trying to get the form working to add components. Will be working on it more tomorrow.
Comment #12
fenstratAny progress here @Michelle? Even a WIP. I'm spending next week on webform and this is one of the first patches I'd like to move forward.
Comment #13
michelleOh! I'm so sorry. I had a lot of work in progress and then we realized that the client only needed a contact form and switched to doing what was needed for that. I had intended to take some of my own time to finish the in progress stuff and submit a patch and it just got lost in the mad rush of days.
When you say "next week" do you mean _next_ week as in starting on May 11th or this next week as in starting today? If it's the former, I'll spend some time this week getting it cleaned up and submitted. If you need it sooner, I'll just make a patch out of what I have as is.
Comment #14
fenstratNo worries at all Michelle! Yeah by next week I mean the one starting May 11th. I'd like to get going this weekend 9th/10th so wherever you're at by the end of this week put it up as a patch and I'll jump in from there. Cheers.
Comment #15
michelleOk, will do!
Comment #16
michelleSorry this is late. I forgot about timezones and figured first thing in the morning would be ok but I suppose Saturday is half gone for you by now. :(
Anyway, it's been serious crunch time at work and all my brainpower has been sucked up there so I did not have time to clean this up. When I tried to do a diff against a fresh clone, I was getting changes that I don't think should be part of this so I ended up doing a diff against the last commit before I started making changes, which is this one:
This patch includes the prior patch on this issue.
It's been a while since I've looked at the code but I think I was following the image module example of imagestyle and effect with the webform being the imagestyle and the component being the effect. I believe I was in the process of making a webform config entity based on the imagestyle one when I got pulled off it. I know that's out of scope for the components to plugins on this issue but it seemed like the next logical step to put it all together and I was trying to get this working for the client at the time, not just focused on this issue.
Sorry it's not a clean and clear patch. :( Hopefully you can still make use of it.
Comment #17
fenstratNo worries, thanks Michelle. As you'd not posted anything I started work on converting /node/{node}/webform, i.e. the component add/edit page for an individual webform. But it's kind of a chicken and egg situation as without the conversion to plugins quite a deal of that needs reworking based on this. So I'll work on merging what I'd started there with what you've got here. Tricky as there's so many broken things! Anyway, thanks for your work here.
Comment #18
michelleYeah, that's the hard thing about porting modules is there is so much broken to start with that you keep having to fix one thing to get another thing working and so on.
I am pretty sure I had /node/{node}/webform working as far as being able to actually build a webform. You couldn't do anything with it but you could put the components on it. One of the next steps was to move the components' form code from the .inc files into the buildForm() of each plugin. I started with TextField and I _think_ I had that working but I'm not positive. I switched away from this on April 17 so it's all a little fuzzy. These are my last notes before the switch:
Comment #19
fenstratPretty sure @Michelle isn't working on this, and I'm not currently either. I think there's just not enough functionality actually working in webform 8.x-4.x to attempt this conversion of components to plugins. Therefore I'm working towards trying to get some of the basic functionality ported and working in D8, and ideally some or all of the tests ported as well. Once that is done this will be my top priority. Having said that if someone is interested in continuing this now by all means go for it.
Comment #20
michelleNo, I'm not anymore. My contrib time is all over the place based on what clients need, unfortunately. It's wonderful that the company gives back so I don't want to sound like a complaint but it does make it hard to stay focused on a project for the long haul. Hopefully as we get more D8 clients someone else will need this. :)
Comment #21
safetypinHere's a clean patch from the current 8.x-4.x branch. I didn't make any changes to anything, hopefully I didn't make any mistakes either.
Comment #22
safetypinOf course, I did actually make a mistake: I put the Annotations folder in the wrong place. I'm hiding my previous patch.
Comment #23
safetypinHere's a corrected reroll of patch #16 against the main 8.x-4.x branch.
Comment #24
ianthomas_ukThis reroll has conflicted with #2488934: Convert webform_components_form to D8 which has already implemented WebformComponentsForm. You should not be replacing code from that issue unless you have a reason to.
Comment #25
safetypin@ianthomas_uk Ah, I suppose I should have realized that since file exists that it was committed from another issue. I'll remove that part of the patch.
Comment #26
safetypinI've gone through the commit, http://cgit.drupalcode.org/webform/commit/?id=db6601c, from #2488934: Convert webform_components_form to D8, and as far as I can tell, the only conflicting changes are the src/Form/WebformComponentAddForm.php file. So, I've removed that section of the patch. I'll hide the old patch, since it contained the error. @ianthomas_uk, do you see any other problems with this reroll?
Comment #27
safetypinSo, I've made a rough first-pass at a webform component that can actually be saved to the database. I really just copied over the old insert code, and the only thing I adjusted was the code to lock the database before incrementing a component's cid.
Comment #28
tedbowI this is an important step I have added some stuff to the last patch.
This patch allows you to add and edit components on a webform. I have only been testing in on Text Field. I would suggest getting it working on 1 or two component types to figure out the ComponentInterface will need to be and what can live in ComponentBase before filling out the rest of the components.
I am providing and interdiff but here is what this patch adds
Comment #30
podarok@tedbow I've merged your work. As for me it looks good. Still needs work, but let's plan out progress to make small changes to be able to test them easily. Ping me directly if you'll have new additions
Comment #31
podarokI'm suggesting to work on most popupal components at first
- textfield
- email
- checkbox
- select
- phone
Comment #32
podarokI can be a bit crazy, but let's start to talk about making webform as part of Contact module
It already has all needed components, except of submissions storage
Comment #33
michelleIf you want to go that route, here's the storage: https://www.drupal.org/project/contact_storage
Comment #34
podarok@Michelle - wow
Thanks you tons
The only thing is to write upgrade path, I guess
Comment #35
michelleNo problem. That's the route we ended up going for a project. I don't know if it will meet everyone's needs that uses webform but it works very well for a general contact form.
Comment #36
safetypin@podarok, have you considered the peformance implications when scaled? I'm not an expert, but this has been discussed before, and I don't think it was resolved. It may well be a very unusual case, but some d7 webforms have hundreds of fields, and hundreds of thousands of submissions. I don't have any that are that big, but others have voiced this concern.
Comment #37
michelleJust noticed... Looks like Acquia went this route as well: https://www.drupal.org/node/2603724#comment-10601556
Comment #38
naveenvalechamy two cents
https://www.ostraining.com/blog/drupal/drupal-8-contact-forms/
TL; DR
I believe that the storage mechanism of webform module is different and would provides performance boost.
Comment #39
larowlanNote what I said about field map, number of content-types etc might not be so big a problem on D8 because of work done in #2482295: Rebuilding field map with many bundles/fields is very slow
Comment #40
andypostenabled should have default value in base plugin
Better to have getCapabilities()/defaultValues() method to return available options and their defaults
Comment #41
fenstratClosing to clear out the old Webform 8.x-4.x branch. See #2827845: [roadmap] YAML Form 8.x-1.x to Webform 8.x-5.x.