Problem/Motivation

If our goal is to segregate our interfaces by workflow types that are configurable and workflows that are not, I think it makes sense to move the methods for transition/state forms to WorkflowTypeFormInterface/WorkflowTypeFormBase.

That leaves implementations of workflow types that are simple and don't require the collection of extra information for states/transitions/as a whole light.

Proposed resolution

Move these methods.

Remaining tasks

Discuss and agree on an approach.

User interface changes

API changes

Data model changes

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
FileSize
6.37 KB

Lets see what tests break.

Status: Needs review » Needs work

The last submitted patch, 2: 2877926-move-methods-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: +Workflow Initiative
FileSize
1.14 KB
7.53 KB

Not sure I fully understand why we need this level of abstraction, but fixing the test anyway.

@Sam152 - I'll ping you on IRC so you can convince me.

timmillwood’s picture

FileSize
5.79 KB
11.63 KB

Here's a better fix for the issue, thus allowing TestType workflow plugin to stay as only extending WorkflowTypeBase and not WorkflowTypeFormBase. Then use ComplexTestType to extend WorkflowTypeFormBase and test that functionality.

Sam152’s picture

+++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
@@ -43,10 +44,13 @@ public function form(array $form, FormStateInterface $form_state) {
+    if ($workflow->getTypePlugin() instanceof WorkflowTypeFormInterface) {
+      $form['type_settings'] = [
+        $workflow->get('type') => $workflow->getTypePlugin()

+++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
@@ -62,10 +63,13 @@ public function form(array $form, FormStateInterface $form_state) {
+    if ($workflow->getTypePlugin() instanceof WorkflowTypeFormInterface) {
+      $form['type_settings'] = [

+++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
@@ -62,10 +63,13 @@ public function form(array $form, FormStateInterface $form_state) {
+    if ($workflow->getTypePlugin() instanceof WorkflowTypeFormInterface) {
+      $form['type_settings'] = [
+        $workflow->get('type') => $workflow->getTypePlugin()

+++ b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
@@ -78,10 +79,13 @@ public function form(array $form, FormStateInterface $form_state) {
+    if ($workflow->getTypePlugin() instanceof WorkflowTypeFormInterface) {
+      $form['type_settings'] = [
+        $workflow->get('type') => $workflow->getTypePlugin()

Maybe we should extract getTypePlugin into a variable in all the cases here.

Otherwise, I like it!

timmillwood’s picture

FileSize
3.64 KB
11.82 KB

Deal!

Here's a $workflow_type_plugin for you.

Sam152’s picture

+1 RTBC, but probably should have someone else review this based on #2.

Sam152’s picture

Minor OCD, we already have 7 instances of $workflow_type, none of $workflow_type_plugin + updating the formatting to be on one line.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

In #8 @Sam152 RTBC'd my code, I RTBC his code, so we're all good?

larowlan’s picture

  1. +++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
    @@ -43,10 +44,13 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type instanceof WorkflowTypeFormInterface) {
    
    +++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
    @@ -62,10 +63,13 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type instanceof WorkflowTypeFormInterface) {
    
    +++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
    @@ -62,10 +63,13 @@ public function form(array $form, FormStateInterface $form_state) {
    +    if ($workflow_type instanceof WorkflowTypeFormInterface) {
    

    this breaks duck-typing - instanceof always feels like an anti-pattern to me - can you elaborate more as to why we want to do this?

  2. +++ b/core/modules/workflows/tests/modules/workflow_type_test/src/Plugin/WorkflowType/TestType.php
    @@ -2,6 +2,7 @@
    +use Drupal\Core\Form\FormStateInterface;
    

    this looks unused?

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Updating status

Sam152’s picture

Re: #11.2, the idea here is not forcing plugins to implement WorkflowTypeFormInterface, with empty implementations where forms aren't required. We're already checking the type of plugin the workflow type is to ensure \Drupal\Core\Plugin\PluginFormInterface::buildConfigurationForm is optional, this issue is just giving state and transition forms the same treatment.

A similar concept exists with ImageEffectBase and ConfigurableImageEffectBase. I guess the trade off here is, do we want plugins implementing methods that are potentially irrelevant or segregate the interfaces accordingly and check those types where relevant.

Sam152’s picture

Status: Needs review » Needs work

Discussed this with @larowlan. It was suggested to add a method on the base called hasForms or something like that. Having thought about this, I'm in two minds about this.

One one hand it makes the code a bit cleaner, instanceof checks are generally associated with anti-patterns so it's nice to move that decision somewhere else. On the other hand, it doesn't actually enforce at a language level that the set of methods we need from the interface are actually going to be implemented. A plugin implementation could technically return TRUE from the method, while not implementing the interface. Maybe a sign that a convention over a language feature is a smell. It also adds a method to the "simple" workflow implementation that doesn't really make sense in isolation. Why does a "hasForms" method exist in a context where I never expected a form in the first place?

Maybe a final method on the base which checks static::class implements the right interface would alleviate the first issue for me, but it's a coin toss for me. I probably lean towards simply using instanceof checks where appropriate and deferring to the pattern we already have.

NW for consensus.

timmillwood’s picture

I'm a big instanceof fan, and didn't realise it was anti-pattern. I'd much rather than do that than add a method, which basically does the same.

I still have no issue with #9 (apart from the unused use).

timmillwood’s picture

Status: Needs work » Needs review

Moving this back to Needs Review, because I think review is what is needed base in #14, not work.