Dimensions are not saved if image is added with BBCODE manually

[Chromaniac]

Affected version

2.3 Beta 4

So, this has been tested on a brand new Xenforo 2.3 Beta 4 install.

Image proxy disabled. Images added directly with BBCODE either in visual mode or bbcode mode... Dimensions are not tracked. I even tried using preview but that did not make a difference. (Preview push unfurled URL data to database even if the post is never posted!)

Turned on proxy. Proxy did not fetch existing embedded image dimensions. Images are loaded with blank width and height attributes.

If an external image is added with the toolbar image insert button, dimensions are fetched on the spot and can also be seen in the bbcode view.

Reference post here.

Optimally speaking, external image dimensions should be recorded even if they are embedded manually using BBCODE in either of the post editor view. As stated by @Kirby here.

Images posted directly with [img] bbcode do seem to get their dimensions recorded if proxy is enabled. So, this works for new images but not historic images. If I disable proxy, the image retains dimensions which is nice! Xenforo also updates image dimension of old post images (without dimensions) if proxy is enabled and I edit the post and save it. Nice but not an optimal solution. Unless Rebuild post embed metadata would automatically do it for historic posts as indicated here? Probably would result in blacklisted server IP.

Expected behavior is if an old image has been proxied, data is stored in post meta data, and dimensions would be used even if proxy is disabled in future for the said image! This behavior would be very nice as forum owners can turn proxy on for a few weeks and fetch dimension data for historic images automatically! Though since Imgur blocks proxy, proxy route would not work for a lot of images shared on forums over the years . Though one can use the DigitalPoint Cloudflare addon to make it work on temporary basis.

 

[Kirby]

The bug is actually a lot more complex than it looks on the surface and IMHO a proper fix requires quite some work.

Let's start at \XF\BbCodeProcessor\AnalyzeUsage::analyzeImgTag

public function analyzeImgTag(array $tag, array $options, $finalOutput, Processor $processor): void
{
    if (empty($tag['children']))
    {
        return;
    }

    $url = $tag['children'][0];

    if (filter_var($url, FILTER_VALIDATE_URL) === false)
    {
        return;
    }

    $size = $tag['option']['size'] ?? null;
    $hash = md5($url);

    if ($size)
    {
        [$width, $height] = explode('x', $size, 2);
        $width = (int) $width;
        $height = (int) $height;
    }
    else if (\XF::options()->imageLinkProxy['images'])
    {
        $imageProxyService = \XF::service(\XF\Service\ImageProxy::class);
        $proxiedImage = $imageProxyService->getImage($url);

        if ($proxiedImage)
        {
            $width = $proxiedImage->width;
            $height = $proxiedImage->height;
        }
    }

    $this->images[$hash] = [
        'url' => $url,
        'width' => $width ?? null,
        'height' => $height ?? null
    ];
}

Now assume that the image proxy is disabled and the following input is processed:

[IMG size="192x192"]https://xenforo.com/community/data/avatars/l/11/11388.jpg?1701366656[/IMG]
[img]https://xenforo.com/community/data/avatars/l/11/11388.jpg?1701366656[/img]

The first call to the method (for the first [img] tag) will set the dimensiosn for the image in $this->images[$hash]
But the second call (for the second [img]) will overwrite width and height to null as there is no size attribute for that tag and the image proxy is disabled.

Fixing this is easy, the code just needs to be changed so it doesn't overwrite existing width / height with null.

If there is no size attribute things get more complicated - in this case the dimensions are only attempted to be recorded if the image proxy is enabled due to if (\XF::options()->imageLinkProxy['images']).
Removing the condition would be easy, but if this is done the image proxy would start to save the images (and thus waste disk space).
This could also be addressed relatively easily by setting some kind of flag on the servie to not save the image (just the metadate in the database).

But now it gets complicated:
The image proxy has a concurrency limit, if that is reached $imageProxyService->getImage($url) will return an entity without dimensions set.

If we don't want dimensions to just fail if this happens it would be necessary to implement some async backoff & retry mechanism for each entity - I unfortunately don't see a way to implement this in the message preparer as the entity might not yet have been saved when it is invoked.

Entity classes ProfilePost, ProfilePostComment and ConversationMessage are entirely missing code to set image dimensions, see https://xenforo.com/community/profile-posts/32525/ as an example.

Last but not least jobs PostEmbedMetadata, ProfilePostEmbedMetadata, ProfilePostCommentEmbedMetadata & ConversationEmbedMetadata don't have code to rebuild embed metadata for images; probably would make sense to add this to AbstractEmbedMetadataJob.

