Hi marcusk,

Thank you for reaching out to with this.

As a first step, I would like to understand better your goal and the reasoning behind the feature request (before the decision for custom implementation of custom FormEngine).
You mentioned that you want to use the "built-in form functionality", could you tell us more about what have you tried and what is missing for you?
Have you looked into the Embedded Task Forms functionality (User guide)? Does it fit your needs?
After giving us more details on the above, I will continue with your concrete questions/bullets.

Best regards,
Yana

Hi Yana,

With "built-in form functionality" I meant the API for rendering forms (FormService::getRenderedTaskForm, which delegates to FormEngine for the actual rendering) and submitting forms (FormService::submitTaskForm, which delegates to FormHandler for converting the submitted values to process variables), and the API for customizing their behavior (by configuring a different FormEngine using processEngineConfiguration::setCustomFormEngines / by providing a custom FormHandler in camunda:formHandlerClass).

The built-in task form renderers (including Embedded Task Forms and Generated Task Forms) and default submit-handling do not fit my needs. To be more concrete, the way we do form handling is by providing a form class that contains (potentially nested) properties for every form field, and annotations containing meta-data about how a field should be rendered, validated etc. When a form is submitted the entered values are put into a new instance of the form class, which is then stored in a single process variable.

So just to be clear, Camunda already supports the configuration of a custom FormEngine, ánd providing custom FormHandlers. It's just that I've encountered some issues when actually trying to use these extension points. For this I have some improvements in mind.

Regards, Marcus

Hi Marcus,

Thank you for your explanation. I have a couple of more questions:

1. the functionality that is common to rendering and submission is needed by these two classes.
   > FormEngine and FormHandler are interfaces, maybe you can implement one class which implement both of them?
2. the FormHandler.submitFormVariables method has no access to the FormData.
   > It will be interesting to give us an example of your use case why do you need access to the FormData.
3. the only way to provide a custom FormHandler is by putting the camunda:formHandlerClass attribute on every task.
   > Did you consider implementing a ParseListener?

Hi Yana,

I've created a minimized version of our current implementation here, including the use of a ParseListener as you suggested. I'm relatively happy with this implementation, except for the breaking issue I mention in point 3.

2. It will be interesting to give us an example of your use case why do you need access to the FormData.

Remember that in our case the formKey contains the fully qualified name of the class that represents the form, which together with the deploymentId is used to load the class. This class is just as much needed for form submission as it is for rendering, because during submitForm a new instance of that class is created, and its fields are filled using the entered values. As a workaround I retrieve the formKey and deploymentId in parseConfiguration, so they can be used by submitFormVariables, which delegates most work to the FormEngine (where I think it should be), and then stores the resulting Form-instance in a process variable.

1. FormEngine and FormHandler are interfaces, maybe you can implement one class which implement both of them?

That's possible, but each task definition has its own FormHandler instance (since it's also responsible for storing the result of parseConfiguration), so you'd still need additional measures to be able to share state (e.g. caching, so we don't have to load en analyse the form class each time) between the form engine and the form handlers. The solution I chose is to have a reference to the FormEngine in each FormHandler.

3. Did you consider implementing a ParseListener?

Good idea, I did not think of that I used it in the linked implementation, and it works quite well in principle.

However, this has brought to light a fundamental issue with overring a FormHandler in a ParseListener. When ParseListener#parseUserTask is called, a FormHandler has already been assigned to the task (by BpmnParse::parseTaskDefinition), and its parseConfiguration-method has already been invoked (from the same place). Therefore parseConfiguration on the new FormHandler will never be called!

Now I don't want to come across as negative, so let me offer some suggestions instead of complaining

In the short term, a small enhancement would solve my primary issue: support the possibility to specify a default FormHandler-factory* in the ProcessEngineConfiguration. This default is then used (instead of "new DefaultTaskFormHandler()" as it is now) to assign a form handler when it isn't explicitly specified using camunda:formHandlerClass. This change would remove the need for a ParseListener to set the custom FormHandler, and therefore circumvent the problem I mentioned in point 3.
(* why a factory instead of a class? so we can easily pass additional constructor parameters)

