Problem/Motivation

Webforms can have the following statuses:

  /**
   * Webform status open.
   */
  const STATUS_OPEN = 'open';

  /**
   * Webform status closed.
   */
  const STATUS_CLOSED = 'closed';

  /**
   * Webform status scheduled.
   */
  const STATUS_SCHEDULED = 'scheduled';

And this are used as values for:

  /**
   * The webform status.
   *
   * @var bool
   */
  protected $status = WebformInterface::STATUS_OPEN;

This is highly problematic because the API of configuration entities is going to do weird things when calling ->disable() and enable() on webforms

Proposed resolution

Use a new property for this. Eventually we'll move status out of the default configuration entity fields but that's going to take until Drupal 9. See #2872149: All configuration entities can be disabled but many don't actually define what, if anything, that that means for more about status and configuration entities.

Remaining tasks

User interface changes

None

API changes

Probably

Data model changes

Definitely

Comments

alexpott created an issue. See original summary.

jrockowitz’s picture

For the Webform entity I am basically overloading the ::setStatus method to support scheduling. The ::disable and ::enable methods are still supported.

If Drupal core starts allowing configuration status to be controlled via the UI or Views bulk operations, the Webform module could have some serious problems

I am open to creating a new 'status' property. Truth is I am not sure what to call it.

Also, I am using the same type of 'status' property for Webform fields and that would need to be updated as well. Right now, I am still have a hard time writing a stable update hook for altering existing fields. @see #2866957: Updating from beta9 to beta10 fails to beta11 fails

For people,wanting to understand the issue, here is the Webform::setStatus method.

  /**
   * {@inheritdoc}
   */
  public function setStatus($status) {
    if ($status === NULL || $status === WebformInterface::STATUS_SCHEDULED) {
      $this->status = WebformInterface::STATUS_SCHEDULED;
    }
    elseif ($status === WebformInterface::STATUS_OPEN) {
      $this->status = WebformInterface::STATUS_OPEN;
    }
    elseif ($status === WebformInterface::STATUS_CLOSED) {
      $this->status = WebformInterface::STATUS_CLOSED;
    }
    else {
      $this->status = ((bool) $status) ? WebformInterface::STATUS_OPEN : WebformInterface::STATUS_CLOSED;
    }

    // Clear open and close if status is not scheduled.
    if ($this->status !== WebformInterface::STATUS_SCHEDULED) {
      $this->open = NULL;
      $this->close = NULL;
    }

    return $this;
  }

which is overriding the below default implementation.

  /**
   * {@inheritdoc}
   */
  public function setStatus($status) {
    $this->status = (bool) $status;
    return $this;
  }
bucefal91’s picture

If anybody is interested in listening to my 2 cents.. I've read through alexpott's core issue he references. Overall I tend to believe webform's status (at least in its current shape) is a different thing from config entity status and hence should be called differently and use different interface because, as alexpott describes, when somebody/something tries to walk all config entities and do something about their "status", it assumes a certain meaning behind that status (and actually all config entities have agreed to obey that meaning by implementing the ConfigEntityInterface interface). This assumed meaning turns out to be different when it comes to a webform (but the invoking code, the one that walks all config entities) does not know that. So it will become a pain sooner or later.

Alternatively, the "status" in the context of a webform can be simplified down to the level of general "status" meaning for a config entity. I still haven't explored this option in depth, but from a quick study of Webform entity: the status could be turned back into a boolean where TRUE means "open" and FALSE menas "closed". The missing 3rd status "scheduled" can be achieved by checking the value of "open" and "close" properties.

Does anyone have a preference as for which way to go?

jrockowitz’s picture

@bucefal91 I agree with you that status should be reverted back to the default behavior.

This is a major API change that needs to done carefully, especially because the Webform (entity reference) field's status may also need to be updated. Personally, I am procrastinating dealing with this because we have until D9 to fix the problem.

I think we have to fully write out a plan of attack before implementing anything.

jrockowitz’s picture

Priority: Critical » Major

I am changing this issue's priority to major since it should not block tagging a stable release.

jonathanshaw’s picture

Should this be considered for 6.x?

jrockowitz’s picture

Version: 8.x-5.x-dev » 6.x-dev

We could move all the WebformInterface::setStatus and WebformInterface::getStatus code to WebformInterface::setFormStatus to WebformInterface::getFormStatus. I think this change will be be a little confusing but we can make sure to include comments in the API docs.

I should be able to roll a patch this via PHPStorm without any problems.

This is definitely a 6.x change.

jrockowitz’s picture

The concept of changing status for formStatus is a lot more work than I expected. I am not sure if I have time available to address this issue.

jrockowitz’s picture

Status: Active » Postponed

I do want to prematurely start fixing a "none" issue.

I am pretty sure when #2872149: All configuration entities can be disabled but many don't actually define what, if anything, that that means is resolved we will have to make some minor adjustments or workarounds to managing a webform's status. For example, we might be able to keep WebformInterface::setStatus but store the status string in a form_status property.

jrockowitz’s picture

Status: Postponed » Closed (won't fix)

I am going to make this as won't fix until there is actually a reason to fix it.