There is merit for having composite elements that can be customized to the needs of a site without requiring the creation of an independent composite definition in code.

This is a feature based upon the requests in issues like #2862863 and #2824714.

Problem/Motivation

Code based composites are great for features that will be used on many sites (Address, Phone numbers, Links, etc) but are not great for elements that may be desired on multiple forms within the same site. As composites are implemented, you can either rebuild the custom composite on each form or build the element as a new class file. There is no in between. The ability to implement something that bridges this gap would simplify the number of individual composites the admin must maintain.

Proposed resolution

Extend the composite base to implement a custom composite element definition, using a driver and config entity to track the element structure.

Limitations of composite implementation

The developed patch provides a number of desired features. This includes the interface for building the composites, based on the custom composite interface, as well as the required entity types and pages. There however are some structural cruft that was required as the result of the way the composites were designed.

The patch provides a few changes to the way that the composite Element is handled. The element data cannot be directly connected to the Element definition since these classes are accessed statically. The implementation of composites retrieves the composite data without knowledge of the providing WebformElement, since each is paired directly.

This architecture then requires that a number of functions in the Element\WebfromCompositeBase that are overridden needlessly to implement a version of the getCompositeElement function that is aware of the WebformElement's derivative id. This modification would be better back ported to the CompositeBase and could make other composites more extensible.

Additionally there could be merit to eliminate the requirement of the ReusableElementDefinition all together if all composites stored the element metadata on the WebformElement and not the render Element definition.

Todo:

Assuming this feature is a logical thing to implement and can be done in a clean and organized way, the follow are features that would be useful to implement as well.

  • Provide a button (or other action) that allows an admin to convert a custom composite into a reusable composite on the custom composite page.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

richgerdes created an issue. See original summary.

richgerdes’s picture

Title: {Your title should be descriptive and concise} » Provide reusable custom composites
Issue summary: View changes
FileSize
34.67 KB

The attached patch provides a basic implementation.

There may still be more work to be done or restructuring, see summary for details.

richgerdes’s picture

Issue summary: View changes

Typos.

jrockowitz’s picture

@richgerdes Wow, I thought you were just making a recommendation and then I see this huge patch.

Honestly, right now, I can't support/add this level of functionality to the core webform module. I think this feature should be developed as a separate contrib module and you can get all the glory/gory of being a module maintainer.

You can probably take over the webform_composite.module namespace and use the WebformComposite entity namespace.

I will review and commit any patches that are needed to tweak to the existing WebformComposite API.

If you can make into the NYC Drupal Meetup, I can help you plan out the general architecture and by you a beer.

richgerdes’s picture

The patch may seem huge at first glace, but i think there is actually less going on then you think. The Element\WebformReusableComposite class thats provided is mostly to work around the api limitations, and the right changes to the api could eliminate it all together.

Beyond that the WebformReusableCompositeFrom takes a good deal of its code from WebformComposite::buildCompositeElementsTable(). Which possible could be broken out into its own element type and leveraged in both locations without the need for the duplicate code, and would make the WebformComposite class cleaner.

The remaining classes make up the required code for the entity, the listing page, and plugin definition and deriver, which are all fairly boiler plate.

While writing this, i am seeing that there is actually a number of ways to simplify this feature and reduce the duplicate. I am going to take another stab at this and see if I can make the patch a little more user friendly. If you still think its too complicated a feature for webforms core, I can work on breaking it out into a standalone module.

jrockowitz’s picture

My concern is that this feature will open up a huge can of worms. Many people (including myself) are going to expect a full UI with element dialogs, condition logic, multiple value support, file uploads, etc... for reusable composites.

I am more comfortable telling most developers to create reusable composite elements as plugins.

richgerdes’s picture

I understand that you don't want to create a large amount of technical dept. But i don't think this creates any new gaps in feature support. Things like conditional logic and file uploads from composites are two features that are already lacking in the design of composites. This feature (as currently designed) works within the same element limitations as the Custom composite entity you recently built. The implementation is also designed to as simply as possible to remap the getCompositeElements function implemented in the Composite render Element, to the WebformElement, without requiring reworking the entire composite api. Which means that the UI that is currently available for any code defined composites will be used without any changes those dialog. If their are particular aspects of the UI for reusable composites that you would want to see. I can put them other. There is also no need to complete this feature requests since there is a bit of work required for it, proper review and the desire for complete implementation are desirable for me as well.

If I am wrong and there are aspects of the UI and other features that this would make desirable, I am happy to spend the time needed to get those features implemented, either as part of this request or independently. If its simply a concern about the effort to upkeep, i would be happy to join you as a maintainer of the webform 8.x-5.x.

I took another stab and found a few bugs in it as well and made some improvements to the api and structure. Along with the patch i am providing simplified diffs breakdowns for its 3 major aspects; API changes to reduce the logic needed, The config entity for storing the customized elements, and the new WebformElement plugin. This might help you follow the changes since there are a number of files touched. There are probably still some outstanding improvements that should be made, let me know if you have any thoughts.

The bulk of the API change is providing the custom composite editing interface as a render element (`webform_composite_builder`) so i can reuse it for the entity. The other changes allow for the support of derivative plugins by using getBaseId() for the render element lookup and providing the render array to getCompositeElements(). Parts 2 and 3 individually add the entity type and the element plugins respectively.

Please take a few minutes to look at whats going on with the changes. I think this would be a more valuable part of webforms core then it would as a standalone module. If not, i will see about building it standalone.

richgerdes’s picture

I forgot to rebuild that patch after making a last minute change. Attached patch resolves the syntax issue and the other last minute change.