On the longer term, if Camunda ever considers a redesign of the form handling API, I have the following ideas about that:

• FormHandler should only be responsible for parsing the task definition XML, and create FormData-instances based on that. It would also make sense to have it decide on the name of the FormEngine to use (e.g. based on the parsed XML, or bound to the FormHandler type), unless one is specified explicitly.
• FormEngine should be responsible for form rendering and submission.
• Perhaps have FormEngine parameterized on the subtype of (Start/Task)FormData, so FormHandler can easily pass additional data to the FormEngine it chose.

I think this would be a cleaner seperation of concerns, making the functionality easier to understand and extend.

I said:

In the short term, a small enhancement would solve my primary issue: support the possibility to specify a default FormHandler-factory* in the ProcessEngineConfiguration.

but while working on a PR for this, this turned out have more impact than I initially thought, because there's no clear access path from BpmnParse to the ProcessEngineConfiguration.

So I'm now working on the bug concerning the faulty interaction between ParseListeners and FormHandlers instead, which I probably should have in the first place That should solve my primary issue.

Hi Marcus,

Thank you for the provided input. I just want to verify that I understood you correctly.
You decided to choose the approach to implement a ParseListener. However, you stumble upon the problem that FormHandler has already been assigned to the task when the ParseListener#parseUserTask is called.
And now you are focused on resolving this problem, is that right?

Hi Yana,

That is correct.

A related issue applies to start form handlers: the parse listener is called before the start form handler is set. So if you set a custom one is the parse listener, it is happily overwritten by parseStartFormHandlers. I'm working on resolving this too.

Here is the pull request: https://github.com/camunda/camunda-bpm-platform/pull/444

Hello Marcus,

I am sorry for the delay with this.
I checked your pull request. I understand that ideally the suggest implementation will make sense.
However, the current implementation has its own benefits. As this is not a trivial change, I would purpose you to consider the following workaround:
Explicitly parse the configuration of the form handler in the implemented parseListener.parseUserTask() so that the correct custom form handler is assigned.
What do you think of this option?

Best regards,
Yana

Hi Yana,

If I understand you correctly your suggested workaround is to call
taskFormHandler.parseConfiguration(Element, DeploymentEntity, ProcessDefinitionEntity, BpmnParse) manually from
ParseListener.parseUserTask(Element, ScopeImpl, ActivityImpl).

I actually did consider this already, but I see no way to get access to the BpmnParse from ParseListener.parseUserTask? Also there is no obvious way to get hold of the DeploymentEntity.

Could you explain what the benefits are of the current implementation? Perhaps I can take this into account in the implementation and unit tests. To be frank, the current implementation seems clearly buggy to me, especially since start- and task form handlers work inconsistenly.

Regards, Marcus

Hi Marcus,

I am sorry for my late reply.

My suggestion was in this direction yes. I think you don't need an instance of BpmnParse, you can try something like:

  @Override
  public void parseUserTask(Element userTaskElement, ScopeImpl scope, ActivityImpl activity) {
    ExampleFormHandler exampleFormHandler = new ExampleFormHandler(formEngine);
    // ... 
    // the implementation for taskFormHandler.parseConfiguration()
    exampleFormHandler.setDeploymentId(scope.getProcessDefinition().getDeploymentId());
    exampleFormHandler.setFormKey(userTaskElement.attributeNS(BpmnParse.CAMUNDA_BPMN_EXTENSIONS_NS, "formKey"));
  }

I hope that helps.

Could you explain what the benefits are of the current implementation?

It's about structure and pattern. The parsing is done at the beginning. And again I do see the point of improving the code. However, at this point we will try to avoid such change.

Best regards,
Yana

Hi Yana,

I think you don't need an instance of BpmnParse, you can try something like: [...]

Ok, while this approach does work for task form handlers, we also need the same functionality for start form handlers, where the issue I mentioned previously is a showstopper:

A related issue applies to start form handlers: the parse listener is called before the start form handler is set. So if you set a custom one is the parse listener, it is happily overwritten by parseStartFormHandlers.