The following patch set is an attempt to fix some of those issues, but it is not complete (no backoff / retry, missing necessary template changes for tools_rebuild)

--- src/XF/BbCode/ProcessorAction/AnalyzeUsage.php    Tue Apr 16 21:49:18 2024
+++ src/XF/BbCode/ProcessorAction/AnalyzeUsage.php    Tue Apr 16 22:03:32 2024
@@ -199,2 +199,3 @@
         $hash = md5($url);
+        $width = $height = null;
 
@@ -206,5 +207,6 @@
         }
-        else if (\XF::options()->imageLinkProxy['images'])
+        else
         {
             $imageProxyService = \XF::service(\XF\Service\ImageProxy::class);
+            $imageProxyService->setKeepImage(false);
             $proxiedImage = $imageProxyService->getImage($url);
@@ -218,7 +220,10 @@
 
-        $this->images[$hash] = [
-            'url' => $url,
-            'width' => $width ?? null,
-            'height' => $height ?? null
-        ];
+        if ($width || $height)
+        {
+            $this->images[$hash] = [
+                'url' => $url,
+                'width' => $width ?? null,
+                'height' => $height ?? null
+            ];
+        }
     }

--- src/XF/Entity/ConversationMessage.php    Tue Mar 19 22:31:32 2024
+++ src/XF/Entity/ConversationMessage.php    Tue Apr 16 23:17:10 2024
@@ -206,3 +206,4 @@
             'viewAttachments' => true,
-            'unfurls' => $this->Unfurls ?: []
+            'unfurls' => $this->Unfurls ?: [],
+            'images' => $this->embed_metadata['images'] ?? []
         ];

--- src/XF/Entity/ProfilePost.php    Tue Apr 16 23:15:38 2024
+++ src/XF/Entity/ProfilePost.php    Tue Apr 16 23:08:24 2024
@@ -554,3 +554,4 @@
             'viewAttachments' => $this->canViewAttachments(),
-            'unfurls' => $this->Unfurls ?: []
+            'unfurls' => $this->Unfurls ?: [],
+            'images' => $this->embed_metadata['images'] ?? [],
         ];

--- src/XF/Entity/ProfilePostComment.php    Tue Apr 16 23:16:51 2024
+++ src/XF/Entity/ProfilePostComment.php    Tue Apr 16 23:16:51 2024
@@ -273,3 +273,3 @@
             'viewAttachments' => $this->canViewAttachments(),
-            'unfurls' => $this->Unfurls ?: []
+            'unfurls' => $this->Unfurls ?: [],
             'images' => $this->embed_metadata['images'] ?? []

--- src/XF/Job/AbstractEmbedMetadataJob.php    Tue Mar 19 22:31:32 2024
+++ src/XF/Job/AbstractEmbedMetadataJob.php    Tue Apr 16 22:57:07 2024
@@ -148,2 +148,7 @@
 
+    protected function rebuildImages(Entity $record, \XF\Service\Message\Preparer $preparer, array &$embedMetadata): void
+    {
+        $embedMetadata['images'] = $preparer->getEmbeddedImages();
+    }
+
     public function getStatusMessage()

--- src/XF/Service/ImageProxy.php    Tue Mar 19 22:31:32 2024
+++ src/XF/Service/ImageProxy.php    Tue Apr 16 21:55:13 2024
@@ -9,2 +9,3 @@
     protected $forceRefresh = false;
+    protected $keepImage = true;
     protected $maxConcurrent = 10;
@@ -26,2 +27,7 @@
 
+    public function setKeepImage(bool $keepImage): void
+    {
+        $this->keepImage = $keepImage;
+    }
+
     public function isRefreshForced()
@@ -249,3 +255,10 @@
             $fileHash = md5_file($fetchResults['dataFile']);
-            if (!$image->pruned && $image->file_hash === $fileHash)
+          
+            $pruned = false;
+          
+            if (!$this->keepImage)
+            {
+                $pruned = $saved = true;
+            }
+            else if (!$image->pruned && $image->file_hash === $fileHash)
             {
@@ -272,3 +285,3 @@
                 $image->mime_type = $fetchResults['mimeType'];
-                $image->pruned = false;
+                $image->pruned = $pruned;
                 $image->failed_date = 0;

--- src/XF/Service/Message/Preparer.php    Tue Mar 19 22:31:32 2024
+++ src/XF/Service/Message/Preparer.php    Tue Apr 16 22:59:10 2024
@@ -228,2 +228,7 @@
 
+    public function getEmbeddedImages(): array
+    {
+        return $this->images;
+    }
+
     public function getEmbedMetadata()