Problem/Motivation

Currently form_builders design is to manipulate Form API arrays. When saving the configuration those Form API arrays are transformed into the format needed for the form type (ie. webform components). This works fine as long as every property can be mapped 1:1 to an $element['#property'] . Problems start when it doesn't:

  • When one form_builder element doesn't map 1:1 to a Form API #type -> See the workarounds for select/option fields.
  • When the form type doesn't produce an $element['#property'] for every property form_builder fails to find already stored values for that property. Saving the form then leads to the stored values being erased.

Proposed resolution

    >
  1. Refactor properties/elements into a plugin system that directly shows the needed interface for every property/element. A backward-compatible implementation (invoking the hooks) should be provided. The current behavior can be implemented as a base-cllass for earch plugin-type.

    2. /li>

Perhaps some of those shouldn't be done in 7.x-1.x.

Remaining tasks

  • Refactoring: Properties as objects / plugins.

User interface changes

None planned.

API changes

New object oriented API - fully backwards compatible.

CommentFileSizeAuthor
#4 form_builder-oop-combined-2464957-4.patch49.57 KBtorotil
#2 form_builder-refactor-form_type-and-properties-2464957-2.patch25.41 KBtorotil

Comments

torotil’s picture

torotil commented
Issue summary: View changes
torotil’s picture

torotil commented
StatusFileSize
new form_builder-refactor-form_type-and-properties-2464957-2.patch25.41 KB

Here is my first attempt at refactoring. It uses plugins for form-types and properties. There is only one minor API-change needed: hook_form_builder_types_alter() is now called without the defaults inserted. With the refactoring I was already able to remove the 1:1 renderable mapping in form_builder_webform.

The downside is that it still is only a transformation of the original code. The API could still use some cleanup.

I'd like to get some additional eyes on this - or at least let it sit for a while before moving on.

torotil’s picture

torotil commented
Status: Active » Needs review
torotil’s picture

torotil commented
Issue summary: View changes
StatusFileSize
new form_builder-oop-combined-2464957-4.patch49.57 KB
fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

Great work here @torotil. I plan to review this properly in the next few days.

mausolos’s picture

mausolos commented

I was not able to get the patch to work for display classes, which are my primary concern at the moment (ref. from https://www.drupal.org/node/2463259).

I attempted to apply form_builder-oop-combined-2464957-4.patch to my codebase. As a sidenote, it failed both my drush make and my git apply -v (with no error message, in spite of -v), which I thought was a bit strange.

I manually applied the patch changes, line-by-line, so it's possible I might have made a mistake somewhere.

1) I created a new form.
2) I added a fieldset (using form_builder's UI page).
3) I added a class to the fieldset.
4) I saved the webform.
5) Before doing anything else, I checked the fieldset CSS classes field again, and found it to be empty.
6) I loaded the form_builder UI view in a completely different browser (Chrome vs. FF), to be sure form_builder's aggressive cache wasn't doing something funky. It also loaded without the CSS classes that I had entered
7) If I don't save the new blank form_builder UI of my form, but just go to view the form after adding the class, the class still renders as expected.

I hope this was of some use. I look forward to seeing the working solution!

Charles

torotil’s picture

torotil commented

The patch was generated using git diff you can apply it with patch -p1.

Using webform-7.x-4.x and form_builder-7.x-1.x + the patch from number 4 I did the following:

0) apply the patch via curl https://www.drupal.org/files/issues/form_builder-oop-combined-2464957-4.patch | patch -p1
1) create a new form
2) add a fieldset to the form and a textfield within it
3) add a class to the fieldset
4) reloaded the fieldset configuration form (clicking twice on the pen) and check whether the class is still set -> class was set
5) save the form, open the fieldset configuration form again and check whether the class is still set -> class was set.
6) view the webform, check whether the class is in the output -> class was in the output.

@mausolos Could you please check whether the patch was applied correctly. This sounds a lot like you're testing the plain 7.x-1.x.

Please keep those reviews coming ;)

  • torotil committed d22b325 on 7.x-1.x 
    Issue #2464957: Refactor form types and properties as classes.

API-...
torotil’s picture

torotil commented
Status: Needs review » Fixed

I've added additional documentation to the patches and committed them to 7.x-1.x -- It would still be good to have someone else opinion on the changes, though.

I guess the next step is to turn elements/fields into plugins too.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.