Many PHP notice on mass unsubscribe

I have the exact same errors but with different line numbers as seen below

    Notice: Undefined offset: 42 in simplenews_subscription_list_remove_submit() (line 1027 of /var/www/deploy/drupal-webroot/sites/all/modules/contrib/simplenews/includes/simplenews.admin.inc).
    Notice: Trying to get property of non-object in _simplenews_newsletter_name() (line 2114 of /var/www/deploy/drupal-webroot/sites/all/modules/contrib/simplenews/simplenews.module).
    Notice: Undefined offset: 44 in simplenews_subscription_list_remove_submit() (line 1027 of /var/www/deploy/drupal-webroot/sites/all/modules/contrib/simplenews/includes/simplenews.admin.inc).
    Notice: Trying to get property of non-object in _simplenews_newsletter_name() (line 2114 of /var/www/deploy/drupal-webroot/sites/all/modules/contrib/simplenews/simplenews.module).
    Notice: Undefined offset: 45 in simplenews_subscription_list_remove_submit() (line 1027 of /var/www/deploy/drupal-webroot/sites/all/modules/contrib/simplenews/includes/simplenews.admin.inc).
    Notice: Trying to get property of non-object in _simplenews_newsletter_name() (line 2114 of /var/www/deploy/drupal-webroot/sites/all/modules/contrib/simplenews/simplenews.module).

I hope I will get to investigating this bug and if a solution comes up then submit a patch too, but right now I am chasing deadlines and have a HUGE mass unsubscription to do. I am doing the SQL query route, pulling out the `snids` from the emails and then deleting their records from `simplenews_subscription` table for now.

My setup details that might help find the bug below.

$ drush pmi simplenews
 Project          :  simplenews                                                                  
 Type             :  module                                                                      
 Title            :  Simplenews                                                                  
 Description      :  Send newsletters to subscribed email addresses.                             
 Version          :  7.x-1.0                                                                     
 Package          :  Mail                                                                        
 Core             :  7.x                                                                         
 Status           :  enabled                                                                     
 Path             :  sites/all/modules/contrib/simplenews                                        
 Schema version   :  7010     

Don't see how this would be possible, that sound like you have categories that are displayed and selected for unsubscribe but then don't exist anymore.

Please check your newsletters and their id's, do they match the id's in those error messages? Do you have terms in the simplenews vocabulary that aren't listed as a newsletter?

I agree with the original post. The line numbers may change, perhaps due to different revisions of code. I believe the problem is that when you attempt to "mass un-subscribe" an email address from a list, to which the email is not currently subscribed, you get this out of range index error.
I commonly check ALL the lists when I am un-subscribing someone. I wish to remove them from all my lists, not just the ones I know they belong to. So, if the email is not already subscribed to a specific list, then shouldn't get an error, just skip the item and move on.

The call to simplenews_categories_load_multiple() is supposed to only cache when called to get all categories, but it's really not. Here is a patch to fix that. But in reality, I think the cache should be removed from that function.

The patch in #4 works for me. Thanks.

I agree that the patch makes sense.

If simplenews_categories_load_multiple() currently receives $tids or $conditions, ... then the static categories cache is overwritten. The next call of the function without condition results in lack of all other $tids.

However, we need test coverage for this ugly bug.
Test location to adapt/extend (and reproduce the bug) is simplenews.test line 1446+.

Would it be better if we cache and retrieve from cache in every case?
I was thinking that I can serialize() or json_encode() the query parameters, and use them as cache key.

I doubt it's worth doing this as it's only a static cache, not a persistent one.
This is not a common pattern in Drupal yet.