richgerdes’s picture

Fixed composite definition in test and example.

jrockowitz’s picture

I did a quick review of the UI/UX and code, and I have to say WOW. What you are doing makes a lot of sense.

Still, I am targeting an RC for December 25, and I need to stop adding major features. I think reusable composite elements should be a separate sub-module and probably live in a new contrib project.

A few thoughts/notes...

  • What I like most about your solution is that it can evolve with the Webform module.
  • People need to be able to edit reusable composite element's YAML source.
  • Reusable composite elements must be translatable, which requires some custom code.
  • The reusable composite UI should be under 'Configuration' (/admin/structure/webform/config) after 'Elements' and maybe just called 'Composites'.
  • We need to figure out the best namespace. Maybe just webform_composite.module or webform_shared_composite.module. Personally, I like using the word 'shared' over 'reusable.'
  • How do we handle data collected from an updated or deleted composite element?
  • Since all webform elements are optional, moving the UI and code into a submodule will allow people to disable this feature.

A next step would be to move the reusable composite element into a sub-module which will make it easier to move into a dedicated contrib module and isolate the changes that are needed to core webform module.

I am open to adding co-maintainers to the Webform module, but I need people to first help me with the issue queue and general maintenance before having them implement new features. Everyone, including myself, needs to help improve the test coverage and help migrate the SimpleTests to PHPUnit.

richgerdes’s picture

Thanks for looking this over. Now knowing that there is a December RC on the table, I defiantly think making this a separate project is a smart choice. If its ends up being a useful feature we can work towards integrating it later. I originally built it as a separate module ('webform_dynamic_composite' in private development), so I can easily break it back out again.

For naming, i am definitely not tried to the word 'reusable' but shared also seems a little off to me. For the module, what do you think of 'webform_composite_tools'? Since really its another way to build out composites. We can then leverage the 'webform_composite' entity type, if you don't think there is the need for that in the scope of webforms. and can use the classname WebformComposite as well. The only case for needing the 'reusable'/'shared'/'dynamic' wording would be in the id of the WebformComposite plugin, which i would propose moving the exiting custom composite to 'webform_custom_composite' to leave the 'webform_composite' open. That's probably not smart since its already been release (as beta) with the id, unless we want to worry about making update steps for it. So, maybe 'webform_global_composite' might be a viable option. I like it a little more since 'shared' feels like you make a custom composite and duplicate it to another form, and not build it globally (maybe that's just me though).

I think adding source back in is desired, only left out since i had a hard time figuring out how to make the tabbed dialog used in the other Webform UI's. I can take a shot at the translation, just point me in the right direction as to what needs to be worked on.

Can you elaborate on the data handling question? I think there may need some improvements, but i am not sure exactly what you are getting at.

I can help with cutting through the issue queue. Though taking a quick review of whats still open, you have handled most of the low hanging fruit in the queue. Is there a system your using to determine what you are planning on adding to the RC? Test coverage is something I (regretfully) have not had the changes to get into with Drupal as heavily is I wish. Since migrating to PHPUnit is a goal, I would also be up for taking a shot at working on them.

jrockowitz’s picture

The Webform module is still in beta; we can make API changes.

We should change 'WebformComposite' to 'WebformCustomComposite' and change 'webform_composite' to 'webform_custom_composite' this will free up the 'WebformComposite' namespace for your sub-module. At the same time, we can create a WebformElementComposite element which would be the UI for existing custom composite and your global composites. I can probably knock this out work next week.

You should copy the UI/UX from the WebformOptions entity. Remaining tasks like translations and data handling can become separate action items.

If you are up for getting involved with helping to support the Webform module, our next step would be to set-up a weekly or bi-weekly Google Hangout and for now use the Drupal Slack channel for communication.

I am available to talk this Friday almost any time. We can start by reviewing general things like the roadmap and critical issues.

BTW, my goal is to get you set up with your module namespaces so that I can promote it and push people to test it.

jrockowitz’s picture

jrockowitz’s picture

I committed the patch from #2924551: Move the WebformComposite element to WebformCustomComposite and create a WebformElementComposite (Builder). .

There is now a \Drupal\webform\Element\WebformElementComposite builder which can be used by the WebformCustomComposite element and 'reusable/shared' WebformComposite elements.

The https://www.drupal.org/project/webform_composite namespace is available.

@richgerdes If you need any help feel free to ping me on the Drupal Slack channel.

richgerdes’s picture

Sorry for taking so long to close this out. I converted the patch into the webform_composite module, using the open namespace. I am still working on some of the improvements to the UI that we talked about, but wanted to get the existing base layed down, so this issue could get closed and progress tracked on the new project. Let me know if you would like to be a maintainer of the project.

The attached patch has the changes that would be desired from the api in order to support the webform_composite module as designed.

richgerdes’s picture

jrockowitz’s picture

Status: Active » Needs review

I am marking the patch as needs review to get the automated tests to run.

The last submitted patch, 7: 2921623-7-part_3-Element.diff, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

richgerdes’s picture

The actual test passed, one of older patches was what triggered the failure.

jrockowitz’s picture

I committed the patch. I will be tagging a new beta release next Sunday or Monday.

I also added a link to your module from the Webform modules list of add-ons.

@see https://www.drupal.org/docs/8/modules/webform/webform-add-ons#comment-12...

As soon as you tag an alpha release, I will mark your module in add-ons list as recommended.

richgerdes’s picture

Awesome, I will let you know as I make progress on the remaining tasks for the build out of the forms.

jrockowitz’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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