Skip to content
Snippets Groups Projects

Resolve #3422872 "Contact settings"

Closed Resolve #3422872 "Contact settings"
issue/drupal-3422872:3422872-contact-settings into 11.x
Closed Wim Leers requested to merge issue/drupal-3422872:3422872-contact-settings into 11.x

Closes #3422872

Merge request reports

Merged results pipeline #144309 passed with warnings

Stage: 🏗️ Build
Stage: 🪄 Lint
Stage: 🗜️ Test
Pipeline: drupal

#144315

    Merged results pipeline passed with warnings for 661a0173

    Closed by Lee RowlandsLee Rowlands 1 year ago (Apr 11, 2024 9:53pm UTC)

    Loading

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    core/lib/Drupal/Core/Entity/EntityStorageBase.php
    269 269 public function loadMultiple(array $ids = NULL) {
    270 270 $entities = [];
    271 271 $preloaded_entities = [];
    272 if ($ids) {
    273 foreach ($ids as $key => $id) {
    274 if ($id === NULL) {
    275 unset($ids[$key]);
    276 }
    277 }
    278 }
    • Comment on lines +272 to +278
      Wim Leers
      Wim Leers @wimleers ·
      Author Developer

      :thinking: This was introduced in the currently last commit (!6713 (9151cc0a)). This very much looks like babysitting broken code. The commit message does not justify this change, nor does this code.

      If those tests were failing, that very strongly suggests those tests are broken, because they must contain something like `loadMultiple(['foo', NULL, 'bar']), which makes no sense :sweat_smile:

    • Kunal Sachdev
      Kunal Sachdev @kunal.sachdev ·

      Actually the tests are testing

      // Test contact form with no default form selected.
    $this->config('contact.settings')
      ->set('default_form', '')
      ->save();
    $this->drupalGet('contact');
    $this->assertSession()->statusCodeEquals(404);

      and I changed

      $this->config('contact.settings')
      ->set('default_form', '')

      to

      $this->config('contact.settings')
      ->set('default_form', NULL)

      in this test as we now can't set it to empty string.

      Edited by Kunal Sachdev
    • Kunal Sachdev
      Kunal Sachdev @kunal.sachdev ·

      @wimleers If it doesn't make sense to to test if for NULL then I should remove the code to test the contact form with no default form selected, right?

      Edited by Kunal Sachdev
    • Wim Leers
      Wim Leers @wimleers ·
      Author Developer

      The problem is not that contact.setting:default_form can now be NULL. The problem is that whichever code is using that is blindly using that and assuming there's always a valid ID in there — that was never the case! It's just that switching from '' to NULL causes a failure.

      IOW: the same problem existed, the symptom is just different.

      Hence this MR should update (fix, really!) \Drupal\contact\Controller\ContactController::contactSitePage() to not do this anymore:

            $contact_form = $this->entityTypeManager()
        ->getStorage('contact_form')
        ->load($config->get('default_form'));
      // If there are no forms, do not display the form.
      if (empty($contact_form)) {
    • Kunal Sachdev
      Kunal Sachdev @kunal.sachdev ·

      I have changed it already in !6713 (c83ccd79) to

      $contact_form = $this->entityTypeManager()
        ->getStorage('contact_form')
        ->load($config->get('default_form'));
      // If there are no forms, do not display the form.
      if (is_null($contact_form)) {
    • Wim Leers
      Wim Leers @wimleers ·
      Author Developer

      If you already fixed it, then why did you not revert the changes to EntityStorageBase?

      Revert those changes. Then you'll find out that the change you quoted did not fix it: you're still passing NULL to the ::load() method… :grimacing:

    • Kunal Sachdev changed this line in version 25 of the diff

      changed this line in version 25 of the diff

    • Kunal Sachdev
      Kunal Sachdev @kunal.sachdev ·

      Okay, my bad, was thinking in some other direction and mixed things up in my head. I think I have fixed it now.

    • Please register or sign in to reply
  • Wim Leers
  • Wim Leers
  • Wim Leers
  • Wim Leers
  • Wim Leers
  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 20 commits

    added 20 commits

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
    • Please register or sign in to reply