(dev/mail/8) Using ACL to restrict mailing recipients leads to fatal error

[monishdeb]

Overview

Steps to replicate:

1. Assign an ACL permission for one or more contacts
2. Compose a mailing and select a recipient group. Ensure that those contacts are included in that group
3. Submit and Send Mailing immediately

Issue link - https://lab.civicrm.org/dev/mail/issues/8

Before

Result into fatal error - https://pastebin.com/aMa2KYy0

After

Fixed the error.

[seamuslee001]

@monishdeb test failures look related

[monishdeb]

Strange after fixing the test failures, it passes on local

Monishs-MacBook-Pro:civicrm monish$ phpunit4 tests/phpunit/CRM/Mailing/BAO/MailingTest.php 
PHPUnit 4.8.21 by Sebastian Bergmann and contributors.

.
Installing civicrm47_drupal_test database
..

Time: 19 seconds, Memory: 67.25Mb

OK (3 tests, 90 assertions)

But fails in Jenkin :/

[monishdeb]

Jenkins test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@seamuslee001 remaining fail didn't seem related - are you up with this enough to review. Seems like it needs to be merged to this rc

NB @seamuslee001 @totten a case could be made to include it in a 5.0.1 drop. I'm agnostic - I don't think I would argue for a 5.0.1 drop specifically to include this as it was a regression in 4.7.31 rather than 5.0.0 - but since there is now a PR open for a 5.0.0 specific regression (#11975) & if this is very clear cut then I think it could go in a 5.0.1 drop on the grounds it is dealing with a critical error & it is fairly recent (only one more release ago)

[seamuslee001]

@eileenmcnaughton last time i checked the fail was related directly to Monish's test but we will see what this run does. The change looks right to me but isolated testing as the tests passing but doing something like ./tools/scripts/phpunit 'CRM_AllTests' causes it to fail why? flips tables

[monishdeb]

@seamuslee001 earlier my added UT was failing because the contacts created in it was affecting other UTs in that test-case. Now when I added the data cleanup at the end of it, this should pass now. Lets see!

[monishdeb]

This is weird, now the test failure reappear again and changed nothing :/ https://test.civicrm.org/job/CiviCRM-Core-PR/20117/

If I trigger the test build then it might show different failure

[monishdeb]

Jenkins test this please

[seamuslee001]

@monishdeb coming up with same incorrect result https://test.civicrm.org/job/CiviCRM-Core-PR/20121/testReport/junit/(root)/CRM_Mailing_BAO_MailingTest/testgetRecipientsUsingACL/ i did some partial debugging on the weekend and was finding that even doing a SELECT * FROM civicrm_mailing_recipients was returning nothing

[eileenmcnaughton]

@monishdeb I found it!! Comments in this code turn out to be prescient - switch to Civi::static caching in the function below

  /**
   * Fill the acl contact cache for this contact id if empty.
   *
   * @param int $userID
   * @param int|string $type the type of operation (view|edit)
   * @param bool $force
   *   Should we force a recompute.
   */
  public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force = FALSE) {
    // FIXME: maybe find a better way of keeping track of this. @eileen pointed out
    //   that somebody might flush the cache away from under our feet,
    //   but the alternative would be a SQL call every time this is called,
    //   and a complete rebuild if the result was an empty set...
    $_processed = array(
      CRM_Core_Permission::VIEW => array(),
      CRM_Core_Permission::EDIT => array());

[eileenmcnaughton]

@seamuslee001 back to you to confirm since I think you have a handle on this (5.1) change

[monishdeb]

I was not making any noise until what Jenkins has to say this time 😉

[eileenmcnaughton]

:-)

[monishdeb]

At least this is a different failure this time https://test.civicrm.org/job/CiviCRM-Core-PR/20220/

[monishdeb]

Jenkins test this please

[seamuslee001]

Permission changes look fine to me and the change in the mailing is correct IMO

[seamuslee001]

(CiviCRM Review Template WORD-1.1)

• (r-explain) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run)Pass i believe others have reported this fixes things for them.
• (r-user) Pass
• (r-tech) Pass

[seamuslee001]

merging

[monishdeb]

Thanks @eileenmcnaughton @seamuslee001