PHP 8 warning in MediaManager

[ameotoko]

My Sentry reported this:

ErrorException: Warning: Undefined array key 0

#19 /vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Widget/MediaManager.php(174): Isotope\Widget\MediaManager::validateUpload
#18 /vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Widget/MediaManager.php(98): Isotope\Widget\MediaManager::ajaxUpload
#17 /vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Backend.php(335): Isotope\Backend::executePostActions
#16 /vendor/contao/core-bundle/src/Resources/contao/classes/Ajax.php(509): Contao\Ajax::executePostActionsHook
#15 /vendor/contao/core-bundle/src/Resources/contao/classes/Ajax.php(491): Contao\Ajax::executePostActions
#14 /vendor/contao/core-bundle/src/Resources/contao/classes/Backend.php(430): Contao\Backend::getBackendModule
#13 /vendor/contao/core-bundle/src/Resources/contao/controllers/BackendMain.php(168): Contao\BackendMain::run
#12 /vendor/contao/core-bundle/src/Controller/BackendController.php(49): Contao\CoreBundle\Controller\BackendController::mainAction
#11 /vendor/symfony/http-kernel/HttpKernel.php(163): Symfony\Component\HttpKernel\HttpKernel::handleRaw
#10 /vendor/symfony/http-kernel/HttpKernel.php(75): Symfony\Component\HttpKernel\HttpKernel::handle
#9 /vendor/symfony/http-kernel/Kernel.php(202): Symfony\Component\HttpKernel\Kernel::handle
#8 /vendor/symfony/http-kernel/HttpCache/SubRequestHandler.php(86): Symfony\Component\HttpKernel\HttpCache\SubRequestHandler::handle
#7 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(481): Symfony\Component\HttpKernel\HttpCache\HttpCache::forward
#6 /vendor/symfony/framework-bundle/HttpCache/HttpCache.php(73): Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache::forward
#5 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(271): Symfony\Component\HttpKernel\HttpCache\HttpCache::pass
#4 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(287): Symfony\Component\HttpKernel\HttpCache\HttpCache::invalidate
#3 /vendor/friendsofsymfony/http-cache/src/SymfonyCache/EventDispatchingHttpCache.php(126): Contao\ManagerBundle\HttpKernel\ContaoCache::invalidate
#2 /vendor/symfony/http-kernel/HttpCache/HttpCache.php(215): Symfony\Component\HttpKernel\HttpCache\HttpCache::handle
#1 /vendor/friendsofsymfony/http-cache/src/SymfonyCache/EventDispatchingHttpCache.php(98): Contao\ManagerBundle\HttpKernel\ContaoCache::handle
#0 /public/index.php(44): null

[aschempp]

I'm not sure that's the right fix. There are multiple checks before that line that files are actually uploaded, including two lines above. So apparently the array does use another key than 0? Can you reproduce the problem locally and check what the array key is?

[ameotoko]

So apparently the array does use another key than 0?

The bug is that the $varInput might not be an array at all. It is intialized as an empty string in line 150, and if $objUploader->uploadTo() throws an exception in the very next line, then $varInput remains a string and is never modified before that return.

core/system/modules/isotope/library/Isotope/Widget/MediaManager.php

Lines 150 to 174 in 8a48dde

	$varInput = '';
	try {
	$varInput = $objUploader->uploadTo($uploadFolder);
	} catch (\Exception $e) {
	$this->addError($e->getMessage());
	}
	if ($objUploader->hasError()) {
	$messages = System::getContainer()->get('session')->getFlashBag()->peek('contao_be_error');
	if (\is_array($messages)) {
	foreach ($messages as $strError) {
	$this->addError($strError);
	}
	}
	}
	Message::reset();
	if (!\is_array($varInput) || empty($varInput)) {
	$this->addError($GLOBALS['TL_LANG']['MSC']['mmUnknownError']);
	}
	return $varInput[0];

[aschempp]

Did you get that upload error yourself? Can you maybe screenshot what's visible in the back end? In the code it looks like there would be at least 3 error messages for the same thing?

[aschempp]

while you're at it, maybe you can also verify #2468 ?

[ameotoko]

EDIT: #2468 precisely describes the problem below, but I only checked it after posting this comment😁

ORIGINAL: I have some new info related to this piece of code, but I am not sure whether it is related to this issue.

If filesize of uploaded image is bigger than allowed, than $objUploader->hasError() is true, but no exception is thrown and no flash messages are returned ($messages is empty). In the backend, "Upload failed" error message is shown without this PR's fix, and "The file could not be uploaded for unknown reason. Please check the system log." with the fix.

In the screenshot, first message is before the fix is applied, second - after:

The system log is empty btw, despite what the message says.

The reason is – there is no contao_be_error entry in the flash bag. Instead, it contains contao.BE.error, which contains the correct error message:

if ($objUploader->hasError()) {
-   $messages = System::getContainer()->get('session')->getFlashBag()->peek('contao_be_error');
+   $messages = System::getContainer()->get('session')->getFlashBag()->peek('contao.BE.error');

    if (\is_array($messages)) {
        foreach ($messages as $strError) {
            $this->addError($strError);
        }
    }
}

[aschempp]

so fixing the name of the session bag name would make the change in this PR unnecessary?

[ameotoko]

No, it would just fix my specific case and #2468. But as far as I understand, in PHP 8 you can't just initialize a variable as a string and then access it as an array later.