In other words, start form handlers cannot be set by a parse listener, because it will always be overwritten with the DefaultStartFormHandler. This is one of the issues my pull requests fixes. I'll try to elaborate below.

It's about structure and pattern. The parsing is done at the beginning. And again I do see the point of improving the code. However, at this point we will try to avoid such change.

My impression is that you may be overestimating how much the pull request changes the existing behavior, which is understandable since this ticket is getting rather long. The PR's scope is limited to fixing the bugs and inconsistencies I encountered in the interaction between start/form task handlers and parse listeners.

The current (before my PR) behavior for start form handlers is as follows:

• Call BPMN parse listeners.
• Parse the start form handler (or fall back to the default one).
• Call the form handler's parseConfiguration.
• Wrap the form handler in DelegateStartFormHandler and bind it to the process definition.

Whereas the current behavior of task form handlers is as follows:

• Parse the task form handler (or fall back to the default one)
• Call the form handler's parseConfiguration
• Wrap the form handler in DelegateStartFormHandler and bind it to the user task
• Call BPMN parse listeners

As you can see, the order of operations is different between the two variants (which is confusing in itself), each causing their own problems:

• In the case of start form handlers, since parseStartFormHandlers is called after the parse listeners are applied, form handlers set by the parse listeners are overwritten. (I don't think there's a workaround for this)
• In the case of task form handlers, since parseConfiguration is called before the parse listeners are applied, a form handler set by the parse listener will never have its parseConfiguration called. (You provided a workaround for this)

So what my PR attempts to achieve is the following:
1. Properly( * ) allow form handlers to be set from parse listeners. (* "properly" as in: that form handler functions as expected)
2. Have consistent behavior between start- and task form handlers.
3. Make the least amount of changes to the current behavior as possible.
4. If there's a choice, choose the most flexible option (which is why I chose the specific order listed below).

The resulting behavior is as follows for both start- and task form handlers:

• Parse the form handler (or fall back to the default one).
• Call BPMN parse listeners (which can overwrite the form handler from the previous step or wrap it if desired).
• Invoke parseConfiguration on whatever form handler is set.
• Wrap the form handler in a DelegateStartFormHandler and bind it to the process definition/user task.

And as you can see, parsing is still done at the beginning.

If you want me to add any unit tests to ensure there are no unintended changes, let me know.

Regards, Marcus

Hi Marcus,

Thank you for your patience with this and the detailed summary!

Indeed, I missed the previous time the problem with the start form handler.
I checked again and I have another proposal. In order to minimize the changes, I would suggest moving only the invocation of the parse listeners at the end of the #parseStartEvents. Just now I understand what is the impact of calling the listeners too early. Having the parse listeners invocation at the end of parse methods will guarantee to apply of the custom implementation once everything is parsed. This #parseStartEvents is only one of very few places where the listeners are not invoked at the end.
Could you please consider my suggestion and check if you can apply the previously provided workaround (from my previous comment) for the start handlers too.

Best regards,
Yana

Hi Marcus,

Due to inactivity, I am closing the ticket and the respective PR.
Please feel free to reopen it in case of questions or you want to continue to work on.

Best regards,
Yana

Hi Yana,
Please reopen the ticket. I'm working on some other things right now, but I'm going to try your suggestion after that and follow up on the result.
Regards, Marcus

Hi Yana,

Your suggested workaround does indeed work. I've created a pull request: https://github.com/camunda/camunda-bpm-platform/pull/496.

Regards, Marcus

Hi Marcus,

Thank you for your contribution.
I assume you were able to build the wanted custom form handler, am I right?
I will check the provided PR and merge it in case all look good.

Best regards,
Yana

Hi marcusk,

I merged the provided PR.
Do you still have some questions regarding the topic or we can close the ticket?

Best regards,
Yana

Hi Yana,

Indeed, in combination with the workarounds this enables us to use our custom form handlers. Thank you!