It depends the loop for mass imports. (I didn't check the exact code calls. That's why i ask for a test. It's very easy then to output the function calls with debug().) If it currently really triggers N times an uncached lookup we could investigate if it's worth getting rid of this query.

Here is a patch to test that the 'categories_load_fix_cache-2002584-4.patch' (Comment #4) works fine and solves the issues on mass unsubscribe.

Please set it to "Needs review" when submitting a patch so the testbots will test it.
And regarding type, it's still a bug report, even if it will be fixed. :-)

Here is the patch I've created before the one I uploaded in the comment #9. With this patch, local tests passes without any issues. I will review the earlier patch and fix the bugs.

Here is the patch I've created before the one I uploaded in the comment #9. With this patch, local tests passes without any issues. I will review the earlier patch and fix the bugs.

So the patch now proves that it fails. Now we need a combined patch with the test and the fix that should then pass.

(Mostly) coding style review below:

1. +++ b/tests/simplenews.test
   @@ -2963,3 +2963,143 @@ class SimplenewsUnitTest extends DrupalUnitTestCase {
   +/**
   + * Tests the functionality of the Simplenews mass unsubscribe functionality.
   + */
   +class SimplenewsMassUnsubscribeTestCase extends SimplenewsTestCase
   +{
   +
   +    /**
   +     * Implement getInfo().
   +     */
   +    static function getInfo()
   +    {
   

   Make sure to check with the drupal coding standards. opening "{" for classes and methods should be on the same line as the class/method.

2. +++ b/tests/simplenews.test
   @@ -2963,3 +2963,143 @@ class SimplenewsUnitTest extends DrupalUnitTestCase {
   +            'name' => t('Test mass unSubscribe'),
   +            'description' => t('Mass unsubscription of anonymous users (for verifying patch categories_load_fix_cache-2002584-4)'),
   +            'group' => t('Simplenews'),
   

   It is no longer necessary to use t() here, just provide the string directly. Also no need to specify the patch.

3. +++ b/tests/simplenews.test
   @@ -2963,3 +2963,143 @@ class SimplenewsUnitTest extends DrupalUnitTestCase {
   +     */
   +    public function setUp()
   +    {
   +        parent::setUp(array('block'));
   +        // Include simplenews.subscription.inc for simplenews_mask_mail().
   +        module_load_include('inc', 'simplenews', 'includes/simplenews.subscription');
   +    }
   

   I assume you copied this because this seems unnecessary, we are not using that function or using the block module.

4. +++ b/tests/simplenews.test
   @@ -2963,3 +2963,143 @@ class SimplenewsUnitTest extends DrupalUnitTestCase {
   +        //Generates 5 new newsletters
   

   Comments should always have an empty space after the // and end with a ".".

5. +++ b/tests/simplenews.test
   @@ -2963,3 +2963,143 @@ class SimplenewsUnitTest extends DrupalUnitTestCase {
   +        //This generates 10 new random emails, basically subscribers
   +        for ($i = 0; $i < 10; $i++) {
   

   There is a lot of code here to replicate the strange behavior of the confirmation message, which repeats the e-mails in a weird way.

   It might be easier to fix the code that prints the e-mails to make them unique (with an array_unique() or so) and then we can simplify the logic here to a single line more or less (create array of e-mails, then implode() into a string.

6. +++ b/tests/simplenews.test
   @@ -2963,3 +2963,143 @@ class SimplenewsUnitTest extends DrupalUnitTestCase {
   +        $this->drupalPost('admin/people/simplenews/unsubscribe', $edit, t('Unsubscribe'));
   +        $this->assertText(t('The following addresses were unsubscribed: @emails.', array('@emails' => $mailVerbose)), t('Subscription successful'));
   +
   +        // Verify subscription changes
   +        drupal_static_reset('simplenews_user_is_subscribed');
   +        foreach ($categories as $tid => $category) {
   +            foreach ($mailList as $mail) {
   +                $is_subscribed = simplenews_user_is_subscribed($mail, $tid);
   +                if (in_array($tid, $enable)) {
   +                    $this->assertFalse($is_subscribed);
   +                }
   +            }
   +        }
   +
   +        $this->drupalLogout();
   

   It looks like you have this code twice here?

   The drupalLogout() is not necessary, because the test is executed in a isolated enviroment that is completely deleted when you are done. You only need logout when switching the user in the same test.

Here is the patch including fix for the mass unsubscribe (comment #4) and the test for testing it(Comment #12).

Reverting issue summary.