Bug #60650

ConfigurationManager::getConfigurationValueByPath ignores AdditionalConfiguration.php

Added by Jost Baron about 5 years ago. Updated 12 months ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2014-07-29
Due date:
% Done:

100%

TYPO3 Version:
6.2
PHP Version:
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

Judging by the uses in the core, it should query the final $GLOBALS['TYPO3_CONF_VARS']-array.


Related issues

Related to TYPO3 Core - Bug #52557: Show message and disallow modification of non DefaultConfiguration values in install tool Rejected 2013-10-06
Related to TYPO3 Core - Bug #60294: If a local file storage is outside the document root, images are sometimes not shown if multiple images are on a page. Closed 2014-07-13
Related to TYPO3 Core - Bug #62301: ConfigurationManager setters make system unstable Rejected 2014-10-17
Duplicates TYPO3 Core - Bug #53417: ConfigurationManager->getConfigurationValueByPath() ignores AdditionalConfiguration.php Rejected 2013-11-07

Associated revisions

Revision 2d574641 (diff)
Added by Markus Klein almost 5 years ago

[TASK] Add note about AdditionalConfiguration.php to Install Tool

Add a note to clarify that AdditionalConfiguration.php is always
overruling any setting made within the Install Tool.

Furthermore we add a note to the ConfigurationManager that it is limited
to Core internal usage only.

Resolves: #60650
Releases: master, 6.2
Change-Id: Icaf7d4b1ce5b14ad0618e38e92a49a33405997f7
Reviewed-on: http://review.typo3.org/35409
Reviewed-by: Christian Kuhn <>
Tested-by: Christian Kuhn <>
Reviewed-by: Anja Leichsenring <>
Tested-by: Anja Leichsenring <>
Reviewed-by: Wouter Wolters <>
Tested-by: Wouter Wolters <>
Reviewed-by: Alexander Opitz <>
Tested-by: Alexander Opitz <>

Revision 26077111 (diff)
Added by Markus Klein almost 5 years ago

[TASK] Add note about AdditionalConfiguration.php to Install Tool

Add a note to clarify that AdditionalConfiguration.php is always
overruling any setting made within the Install Tool.

Furthermore we add a note to the ConfigurationManager that it is limited
to Core internal usage only.

Resolves: #60650
Releases: master, 6.2
Change-Id: Icaf7d4b1ce5b14ad0618e38e92a49a33405997f7
Reviewed-on: http://review.typo3.org/35412
Reviewed-by: Anja Leichsenring <>
Tested-by: Anja Leichsenring <>

History

#1 Updated by Markus Klein about 5 years ago

So you're using the ConfigurationManager in your code somewhere to retrieve configuration, right?

#2 Updated by Jost Baron about 5 years ago

Yes, only for read access - is it not meant to be done that way? Since the core seems to do it, too

#3 Updated by Markus Klein about 5 years ago

  • Status changed from New to Needs Feedback

Were does the Core us it too?

AFAIK ConfigurationManager from Core is to be used only internally (currently only in bootstrap and install tool) in order to manage the various configuration files.
All other Core parts as well as extensions should really use the global TYPO3_CONF_VARS array which hold the real configuration (including everything from AdditionalConfiguration.php)

Be aware that there is another ConfigurationManager from extbase, which is intended to be used by extensions too.

#4 Updated by Jost Baron about 5 years ago

There is one place on the extension manager using this method (fetching the current extension configuration under EXT/extConf/extkey), and several instances in the install tool.

The usage in the extension manager prevents changed extension settings from AdditionalConfiguration.php to be shown in the configuration area of the extension manager. This could be a problem, since the value that is actually used and the value that is shown value are different. To reproduce, add this line to your AdditionalConfiguration.php

$GLOBALS['TYPO3_CONF_VARS']['EXT']['extConf']['rsaauth'] = 'a:1:{s:18:"temporaryDirectory";s:0:"";}';

and change the setting via extension manager. Then the changed setting is shown, but the above setting is used.

I'm not sure about the usages in the install tool, some seem legit (at least under certain assumptions, for example that there is no AdditionalConfiguration.php to be respected while the initial setup is running), about some others I'm not so sure. For example, a configuration preset is marked 'inactive', if its settings are there, but only in AdditionalConfiguration.php.

#5 Updated by Christian Kuhn about 5 years ago

Well, yeah.

The core reads some information directly from LocalConfiguration to be sure that the overload file AdditionalConfiguration is ignored at some places.

