Allow disabling forms to prevent new submissions

Here we go. I also added a test.

1. +++ b/contact_storage.module
   @@ -155,6 +156,16 @@ function contact_storage_entity_operation_alter(array &$operations, EntityInterf
   + */
   +function contact_storage_entity_type_build(array &$entity_types) {
   +  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
   +  $entity_types['contact_form']
   +    ->setLinkTemplate('enable', '/admin/structure/contact/view/{contact_form}/enable')
   +    ->setLinkTemplate('disable', '/admin/structure/contact/view/{contact_form}/disable');
   +}
   
   @@ -193,6 +204,27 @@ function contact_storage_entity_type_alter(array &$entity_types) {
   +  $entity_types['contact_form']->setFormClass('disable', 'Drupal\contact_storage\Form\ContactFormDisableForm');
   +  $entity_types['contact_form']->setLinkTemplate('disable-form', '/admin/structure/contact/manage/{contact_form}/disable');
   +  $entity_types['contact_form']->setFormClass('enable', 'Drupal\contact_storage\Form\ContactFormEnableForm');
   +  $entity_types['contact_form']->setLinkTemplate('enable-form', '/admin/structure/contact/manage/{contact_form}/enable');
   

   You have set the link templates twice now. entity_type_build() should not be needed.

2. +++ b/contact_storage.module
   @@ -193,6 +204,27 @@ function contact_storage_entity_type_alter(array &$entity_types) {
   +function contact_storage_contact_form_access(\Drupal\Core\Entity\EntityInterface $entity, $operation, \Drupal\Core\Session\AccountInterface $account) {
   +  if (!$entity->status() && ($operation == 'view' || $operation == 'update')) {
   +    // Forbids view and update access, but leaves the option to delete a
   +    // disabled contact form and to re-enable it ('enable' operation).
   +    return AccessResult::forbidden();
   +  }
   

   not 100% sure about what we allow and don't allow yet. Possibly we want to only disallow view.

3. +++ b/src/Tests/ContactStorageTest.php
   @@ -167,6 +167,18 @@ class ContactStorageTest extends ContactStorageTestBase {
   +    // Disable the form and verify that it can't be accessed.
   +    $this->drupalPostForm('/admin/structure/contact/manage/test_id_2/disable', NULL, t('Disable'));
   

   Clicking them is always a bit tricky in the last, but can we check that we see the expected enable/disable links as many times as we expect them on the overview page?

4. +++ b/src/Tests/ContactStorageTest.php
   @@ -167,6 +167,18 @@ class ContactStorageTest extends ContactStorageTestBase {
   +    $this->assertText('Access denied');
   

   Lets add a assertResponse(403) / 200 as well.

There, should be good. I'm now allowing update rights on disabled forms (why not, indeed).

Additionally to #11 we have following in contact_storage_entity_type_alter():

  // @todo Replace with access control handler when not enough.
  $entity_types['contact_message']->set('admin_permission', 'administer contact forms');

Introduction of contact_storage_contact_form_access() makes module less flexible cos there's no way to override this access control...

So I put this back to NR to at least implement access handler and continue discussion about meaning of "disabled vs prevent submissions"

My big +1 to introduce another third party setting to "close submissions"

+++ b/contact_storage.module
@@ -193,6 +194,27 @@ function contact_storage_entity_type_alter(array &$entity_types) {
+function contact_storage_contact_form_access(\Drupal\Core\Entity\EntityInterface $entity, $operation, \Drupal\Core\Session\AccountInterface $account) {
+  if (!$entity->status() && $operation == 'view') {
+    // Forbids view access to the contact form.
+    return AccessResult::forbidden();

+++ b/contact_storage.routing.yml
@@ -0,0 +1,15 @@
+    _entity_access: 'contact_form.disable'
...
+    _entity_access: 'contact_form.enable'

Suppose it's time to implement access control handler!

NW to get rid of contact_storage_contact_form_access()

A forbidden can't be overridden, even less so in an access control handler. I don't see a reason to do that.

I thought about the message, but that would be quite a bit more complicated. We need to alter the route to be able to do that. That could conflict with custom code already doing that.

Here are some small fixes.

In general I'm a bit confused how the operations ever worked.

Looks great for me!

I still think we could do without our own access class, but works for me.

Attaching a re-rolled patch against latest 8.x-1.x as the patch from #15 failed to apply because of recent commits.

@zerolab, thanks for the patch. Tests are failing, though, so I re-rolled #15 and made the cssSelect change recommended in #10.

Cheers!
Just looked at my patch, it looks like I failed to git add . and it was missing the ContactFormDisableForm and ContactFormEnableForm classes :-(

The tests are back to green and since it is a re-roll, RTBC?

I think that should be configurable

Here we go with a configurable message instead of denying access.

1. +++ b/contact_storage.module
   @@ -39,6 +41,12 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
   +  $form['contact_storage_disabled_form_message'] = [
   +    '#type' => 'textfield',
   +    '#title' => t('Disabled contact form message'),
   +    '#description' => t("Message to display if the contact form is disabled."),
   +    '#default_value' => $contact_form->getThirdPartySetting('contact_storage', 'disabled_form_message', 'This contact form has been disabled.'),
   +  ];
      $form['contact_storage_preview'] = [
   

   I think above, @larowlan had the idea to (additionally?) show this on the disable confirm form. I think that would be quite nice, as you are reminded of the message when disabling the form.

2. +++ b/contact_storage.module
   @@ -136,6 +145,30 @@ function contact_storage_entity_base_field_info_alter(&$fields, EntityTypeInterf
    /**
   + * Implements hook_entity_operation().
   + */
   +
   +function contact_storage_entity_operation(EntityInterface $entity) {
   +  $operations = [];
   +  /** @var \Drupal\contact\ContactFormInterface $entity */
   

   I thought those shouldn't be needed as we get them by default?

3. +++ b/src/ContactFormViewBuilder.php
   @@ -89,16 +89,30 @@ class ContactFormViewBuilder implements EntityViewBuilderInterface, EntityHandle
   +      $form['disabled_form_error'] = array(
   +        '#type' => 'container',
   +        '#markup' => t($entity->getThirdPartySetting('contact_storage', 'disabled_form_message', 'This contact form has been disabled.')),
   +        '#attributes' => array(
   +          'class' => array('messages', 'messages--error'),
   +        ),
   +        '#weight' => -100,
   +      );
   

   This is a frontend/user facing output. We might want to give users more flexibility to configure this. Probably create a template and pass in the contact form and the message as variables?

   The default output can still be the same.

   weight is not needed I think as nothing else is on that page.

4. +++ b/src/ContactFormViewBuilder.php
   @@ -89,16 +89,30 @@ class ContactFormViewBuilder implements EntityViewBuilderInterface, EntityHandle
   +    $this->renderer->addCacheableDependency($form, $entity);
   

   a comment for this would be good. Something like // Add required cacheability metadata from the contact form entity, so that changing it invalidates the cache.

5. +++ b/src/Controller/ContactStorageController.php
   @@ -0,0 +1,42 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function contactSitePage(ContactFormInterface $contact_form = NULL) {
   +    $config = $this->config('contact.settings');
   

   I think a comment here that explains why we override this would be good. Something like "This is an override of ContactController::contactSitePage() that uses the entity view builder. This is necessary to show the close message for disabled forms.

I made those changes, including adding a template for the disabled form message and adding the disabled message textfield to the form to disable the forms..

I thought those shouldn't be needed as we get them by default?

Nope. the links won't show without this.

Getting close I think, good work.

Lets check those operations tomorrow, please remind me when you work on this.

1. +++ b/contact_storage.module
   @@ -309,6 +308,12 @@ function contact_storage_theme() {
   +      'template' => 'contact_storage_disabled_form',
   

   templates usually use -, not _. Then you don't need to specify it explicitly.

2. +++ b/contact_storage.module
   @@ -324,3 +329,16 @@ function template_preprocess_swiftmailer__contact(&$variables) {
   +function template_preprocess_contact_storage_disabled_form(&$variables) {
   +  $contact_form = $variables['contact_form'];
   +  $variables['redirect_uri'] = $contact_form->getThirdPartySetting('contact_storage', 'redirect_uri', '');
   +  $variables['disabled_form_message'] = $contact_form->getThirdPartySetting('contact_storage', 'disabled_form_message', 'This contact form has been disabled.');
   

   I think it is easier to just provide both variables directly in the render array. You don't need a detached preprocess function then. Sometimes, those are useful (when using render element, for example), but for such simple things, just passing things in directly is easier to follow.

3. +++ b/src/Form/ContactFormDisableForm.php
   @@ -42,7 +42,23 @@ class ContactFormDisableForm extends EntityConfirmFormBase {
   +      '#default_value' => $this->getEntity()->getThirdPartySetting('contact_storage', 'disabled_form_message', 'This contact form has been disabled.'),
   

   I think the fallback should use t(), also in the other place. So when your site is spanish, you get a spanish message by default.

4. +++ b/templates/contact_storage_disabled_form.html.twig
   @@ -0,0 +1,32 @@
   +<div id="block-bartik-content" class="block block-system block-system-main-block">
   +  <div class="content">
   +    <div class="messages messages--error">
   +        {{ disabled_form_message }}
   +    </div>
   +  </div>
   +</div>
   

   Just the inner div is enough I think, the two outer ones are actually provided by the wrapper already, and you'd end up with them duplicated.

Here's for now, still with contact_storage_entity_operation().

Here we go !

Thanks for the review @larowlan ! Here are the changes.