Fixed Template for disabled add-ons will be loaded even when this is unexpected (ie alerts)

[Xon]

Affected version

2.0.4

This is a combination of two problems;

• An alert template owned by a disabled add-on will be loaded.
• There appears to be no way to check if an entity implements a particular getter/function from templates.

This means custom alerts will error if the UserAlert is extended, or any entity which gets passed to the alert is extended due.

empty() doesn't function like php-code, can't call offsetExists, or isset to guard against a template accessing something they shouldn't.

 

[Chris D]

An alert template owned by a disabled add-on will be loaded.

Are you certain about this?

My test case is simply inserting an alert into xf_user_alert for the xfmg_media content type and an action that doesn't exist.

With XFMG enabled, I get a template error (because it's trying to load a template and the template doesn't exist). With XFMG disabled, I don't get anything, because the alert handler for xfmg_media would not be available and therefore would be filtered out of the list.

Your second point, although I can appreciate its usefulness, isn't actually a bug and more of a suggestion.

 

[Xon]

I think the problem comes by adding a new alert action to a content type owned by another add-on. ie; alert_post_rating

 

[Chris D]

If you add a new alert action to an existing content type, and the add-on which owns that content type is disabled, the alert still won't show because the handler won't be available.

That said, we did only add that in XF 2.0.3 so it may be possible that if you've had reports of this from users of your add-ons, they may be using XF 2.0.2 or below.

 

[Xon]

I've created https://xenforo.com/community/threads/template-empty-does-not-match-php-empty-behaviour.146275/ I'm guessing this should be a feature request, but I've described how it can cause unexpected behaviour.

If you add a new alert action to an existing content type, and the add-on which owns that content type is disabled, the alert still won't show because the handler won't be available.

If add-on A owns template B, which injects a new alert action into add-on C's content type, is add-on A is disabled template B is still loaded if add-on C is enabled.

This becomes an issue, as if Template B touches functions or entity properties that add-on A also injects, you will get the error log spammed with errors.

 

[Chris D]

I obviously appreciate why you would say it is unexpected, though realistically it isn't totally unexpected. In this case, the content type is owned by an enabled add-on and therefore that will attempt to render any template it needs to.

The only viable solution we can come up with at the moment is a way to tie alerts to add-ons, e.g. by adding something like a addon_id field to the xf_user_alert table and the alert list filtering out alerts where the field is not null and the add-on is not active.

In the meantime, the workaround (for the empty stuff too) is very simple. I recommend wrapping your alert template in this:

<xf:if is="is_addon_active('Your/AddOn')">
    // Your alert text that relies on certain code being available
<xf:else />
    // A more basic alert text that can be rendered without any additional code
</xf:if>

 

[Chris D]

I'm not 100% happy with this approach but it gets the job done probably with the least amount of pain possible. Not 100% happy with the dual purpose of the $extra stuff here but certainly beats adding new methods or changing existing method signatures at this stage.

Index: src/XF/Install/Upgrade/2000570-205.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/XF/Install/Upgrade/2000570-205.php    (date 1524572546000)
+++ src/XF/Install/Upgrade/2000570-205.php    (date 1524572546000)
@@ -0,0 +1,21 @@
+<?php
+
+namespace XF\Install\Upgrade;
+
+use XF\Db\Schema\Alter;
+
+class Version2000570 extends AbstractUpgrade
+{
+    public function getVersionName()
+    {
+        return '2.0.5';
+    }
+
+    public function step1()
+    {
+        $this->schemaManager()->alterTable('xf_user_alert', function(Alter $table)
+        {
+            $table->addColumn('depends_on_addon_id', 'varbinary', 50)->setDefault('');
+        });
+    }
+}
\ No newline at end of file
Index: src/XF/Entity/UserAlert.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/XF/Entity/UserAlert.php    (date 1524571883000)
+++ src/XF/Entity/UserAlert.php    (date 1524572661000)
@@ -127,7 +127,8 @@
             'action' => ['type' => self::STR, 'maxLength' => 30, 'required' => true],
             'event_date' => ['type' => self::UINT, 'default' => \XF::$time],
             'view_date' => ['type' => self::UINT, 'default' => 0],
-            'extra_data' => ['type' => self::SERIALIZED_ARRAY, 'default' => []]
+            'extra_data' => ['type' => self::SERIALIZED_ARRAY, 'default' => []],
+            'depends_on_addon_id' => ['type' => self::BINARY, 'maxLength' => 50, 'default' => '']
         ];
         $structure->getters = [
             'Content' => true
@@ -145,6 +146,12 @@
                 'conditions' =>[['user_id', '=', '$alerted_user_id']],
                 'primary' => true
             ],
+            'AddOn' => [
+                'entity' => 'XF:AddOn',
+                'type' => self::TO_ONE,
+                'conditions' => [['addon_id', '=', '$depends_on_addon_id']],
+                'primary' => true
+            ]
         ];
 
         return $structure;
Index: src/XF/Install/Data/MySql.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/XF/Install/Data/MySql.php    (date 1524571883000)
+++ src/XF/Install/Data/MySql.php    (date 1524572558000)
@@ -1996,6 +1996,7 @@
             $table->addColumn('event_date', 'int');
             $table->addColumn('view_date', 'int')->setDefault(0)->comment('Time when this was viewed by the alerted user');
             $table->addColumn('extra_data', 'mediumblob')->comment('Serialized. Stores any extra data relevant to the alert');
+            $table->addColumn('depends_on_addon_id', 'varbinary', 50)->setDefault('');
             $table->addKey(['alerted_user_id', 'event_date'], 'alertedUserId_eventDate');
             $table->addKey(['content_type', 'content_id'], 'contentType_contentId');
             $table->addKey(['view_date', 'event_date'], 'viewDate_eventDate');
Index: src/XF/Repository/UserAlert.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/XF/Repository/UserAlert.php    (date 1524571883000)
+++ src/XF/Repository/UserAlert.php    (date 1524572786000)
@@ -17,6 +17,9 @@
     {
         $finder = $this->finder('XF:UserAlert')
             ->where('alerted_user_id', $userId)
+            ->whereAddOnActive([
+                'column' => 'depends_on_addon_id'
+            ])
             ->order('event_date', 'desc')
             ->with('User');
 
@@ -94,6 +97,13 @@
             return false;
         }
 
+        $dependsOn = '';
+        if (isset($extra['depends_on_addon_id']))
+        {
+            $dependsOn = $extra['depends_on_addon_id'];
+            unset($extra['depends_on_addon_id']);
+        }
+
         $alert = $this->em->create('XF:UserAlert');
         $alert->alerted_user_id = $receiverId;
         $alert->user_id = $senderId;
@@ -102,6 +112,7 @@
         $alert->content_id = $contentId;
         $alert->action = $action;
         $alert->extra_data = $extra;
+        $alert->depends_on_addon_id = $dependsOn;
         $alert->save();
 
         return true;

So, ultimately, just pass a value to the depends_on_addon_id key of your $extra data.