Create contact form block

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Think this is ready to go. I worked on the tests #127 but not the solution.

Question for the committer is if this will need a change record.

At this point, I think this is a feature request, and the presence of 'Create' in the issue title reinforces that.

I think its a good addition, but there's a bit to go still before I think its ready

1. +++ b/core/modules/contact/config/schema/contact.schema.yml
   @@ -49,3 +49,11 @@ contact.settings:
   +      type: mapping
   +      label: 'Form'
   

   for a mapping the keys must be known, so should this instead be a sequence where the keys are the contact forms and the values are the weights?

2. +++ b/core/modules/contact/contact.module
   @@ -37,6 +37,16 @@ function contact_help($route_name, RouteMatchInterface $route_match) {
   +      $output = '<p>' . t('Create separate contact forms for different purposes.') . '</p>';
   +      $output .= '<p>' . t('If you would like additional text to appear on the site-wide contact page, use a block. You can create and edit blocks on the <a href="@blocks">Blocks administration page</a>.', ['@blocks' => Url::fromRoute('block.admin_display')->toString()]) . '</p>';
   

   The remaining tasks says 'UX review' has that occurred for these new strings?

3. +++ b/core/modules/contact/contact.module
   @@ -37,6 +37,16 @@ function contact_help($route_name, RouteMatchInterface $route_match) {
   +      if (\Drupal::moduleHandler()->moduleExists('block')) {
   +        $output .= '<p>' . t('Each form gets a link in the Contact forms block. You can enable this block on the <a href="@block">Block layout</a> page.', [
   

   i'm not sure why we check for the block module here when outside the if we already link to the block display page? Shouldn't both links be inside the if?

4. +++ b/core/modules/contact/src/ContactFormListBuilder.php
   @@ -18,7 +18,7 @@ class ContactFormListBuilder extends ConfigEntityListBuilder {
   -    $header['selected'] = t('Selected');
   +    $header['default'] = t('Default');
        return $header + parent::buildHeader();
      }
    
   @@ -30,7 +30,7 @@ public function buildRow(EntityInterface $entity) {
   
   @@ -30,7 +30,7 @@ public function buildRow(EntityInterface $entity) {
        if ($entity->id() == 'personal') {
          $row['form'] = $entity->label();
          $row['recipients'] = t('Selected user');
   -      $row['selected'] = t('No');
   +      $row['default'] = t('No');
        }
        else {
          $row['form'] = $entity->toLink(NULL, 'canonical')->toString();
   @@ -40,7 +40,7 @@ public function buildRow(EntityInterface $entity) {
   
   @@ -40,7 +40,7 @@ public function buildRow(EntityInterface $entity) {
            '#context' => ['list_style' => 'comma-list'],
          ];
          $default_form = \Drupal::config('contact.settings')->get('default_form');
   -      $row['selected'] = ($default_form == $entity->id() ? t('Yes') : t('No'));
   +      $row['default'] = ($default_form == $entity->id() ? t('Yes') : t('No'));
   

   these changes feel out of scope.

5. +++ b/core/modules/contact/src/Plugin/Block/ContactNavigationBlock.php
   @@ -0,0 +1,235 @@
   +    return ['cache_context.user.roles'];
   

   shouldn't this just be user.permissions?
   also I don't think the cache context is cache_context.user.roles, it should be just user.roles, so that indicates missing test coverage I think

6. +++ b/core/modules/contact/src/Plugin/Block/ContactNavigationBlock.php
   @@ -0,0 +1,235 @@
   +      '#empty' => $this->t('There is no contact forms yet. Create one.'),
   

   Should this be 'there are no contact forms yet'?

   Should the 'create one' text link somewhere for users with appropriate permissions?

7. +++ b/core/modules/contact/src/Plugin/Block/ContactNavigationBlock.php
   @@ -0,0 +1,235 @@
   +      if ($id == 'personal') {
   

   nit ===

8. +++ b/core/modules/contact/src/Plugin/Block/ContactNavigationBlock.php
   @@ -0,0 +1,235 @@
   +      $form_state->setErrorByName('forms', $this->t('At least one category should be selected.'));
   

   We don't seem to have tests for this

9. +++ b/core/modules/contact/src/Plugin/Block/ContactNavigationBlock.php
   @@ -0,0 +1,235 @@
   +      $links[$id] = $contact_form->toLink($contact_form->label(), 'canonical', [
   +        'set_active_class' => TRUE,
   +      ]);
   

   We can make use of named arguments here and avoid the first two arguments which are just the default

10. +++ b/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
    @@ -53,6 +53,8 @@ protected function setUp(): void {
    +    // cspell:ignore contactforms
    +    $this->drupalPlaceBlock('contact_navigation', ['id' => 'contactforms']);
    

    why not use something that doesn't trigger cspell instead of adding the ignore? `contact_forms` would probably do?

11. +++ b/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
    @@ -220,6 +223,21 @@ public function testSiteWideContact() {
    +    $this->assertSession()->statusCodeEquals(200);
    +    $edit = [
    +      'settings[forms][' . $name . '][display]' => TRUE,
    +    ];
    +    $this->submitForm($edit, 'Save block');
    +
    +    // Test that the contact form block appears.
    +    $this->drupalGet('contact');
    +    // cspell:ignore contactforms
    

    there's nothing here that is testing the quite involved logic around ordering of disabled items at the bottom etc that's present in the block form, so I think we should add tests, or simplify that logic

Thanks for working on this again, let's keep up the momentum!

Also we're missing calculateDependencies and onDependencyRemoval here

i.e. the block should depend on the contact form, and should remove it from the selected options when the contact form is deleted

And yes to a change record to trumpet the new addition 📣