Importing Banned Emails Stops after Encountering Duplicates

Anomandaris

Anomandaris

Well-known member
Affected version
2.2.13
When importing an XML file of banned emails, when a duplicate is found it just stops the import. Any unique emails following the duplicate are skipped.

XF v2.2.13
/admin.php?banning/emails

"Banned emails must be unique. The specified banned email is already in use."
Click to expand...

1693949858553.png

If a duplicate is found, it should just ignore it and continue importing the rest of the list.

Right now how it works: if an email that is already on the list, is found on line 5, it just stops there and displays the error. The 30,000 lines following that duplicate are not imported. It should just ignore the duplicates and continue to the next line.
 
I totally agree this should be expected functionality. I hope it's considered a bug instead of a feature request. One could argue its intended functionality, but intending to be awkward doesn't make sense. It doesn't even tell you what the banned email is, does it? Not sure if that would log elsewhere.
 
I was trying to solve this problem but I ended up just getting more confused because the code looks correct. Here are some notes, for anyone else trying to fix this problem:

These are the important files related to this:
Code: 
/src/XF/Service/Banning/Emails/Import.php
/src/XF/Service/Banning/AbstractXmlImport.php
/src/XF/Admin/Controller/Banning.php
/src/XF/Entity/BanEmail.php

The phrase error being triggered is: banned_emails_must_be_unique

AbstractXmlImport from Import.php:
PHP: 
    public function import(\SimpleXMLElement $xml)
    {
        $bannedEmailsCache = (array)$this->app->container('bannedEmails');
        $bannedEmailsCache = array_map('strtolower', $bannedEmailsCache);

        $entries = $xml->entry;
        foreach ($entries AS $entry)
        {
            if (in_array(strtolower((string)$entry['banned_email']), $bannedEmailsCache))
            {
                // already exists
                continue;   <---- ding ding ding
            }

            $this->repository('XF:Banning')->banEmail(
                (string)$entry['banned_email'],
                \XF\Util\Xml::processSimpleXmlCdata($entry->reason)
            );
        }
    }

You can see if the duplicate is detected (in_array is successful), it skips that entry with the continue.

The code looks correct here to me, so I don't know why it still errors out after a duplicate is found


xf_ban_email table listing:
1710124774465.webp
the banned_email string is UNIQUE.

BanEmail Structure Code from BanEmail.php
PHP: 
    public static function getStructure(Structure $structure)
    {
        $structure->table = 'xf_ban_email';
        $structure->shortName = 'XF:BanEmail';
        $structure->primaryKey = 'banned_email';
        $structure->columns = [
            'banned_email' => ['type' => self::STR, 'required' => true, 'maxLength' => 120,
                'unique' => 'banned_emails_must_be_unique',
            ],
            'create_user_id' => ['type' => self::UINT, 'required' => true],
            'create_date' => ['type' => self::UINT, 'default' => \XF::$time],
            'reason' => ['type' => self::STR, 'maxLength' => 255, 'default' => ''],
            'last_triggered_date' => ['type' => self::UINT, 'default' => 0]
        ];
        $structure->getters = [];
        $structure->relations = [
            'User' => [
                'entity' => 'XF:User',
                'type' => self::TO_ONE,
                'conditions' => [
                    ['user_id', '=', '$create_user_id']
                ],
                'primary' => true
            ]
        ];

        return $structure;
    }
 
As someone who has provided tier 2 and tier 1 level customer support at a call center, a duplicate should simply be bypassed and processing continue onward. To halt any process because of a duplicate entry is not best practice. It should, if being ran interactively, warn you. Otherwise it should simply make a note in a log so it can be addressed later. Any other coding aspect is not best practice.
 
Top Bottom