However to be totally honest with you I am a bit disappointed in how this issue was handled. My impression is that we agree on the shortcomings of the old (/current) implementation. Still, I sense a lot of resistance to just fixing these problems, and I don’t understand why. Instead we have workarounds now, and I’m happy these worked for us, but the end result is a situation that is still not very intuitive nor maintainable. Your reasons for not accepting the more structural solution I offered earlier are not clear to me. You said previously it’s about “structure and pattern”, but this is exactly the area that it would improve, in my opinion. I do understand the concern of changes having unintended consequences to existing users, which is why I'd be happy to extend my unit tests with additional verifications.

I have put quite some effort into trying to communicate my experience with using a feature of Camunda (in this case: custom form handlers) from a fresh perspective. Such a perspective can be valuable because as developers we risk becoming so deeply entrenched in our system that we can lose sight of how unintuitive some parts of it may be. When that happens feedback from someone else could be used to ultimately everyone's benefit. At this point, I’m not sure how to proceed with the next issue I find (in fact I’ve already found a fairly obscure one). Should I report it and try to fix it, or just look for a workaround?

What is your perspective on this? Is there anything I should do differently next time?

Best regards, Marcus

Hello Marcus,

We are sorry for any frustration the handling of your contribution may have caused. When we make changes in the product, we often face the decision between a small fix and a larger fix that may introduce more structure. Usually we make these decisions very carefully, taking into account factors such as backwards compatiblity of our API, behavioral compatibility of non-API extension points, complexity of implementation, benefits of introducing new concepts, etc.

In the case we have discussed here, we are talking about extension points of the process engine that are not part of the public API. That means, these hooking points are not guaranteed to remain backwards compatible, are not as extensively tested, and we do not guarantee a particular, documented behavior. In this light, all solutions discussed in this ticket are generally valid. In order to decide for the best solution, we must take the factors mentioned above into account.

In the case of BPMN parse listeners, the principle we want to maintain is that the parse listener has the "last say" on a parsed element. That means, in the first step the Camunda parser does its thing and then a parse listener can modify the parsed element. The Camunda parser should then no longer overwrite any of that.

This was previously not the case for start events, but is now thanks to your pull request https://github.com/camunda/camunda-bpm-platform/pull/496. However, we decided against the bigger implementation provided in https://github.com/camunda/camunda-bpm-platform/pull/444, because it somewhat violates this principle. With the proposed change of PR 444, at the time of invoking the parse listener, the form handler would not yet be fully initialized (since #parseConfiguration is not yet called).

We see two use cases for parse listeners here:

1. The use case discussed in this ticket: Replacing the form handler and then have the #parseConfiguration method be called by the parser.
2. A parse listener that wants to modify a form handler defined in a BPMN model.

In our opinion, these two use cases collide. Use case 2 requires that the form handler is already fully initialized at the time when the parse listener is invoked, if the parse listener depends on the configuration in the BPMN. In consequence, we preferred to keep the existing behavior to avoid breaking such use cases and to stick with the above-mentioned principle.

All that said, we agree that form handling, as well as the parse listener and the parser itself have room for improvement. They are software components that have grown over the years, and being non-API code, have had not such a strong conceptual foundation as other parts of our software have. We are open for suggestions and want to encourage you to raise them. Even if we do not immediately go for a bigger change, it is important for us to be aware of your use cases. For example, if we gather evidence that this is relevant for more people, we would be more confident in making bigger changes, or could even consider creating an API feature with a more solid design and the corresponding guarantees.

I hope our position is understandable. Please let us know if you have further questions or remarks. Once more, we are sorry for any disappointments you have experienced.

Best regards,
Yana & Thorben

Thank you Thorben and Yana for your comprehensive and clear explanation! I can see now that your decision was indeed careful and well thought-out.

You are right. I had not fully considered the 2nd use case. For both use cases to work, it seems to me that the form handler must be initialised (through #parseConfiguration) _before _the parse listeners are invoked (as is done currently) and then once more when it is detected that the parse listener has replaced it, which would become rather complicated.

I also had not realised this functionality was not considered part of the public API.

Best regards, Marcus

Hi Marcus,

It's great that we were able to clarify everything.
Hereby I am closing the ticket.

Best regards,
Yana