Bug #53417

ConfigurationManager->getConfigurationValueByPath() ignores AdditionalConfiguration.php

Added by Viktor Livakivskyi over 1 year ago. Updated 4 months ago.

Status:Rejected Start date:2013-11-07
Priority:Should have Due date:
Assigned To:- % Done:

0%
Category:- Spent time: -
Target version:-
TYPO3 Version:6.0 Is Regression:No
PHP Version:5.4 Sprint Focus:
Complexity:

Description

Hi.

Recently I've discovered an issue in ConfigurationManager. If I override something in AdditionalConfiguration.php, e.g. $GLOBALS['TYPO3_CONF_VARS']['BE']['compressionLevel'] = 4;, while in my LocalConfiguration.php I have this value set to 5, I'll get 4 in Install Tool, but following code will return 5:

$cm = \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance('TYPO3\\CMS\\Core\\Configuration\\ConfigurationManager');
return $cm->getLocalConfigurationValueByPath('BE/compressionLevel');

Same result will be for ConfigurationManager->getConfigurationValueByPath(), which is used by Extension Manager and therefore I'm not able to override $GLOBALS['TYPO3_CONF_VARS']['EXT']['extConf'] for any extension, which makes it impossible to have different configurations for dev-stage-live environments.

Related issues

related to Core - Bug #57289: Respect additional configuration file for silent configur... Closed 2014-03-25
duplicated by Core - Bug #60650: ConfigurationManager::getConfigurationValueByPath ignores... Resolved 2014-07-29

History

#1 Updated by Viktor Livakivskyi about 1 year ago

Small update.
getLocalConfigurationValueByPath() may ignore AdditionalConfiguration.php, becasue it is explicitly mentioned in method's name, that it takes values form LocalConfiguration.php
but
getConfigurationValueByPath() should include defaultConfiguration merged with localConfiguration and merged with additionalConfiguration

#2 Updated by Markus Klein about 1 year ago

@Christian: Can you maybe say something about this?

#3 Updated by Gerrit Code Review 4 months 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 http://review.typo3.org/38041

#4 Updated by Gerrit Code Review 4 months ago

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

#5 Updated by Mathias Brodala 4 months ago

IIRC this is on purpose since AdditionalConfiguration.php can contain arbitrary PHP code. This can break the install tool which is supposed to be the last resort in case of breakage and basically unbreakable.

#6 Updated by Viktor Livakivskyi 4 months ago

Mathias, you're right. But then ConfigurationManager->getConfigurationValueByPath() makes no sense to use, because AdditionalConfiguration.php is ignored. AdditionalConfiguration is intended to be used to override configuration from LocalConfiguration, and we need to rely on this in client code.

E.g. we have following in AdditionalConfiguration:

switch (\TYPO3\CMS\Core\Utility\GeneralUtility::getApplicationContext()) {
    case 'Development':
        $GLOBALS['TYPO3_CONF_VARS']['SYS']['displayErrors'] = 1;

        /* skip */

        break;
    case 'Production/Staging':
        $GLOBALS['TYPO3_CONF_VARS']['SYS']['displayErrors'] = -1;

        /* skip */

        break;
    case 'Production':
    default:
        $GLOBALS['TYPO3_CONF_VARS']['SYS']['displayErrors'] = 0;

        /* skip */

}

And if I get ConfigurationManager->getConfigurationValueByPath('SYS/displayErrors') I'll not get the value, dependant on context, but one, defined in LocalConfiguration.php
So, I can't rely on this value.

#7 Updated by Markus Klein 4 months ago

ConfigurationManager is internal API, see class header

  • IMPORTANT:
  • This class is intended for internal core use ONLY.

#8 Updated by Viktor Livakivskyi 4 months ago

Why is it then mentioned in "What's New" slides of TYPO3 6.0 on page 35 in "Coding Examples"?

http://forge.typo3.org/attachments/download/24568/TYPO3-v6-0-whats-new.pdf

Maybe, makes sense to add remark in that PDF, that developes shouldn't use it or completely remove that page?

UPDATE: also checked code for 6.0 and 6.1 - there were no remark about @internal, therefore I suggested it's usage in client code at the moment of creating this issue

#9 Updated by Markus Klein 4 months ago

Ok that is really bad. Although the title of the slide talks about "LocalConfiguration".
I doubt anybody will touch the slides, since 6.0 is not supported for a long time now.

#10 Updated by Viktor Livakivskyi 4 months ago

Markus, you're right.
Since 6.2 marks this class as internal, then it makes no sense to use it in client code.

So, the issue may be closed.

#11 Updated by Christian Kuhn 4 months ago

  • Status changed from Under Review to Rejected

Hey.

It was unfortunate that the internal low level class to handle LocalConfiguration was mentioned in the slides of former core versions. We clarified with the @internal annotation only later and now in result stumble upon the one or other one complaining about the situation. Sorry about that.

The issue however is bogus since AdditionalConfiguration is not handled at this point by intention - otherwise settings from additionalconfiguration could end up in localconfiguration on writing, which is not wanted.

All "using / 3rd party code" should operate (reading) on $GLOBALS['TYPO3_CONF_VARS'] directly and will then have the "final" array.

This issue will be closed as rejected now.

Also available in: Atom PDF