The problem with AdditionalConfiguration is, that we can not realiably detect or find out what exactly is changed there:
The related issue #52557 tries to detect in "all configuration" in install tool if a value is changed by Additional configuration by comparing the setting within LocalConfiguration and the one in $GLOBALS['TYPO3_CONF_VARS']. If it finds a difference, then it knows "ok, this value was probably changed in AdditionalConfiguration". But this is not reliable since there may be code that only hits if the system is production or dev, or if it is 1 o'clock in the morning or other stuff. Even "grepping" in AdditionalConfiguration for code may not be reliable since we do not know if someone included other files.
So neither executing nor scanning this file gives us a definite knowledge if someone change something in this file.

Therefor, AdditionalConfiguration is currently "ignored" by the core. Core basically says: If you have funny code in this file, that change settings, then those settings may not be settable by core handling code anymore, since we never know what and how you are doing things in there. Thus, it is in developers hands. If a developer overwrites settings there (eg. based on context environment variable), then these settings are basically out-of-support by core code like install tool or em. If a dev forgets about overwritten stuff in this file, and then wonders why his settings changed by install tool or em does not work, well, i just see no good option to help there. In my opinion we can not even help to detect wether an option was changed or not, any code we would come up with is probably doomed to fail in some situations.

Thus, I tend to say: If you use AdditionalConfiguration, then you must take care of those options on your own, together with the according side effects it may have.

I'd close this issue as rejected for these reasons, and I tend to also vote down Sascha's pending patch for #52557.

Maybe, if you align with this view, we could think about how to hint devs better about this fact. Maybe we could add hints (to install tool or/and em) that settings changed within AdditionalConfiguration are ignored by the install tool / em code and changing them within these modules may not have the desired effect. This way, we would at least achieve a better information strategy about this situation.

#6 Updated by Jost Baron about 5 years ago

Hi Christian,

I still have some concerns:

1. Because of some blog posts and other stuff easily found with Google, many people use the ConfigurationManager::getConfigurationValueByPath() method (and also the setters). That's how I came to use it, too. There is no clear indication that it should not be used outside the core, and there is obviously the need for a service like it, which behaves as expected, and which can also be mocked easily.

Is it a good idea to add another "configuration manager" to the core, but for use in extensions to query and modify the GLOBALS['TYPO3_CONF_VARS'] array? Maybe even replacing the current ConfigurationManager with that one, and moving the current ConfigurationManager to some place else and giving it a more speaking name, like LocalConfigurationFileManager?

2. Its clear that the install tool can't ever analyze which options are changed by the AdditionalConfiguration.php. That problem is probably equivalent to the halting problem. But I think what Sascha does makes (partly) sense. There should be a warning that a settings value is not taken from the current LocalConfiguration.php, but comes from somewhere else (in the current context). Just a warning, to make debugging easier. Same thing for the extension manager.

#7 Updated by Christian Kuhn about 5 years ago

1. I think writing the LocalConfiguration file should be a task done by em / install tool (=core) only. Extensions should rely on $GLOBALS['TYPO3_CONF_VARS'] only, not on using some API (BTW: this array can be easily set/mocked within unit test, core tests do that at a lot of places). Maybe we should mark the class and its methods as "@internal, do not use in extensions, use $GLOBALS['TYPO3_CONF_VARS'] directly" to hint devs about this core-internal class, and is not public API.

2. Yep, better hinting in the em / install tool that stuff in AdditionalConfiguration may break "All configuration" (and maybe more?) is a good idea.

#8 Updated by Christian Kuhn almost 5 years ago

  • Status changed from Needs Feedback to Accepted

Tasks to do:

  • State more clearly in ConfigurationManager that this is a lowlevel core API that is usually only touched by core, and that extensions usually shouldn't operate with it.
  • Add something to the install tool that changing AllConfiguration (and more? maybe presets) may not have the desired effect if AdditionalConfiguration overwrites this stuff again.

#9 Updated by Gerrit Code Review almost 5 years ago

  • Status changed from Accepted 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/35409

#10 Updated by Gerrit Code Review almost 5 years 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/35409

#11 Updated by Gerrit Code Review almost 5 years ago

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

#12 Updated by Gerrit Code Review almost 5 years ago

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

#13 Updated by Markus Klein almost 5 years ago

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

#14 Updated by Gerrit Code Review almost 5 years ago

  • Status changed from Resolved to Under Review

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

#15 Updated by Markus Klein almost 5 years ago

  • Status changed from Under Review to Resolved

#16 Updated by Benni Mack 12 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF