Project

General

Profile

Actions

Bug #89148

closed

Media viewhelper empty additionalConfig causes php warning

Added by Jarvis H about 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2019-09-11
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
9
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

Typo3 version 9.5.9

Error message:

PHP Warning: array_merge_recursive(): Argument #2 is not an array in /typo3/sysext/fluid/Classes/ViewHelpers/MediaViewHelper.php line 119

How to reproduce.

Remove typoscript setting:

lib.contentElement.settings.media.additionalConfig >

or remove value from additionalConfig argument in fluid partial FluidStyledContent/Partials/Media/Rendering/Video.html

<f:media file="{file}" class="{class}" additionalAttributes="" alt="{file.alternative}" title="{file.title}" additionalConfig="" />

Cause:

In TYPO3\CMS\Fluid\ViewHelpers\MediaViewHelper the following takes place without checking if $additionalConfig is an array

$additionalConfig = array_merge_recursive($this->arguments, $additionalConfig);

Possible solution:

Add is_array check:

if (!is_array($additionalConfig)) {
    $additionalConfig = [];
}

$additionalConfig = array_merge_recursive($this->arguments, $additionalConfig);
Actions #1

Updated by Oliver Bartsch about 5 years ago

Hi,

I tried to reproduce this. It seems your examples also lead to another error message. So I got:

Argument 4 passed to TYPO3\CMS\Core\Resource\Rendering\VideoTagRenderer::render() must be of the type array, null given, called in /var/www/html/typo3/sysext/fluid/Classes/ViewHelpers/MediaViewHelper.php on line 120

As the VideoTagRenderer has following signature:

FileInterface $file, $width, $height, array $options = [], $usedPathsRelativeToCurrentScript = false

I get this because the Renderer gets instantiated before the `array_merge_recursive` methode is called.

However, `$additionalConfig` should actually be always an array from looking at the `registerArgument` call.
But what I can see is, that an empty string is treated as valid for type array by TYPO3Fluid\Fluid\Core\ViewHelper\AbstractViewHelper->isValidType().
Thats why the defaultValue - an empty array - doesn't get assigned to the argument.

Your proposed is_array check will fix this Error message, though we should takle the fact that an empty string is treated as valid while the type is actually defined as an array.

What do you think?

Actions #2

Updated by Oliver Bartsch about 5 years ago

To make it clear:

The declaration of

<f:media file="{file}" class="{class}" additionalAttributes="" alt="{file.alternative}" title="{file.title}" additionalConfig="" />

is however wrong. It should be:

<f:media file="{file}" class="{class}" additionalAttributes="" alt="{file.alternative}" title="{file.title}" additionalConfig="{}" />

So the bug here is the wrong type check. Your HTML should trigger a InvalidTypeException.
Something like: 'additionalConfig' was registered as type array but is of type string.

Actions #3

Updated by Jarvis H about 5 years ago

Thanks for the feedback. A couple of things to note:

- This issue only occurs on the first page load after changing the additionalConfig attribute in the video partial to additionalConfig="{}"

- The issue occurs every time if additionalConfig=""

I suspect your error for argument 4 in VideoTagRenderer is caused by the same underlying issue which is causing the php warning in the array_merge_recursive call.

As far as I can tell the issue arises because the value of the argument additionalConfig is set to null on the first page load and I traced it back to the ViewHelperInvoker class (vendor/typo3fluid/fluid/src/Core/ViewHelper/ViewHelperInvoker.php).

In the main invoke method, the following happens when additionalConfig="{}" is used (see my inline comments for clarification):


try {
    foreach ($expectedViewHelperArguments as $argumentName => $argumentDefinition) {
        if (isset($arguments[$argumentName])) {
            /** @var NodeInterface|mixed $argumentValue */
            $argumentValue = $arguments[$argumentName];
            $evaluatedArguments[$argumentName] = $argumentValue instanceof NodeInterface ? $argumentValue->evaluate($renderingContext) : $argumentValue;

            // --------------------------------------------------
            // After deleting system caches and loading page '$evaluatedArguments[$argumentName]' equals null
            // as $argumentValue is an instance of \TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\RootNode
            // where the property 'childNodes' is empty causing '$argumentValue->evaluate($renderingContext)' to return null
            if ($argumentName == 'additionalConfig') {
                \TYPO3\CMS\Core\Utility\DebugUtility::debug($evaluatedArguments[$argumentName]);
            }
            // --------------------------------------------------
        } else {
            // --------------------------------------------------
            // After loading page again '$evaluatedArguments[$argumentName]'
            // returns an empty array as it should
            $evaluatedArguments[$argumentName] = $argumentDefinition->getDefaultValue();
            if ($argumentName == 'additionalConfig') {
                \TYPO3\CMS\Core\Utility\DebugUtility::debug($evaluatedArguments[$argumentName]);
            }

            // --------------------------------------------------
        }
    }

    foreach ($arguments as $argumentName => $argumentValue) {
        if (!array_key_exists($argumentName, $evaluatedArguments)) {
            $undeclaredArguments[$argumentName] = $argumentValue instanceof NodeInterface ? $argumentValue->evaluate($renderingContext) : $argumentValue;
        }
    }

    if ($renderChildrenClosure) {
        $viewHelper->setRenderChildrenClosure($renderChildrenClosure);
    }
    $viewHelper->setRenderingContext($renderingContext);

    // --------------------------------------------------
    // Here the arguments property is set for the MediaViewHelper
    $viewHelper->setArguments($evaluatedArguments);
    // --------------------------------------------------

    $viewHelper->handleAdditionalArguments($undeclaredArguments);
    return $viewHelper->initializeArgumentsAndRender();
} catch (Exception $error) {
    return $renderingContext->getErrorHandler()->handleViewHelperError($error);
}

However this only adds to my confusion, as I am guessing it has something to do with the fluid caching mechanism.

Edit: The question is, what would be the expected behaviour when passing either an empty string or empty array to an argument which defaults to an array? InvalidTypeException for an empty string and no warning at all for an empty array?

Actions #4

Updated by Gerrit Code Review over 4 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/63593

Actions #5

Updated by Georg Ringer over 4 years ago

  • Category deleted (Fluid)
  • PHP Version deleted (7.2)
Actions #6

Updated by Gerrit Code Review over 4 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/63593

Actions #7

Updated by Gerrit Code Review over 4 years ago

Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/63733

Actions #8

Updated by Georg Ringer over 4 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #9

Updated by Benni Mack over 4 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF