Problem/Motivation

The configuration system can already store language specific override data. If locale module is not enabled, this data is not accessible (not a problem). If locale module is enabled, only the data localized to the page's interface language is accessible (which is the problem). No other language version is accessible on the page. This is a regression, because we cannot even send user emails in various languages on a page that is a basic feature of user and contact modules in prior versions.

The language overrides are implemented with a general system of configuration listeners and overrides that - though only used for localization and global $conf overrides in core -, allow modules to override configuration values depending on different request conditions (in contrib). Possible examples are organic group specific values or domain specific values (groups and domain modules in contrib). The core listener/override system needs more information about the config override values needed so it can pull in specific data as appropriate. These two use cases apply to core:

  • For localizing configuration to different languages during the same page request we need to add some contextual value (language) to the configuration API or values that can be used to derive language (such as a user account).
  • For administering configuration objects we need to get the default configuration values without any overrides so the values edited in base settings forms are consistent and don't depend on the current interface language.

(The question of how translation of configuration would be possible is evident. That has various usability / workflow / permissions considerations that will most likely not be possible to resolve in core in Drupal 8. A configuration translation user interface will need to live in contrib.)

Proposed solution

  1. Create a configuration context system (objects based on \Drupal\Core\Config\ContextInterface). These objects contain all the parameters we need to load/save configuration objects.
  2. Introduce a context factory (\Drupal\Core\Config\Context\ConfigContextFactory) that can be used to create context object instances.
  3. Set a default configuration context for the page request, and use different configuration context objects for different purposes (administration, send emails to users, etc).
  4. Extend the configuration API to make the config factory context aware, so config() calls would use the context set prior, until the context is left.

API changes, API usage

We have a number of configuration context objects which will determine how the configuration is loaded and overridden for each different purpose for which we are using it.

  • GlobalConfigContext, registered as (config.context with the DIC) which is used as the default configuration context for the page request. It takes values from global $conf as configuration overrides (Then used by ConfigOverrideSubscriber when loading configuration objects).
  • ConfigContext (without futher arguments, registered with the DIC as config.context.free), used for administration (settings forms), configuration install, import and export. The configuration listeners should 'keep hands off' configuration objects loaded for this context unless they need to act upon configuration updates. I.e. these configuration objects won't be overridden with global $conf nor with translations.
  • UserConfigContext, which is specific to user account based contexts (registered with the DIC as config.context.user).

An example of a user context is creation of user accounts. We need to send an email to the user using their language preference instead of the system default or the page request language. Since mail text is stored in the configuration, we use the context system when retrieving the mail text for that user. A user context is created and used to retrieve the mail text from the configuration system. The context is set on the config factory.

The new config_context_enter() and config_context_leave() API functions were added in system.module to enter and leave contexts. These are simplified versions of invocations of certain methods on the ContextFactory. For example this code is used in config import:

    // Use the override free context for config importing so that any overrides do
    // not change the data on import.
    config_context_enter('config.context.free');

   // ... actual import

    // Exit the override free context.
    config_context_leave();

A more complex example with entering a user context for user specific values:

  // Enter a user specific context.  
  $user_config_context = config_context_enter("Drupal\\user\\UserConfigContext");
  // Set the account to use on the context.
  $user_config_context->setAccount($account);

  // Now the configuration retrieved (and any subsequent config() calls in
  // API functions) will work with the context on the top of the context
  // stack (that we most recently entered).
  $mail_config = config('user.mail');

  // Use token_replace(), etc. here on the mail configuration to replace
  // with values proper for the context we entered (eg. the site name or
  // slogan properly translated to the language we use).

  // Reset config context to the prior value on the stack before leaving
  // this operation. This ensures any wrapping code will return to the
  // context that was set prior and our user specific context will not
  // persist.
  config_context_leave();

The locale module's LocaleConfigSubscriber is intelligent to recognize a user account's presence in the context and it sets the language according to the value derived from the user's preference. Other use cases may be a 'groups' module or a 'domain' module changing some configuration data depending on the user.

The point here is that a higher level context is provided and it is the responsibility of the underlying context system with the enabled modules to figure out configuration data for this context. The override/listener system in the config system is generic enough to allow for all kinds of overrides, so the context system is designed to be generic enough and not limit it to one or two specific types of contexts.

New classes introduced, classes changed

CMI context system (v. 2013Feb).png

References

Meta issue: #1616594: META: Implement multilingual CMI
Example of where this should be used (once converted to CMI): #1757566: Convert user account e-mail templates to configuration system

Follow-ups

CommentFileSizeAuthor
#279 minor-context-updates-278.patch11.82 KBYesCT
#279 interdiff-276-278.txt2.04 KBYesCT
#277 interdiff-bytheway.txt1011 bytesYesCT
#276 minor-context-updates-276.patch9.93 KBYesCT
#276 interdiff-271-276.txt315 bytesYesCT
#271 minor-context-updates-271.patch9.97 KBYesCT
#271 interdiff-270-271.txt1.95 KBYesCT
#270 minor-context-updates-270.patch8.03 KBYesCT
#270 interdiff-266-270.txt758 bytesYesCT
#266 minor-context-updates-266.patch8.26 KBYesCT
#261 minor-context-updates-261.patch8.7 KBdas-peter
#261 interdiff-minor-context-updates-251-261.txt3.83 KBdas-peter
#251 minor-context-updates-251.patch8.6 KBYesCT
#251 interdiff-250-251.txt5.09 KBYesCT
#250 minor-context-updates-250.patch4.97 KBGábor Hojtsy
#234 minor-context-updates-234.patch3.6 KBGábor Hojtsy
#232 minor-context-updates-232.patch1.23 KBGábor Hojtsy
#226 minor-context-updates.patch1.1 KBGábor Hojtsy
#213 209-213-interdiff.txt1.36 KBalexpott
#213 1763640-config-context-213.patch59.84 KBalexpott
#212 197-209-interdiff.txt5.54 KBalexpott
#212 1763640-config-context-209.patch59.57 KBalexpott
#197 192-197-interdiff.txt829 bytesalexpott
#197 1763640-config-context-197.patch59.49 KBalexpott
#192 188-192-interdiff.txt2.28 KBalexpott
#192 1763640-config-context-192.patch59.06 KBalexpott
#188 183-188-interdiff.txt4.49 KBalexpott
#188 1763640-config-context-188.patch58.78 KBalexpott
#183 181-183-interdiff.txt13.95 KBalexpott
#183 1763640-config-context-183.patch57.62 KBalexpott
#181 173-181-interdiff.txt3.12 KBalexpott
#181 1763640-config-context-181.patch57.96 KBalexpott
#177 CMI context system (v. 2013Feb).png67.34 KBGábor Hojtsy
#173 168-173-interdiff.txt13.13 KBalexpott
#173 1763640-config-context-173.patch58.15 KBalexpott
#168 165-168-interdiff.txt10.5 KBalexpott
#168 1763640-config-context-168.patch53.58 KBalexpott
#165 162-165-interdiff.txt6.03 KBalexpott
#165 1763640-config-context-165.patch49.87 KBalexpott
#162 147-162-interdiff.txt45.08 KBalexpott
#162 1763640-config-context-162.patch47.74 KBalexpott
#147 1763640-config-context-147.patch41.6 KBAnonymous (not verified)
#143 1763640-143-config-context.patch40.89 KBAnonymous (not verified)
#134 1763640-config-context-134.patch40.72 KBGábor Hojtsy
#132 1763640-config-context-132.patch40.7 KBGábor Hojtsy
#127 1763640-config-context-127.patch40.89 KBJose Reyero
#127 1763640-config-context-127-diff.txt1.73 KBJose Reyero
#124 1763640-config-context-124.patch39.16 KBJose Reyero
#124 1763640-config-context-124-diff.txt675 bytesJose Reyero
#118 1763640-config-context-118.patch38.98 KBgdd
#111 1763640-config-context-111.patch40.25 KBAnonymous (not verified)
#109 1763640-config-context-109.patch64.67 KBAnonymous (not verified)
#107 1763640-config-context-107.patch40.19 KBAnonymous (not verified)
#104 1763640-config-context-104.patch41.52 KBAnonymous (not verified)
#99 1763640-config-context-99.patch41.22 KBAnonymous (not verified)
#98 1763640-config-context-98.patch41.33 KBAnonymous (not verified)
#97 config_context-97.patch40.72 KBAnonymous (not verified)
#90 ExtendedContext-php.txt1.57 KBJose Reyero
#83 1763640-83.patch8.33 KBAnonymous (not verified)
#81 1763640-81.patch8.15 KBAnonymous (not verified)
#77 1763640-77.patch8.09 KBAnonymous (not verified)
#70 1763640-70.patch6.8 KBAnonymous (not verified)
#69 1763640-69.patch6.8 KBAnonymous (not verified)
#67 1763640-67.patch5.44 KBAnonymous (not verified)
#66 1763640-66.patch4.6 KBAnonymous (not verified)
#64 1763640-64.patch4.21 KBAnonymous (not verified)
#59 1763640-language-only-do-not-test.patch5.45 KBAnonymous (not verified)
#40 config_context-1763640-40.patch40.38 KBJose Reyero
#37 config_context-1763640-36.patch43.03 KBYesCT
#37 interdiff-34-36.txt18.59 KBYesCT
#34 config_context-1763640-34.patch41.51 KBYesCT
#33 config_context-1763640-33.patch41.49 KBdas-peter
#33 interdiff-1763640-32-33.txt2.66 KBdas-peter
#32 config_context-1763640-32.patch41.41 KBdas-peter
#32 interdiff-1763640-25-32.txt1.24 KBdas-peter
#27 CMI factory.png57.95 KBGábor Hojtsy
#25 config_context-1763640-25.patch40.48 KBJose Reyero
#25 config_context-1763640-25-diff.txt43.31 KBJose Reyero
#16 1763640-config_context-16.patch22.85 KBJose Reyero
#16 1763640-config_context-16-interdiff.txt5.71 KBJose Reyero
#14 1763640-config-context-14.patch18.82 KBgdd
#9 1763640-config-context-9.patch18.84 KBc31ck
#9 interdiff-4-9.txt8.73 KBc31ck
#8 1763640-config-context-8.patch18.35 KBc31ck
#8 interdiff-4-8.txt9.77 KBc31ck
#4 1763640-config_context-03.patch17.17 KBJose Reyero
#1 1763640-config_context-02.patch17.17 KBJose Reyero
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jose Reyero’s picture

First version of the patch that, to start with, should fix the issue of getting overridden values in system settings forms.

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-config

Put this on the sprint.

Gábor Hojtsy’s picture

Status: Active » Needs review
Jose Reyero’s picture

New version of the patch that fixes some typo, adding the right tag for Configuration sytem

Gábor Hojtsy’s picture

Priority: Normal » Major

Elevating to major as it is a cornerstone to have any CMI language support in core.

Lars Toomre’s picture

Here is a documentation review of the #4 patch for when it next gets re-rolled.

+++ b/core/includes/config.incundefined
@@ -69,6 +70,48 @@ function config($name) {
 /**
+ * Retrieves a configuration object for administration. This means that it has a special
+ * configuration context (admin = TRUE) and configuration plug-ins should mostly keep hands
+ * off this object and don't override it.

This docblock needs a one line description and this second paragraph should wrap at 80 characters or less.

+++ b/core/includes/config.incundefined
@@ -69,6 +70,48 @@ function config($name) {
+ *
+ * @param $name
+ *   The name of the configuration object to retrieve.

Can we add a type hint here... I presume it is string type?

+++ b/core/includes/config.incundefined
@@ -69,6 +70,48 @@ function config($name) {
+ * @param array $context
+ *   Array with contextual data to be used for the configuration factory.
+ * @param Drupal\Core\Config\StorageInterface $storage
+ *   The storage controller object to use for reading and writing
+ *   configuration data.
+ * @param Symfony\Component\EventDispatcher\EventDispatcher

Each of these @param directives need to start with '(optional) ' text and indicate what happens in default case.

Also after the @param directives please add @return directive with correct type hint type.

+++ b/core/includes/config.incundefined
@@ -69,6 +70,48 @@ function config($name) {
+ */
+function config_factory($context = array(), StorageInterface $storage = NULL, EventDispatcher $event_dispatcher = NULL) {
+  $storage = $storage ? $storage : drupal_container()->get('config.storage');

In the field patches, they refer to variables like $context as 'array $context = array()'. I think that should be done here.

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -9,6 +9,7 @@ namespace Drupal\Core\Config;
 
 use Drupal\Component\Utility\NestedArray;
 use Symfony\Component\EventDispatcher\EventDispatcher;
+use Drupal\Core\Config\ConfigFactory;

I think the standard is to place use statements in alphabetical order. Also I am wondering if 'use Drupal\Core\Config\StorageInterface' is missing. Not sure. Also is EventDispatcher needed any more?

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -72,13 +73,13 @@ class Config {
-   *   The event dispatcher used to notify subscribers.
+   * @param Drupal\Core\Config\ConfigFactory $factory
+   *   A configuration factory object that produces this config object.

This needs an '(optional) ' added to start of description as well as an explanation of what happens in default case.

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -371,9 +372,16 @@ class Config {
   /**
+   * Retrieve the configuration factory used to create this configuration object.
+   */

I believe that this docblock needs a @return directive with type hint for what $this->factory is.

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -371,9 +372,16 @@ class Config {
+  /**
    * Dispatch a config event.
    */

Missing a @param directive here.

+++ b/core/lib/Drupal/Core/Config/ConfigEvent.phpundefined
@@ -16,8 +16,9 @@ class ConfigEvent extends Event {
   /**
    * Constructor.

I am pretty sure arguments to a constructor need @param directives.

+++ b/core/lib/Drupal/Core/Config/ConfigEvent.phpundefined
@@ -26,4 +27,11 @@ class ConfigEvent extends Event {
+  /**
+   * Get configuration factory object.
+   */

This needs a @return directive.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -82,8 +90,58 @@ class ConfigFactory {
+  /**
+   * Get storage controller.
+   */

Needs a @return directive with type hint.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -82,8 +90,58 @@ class ConfigFactory {
+   * @param $key
+   *   A string that maps to a key within the context data or empty to get all data.

Needs a type hint and start description with '(optional)'.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -82,8 +90,58 @@ class ConfigFactory {
+   * @return

What type of data is returned here? Is it an array?

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -82,8 +90,58 @@ class ConfigFactory {
+   * @param $key
+   *   A string that maps to a key within the configuration context.
+   *
+   * @param $value
+   *   Any value to be set for this key.

Needs a string type hint for @param.

The @return directive has wrong type hint. I think it should be ConfigFactory as a type hint.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -82,8 +90,58 @@ class ConfigFactory {
+  /**
+   * Dispatch a config event.
+   *
+   * @param $config_event_name

Should be 'Dispatches'. Also need to type hint #config_event_name. I think it is a string.

+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.phpundefined
@@ -23,9 +23,12 @@ class ConfigGlobalOverrideSubscriber implements EventSubscriberInterface {
-      $config->setOverride($conf[$config->getName()]);
+    // Do not override configuration objects that are intended for administration.
+    if (!$event->getFactory()->getContext('config.admin')) {

Needs to wrap at 80 characters.

+++ b/core/modules/config/config.api.phpundefined
@@ -121,3 +121,25 @@ function hook_config_import_delete($name, $new_config, $old_config) {
+ * Delete configuration upon synchronizing configuration changes.

Should be 'Deletes'.

+++ b/core/modules/config/config.api.phpundefined
@@ -121,3 +121,25 @@ function hook_config_import_delete($name, $new_config, $old_config) {
+ * This callback is invoked when a new configuration factory is created
+ * and allows a module to take over the synchronization of configuration data.
+ *
+ * Modules should implement this callback if they manage configuration data
+ * (such as image styles, node types, or fields) which needs to be
+ * prepared and passed through module API functions to properly handle a

These comments need to be rewrapped better for 80 character limit.

+++ b/core/modules/config/config.api.phpundefined
@@ -121,3 +121,25 @@ function hook_config_import_delete($name, $new_config, $old_config) {
+}
\ No newline at end of file
diff --git a/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php

Missing newline at end of file.

+++ b/core/modules/locale/locale.moduleundefined
@@ -885,6 +886,20 @@ function _locale_rebuild_js($langcode = NULL) {
+}
\ No newline at end of file
diff --git a/core/modules/system/system.admin.inc b/core/modules/system/system.admin.inc

Another missing newline at end of file.

tstoeckler’s picture

+++ b/core/includes/config.inc
@@ -69,6 +70,48 @@ function config($name) {
+  $storage = $storage ? $storage : drupal_container()->get('config.storage');
+  $event_dispatcher = $event_dispatcher ? $event_dispatcher : drupal_container()->get('dispatcher');

This could use the cool new ?: pattern I think. I.e.
$storage = $storage ?: d_c()...;

Leaving at needs review.

c31ck’s picture

Patch for the documentation remarks from #6.

c31ck’s picture

Better patch and interdiff, disregard the patch in #8.

amontero’s picture

Status: Needs review » Needs work
@@ -371,9 +374,22 @@ class Config {
   }
 
   /**
-   * Dispatch a config event.
+   * Retrieves the configuration factory that produces this configuration object.
+   *
+   * @return Drupal\Core\Config\ConfigFactory
+   *   A configuration factory that produces this configuration object.
+   */
+  public function getFactory() {
+    return $this->factory;ction system_rss_feeds_settings_submit($form, &$form_state) {
-  config('system.rss')
+  config_admin('system.rss')
     ->set('channel.description', $form_state['values']['feed_description'])
     ->set('items.limit', $form_state['values']['feed_default_items'])
     ->set('items.view_mode', $form_state['values']['feed_item_length'])
@@ -2147,7 +2147,7 @@ function system_date_time_lookup() {
  * @see system_site_maintenance_mode_submit()
  */
 function system_site_maintenance_mode($form, &$form_state) {
-  $config = config('system.maintenance');
+  $config = config_admin('system.maintenance');
   $form['maintenance_mode'] = array(
     '#type' => 'checkbox',
     '#title' => t('Put site into maintenance mode'),
@@ -2169,7 +2169,7 @
+  }
+
+  /**
+   * Dispatches a config event.
+   *
+   * @param $config_event_name
+   *   Event name.
    */

Should not $config_event_name be declared as string in the docblock as it's done elsewhere?
Also, patch no longer applies to HEAD (7b5fbf3).

amontero’s picture

Ops, comment preview was saying I was removing "Configuration Management" tag (?). Undo.

Gábor Hojtsy’s picture

Any concerns from the CMI point of view? Any features missing or suggested to be better implemented?

Gábor Hojtsy’s picture

Title: Add contextual data into Configuration Factory for localization, administration, etc... » Introduce config factory and context to make config accessible in different languages

Hopefully easier to follow title.

gdd’s picture

Status: Needs work » Needs review
FileSize
18.82 KB
+++ b/core/includes/config.incundefined
@@ -68,6 +69,57 @@ function config($name) {
+  $storage = $storage ? $storage : drupal_container()->get('config.storage');
+  $event_dispatcher = $event_dispatcher ? $event_dispatcher : drupal_container()->get('dispatcher');

This pattern took me a few moments to parse, I'd much rather see if put into a full if (!empty()) statement. Also I believe this will throw a notice when $storage / $event_dispatcher is NULL.

I rerolled to HEAD.

Ultimately I don't have a lot of problems with this. The implementations look really straightforward. We will probably want to expand the docs for config_admin() a bit to expand on when you should and shouldn't use it. This is also going to cause a bunch of existing conversion patches to need rerolls, and we need to make sure and followup with them. I would like sun to take a look at it before we push it in if possible, but I don't want to hold it up forever either.

Gábor Hojtsy’s picture

Thanks for the review and support! :) More reviews are welcome of course :)

Jose Reyero’s picture

Rerolled the patch with some updates:
- Expanded documentation as suggested by @heyrocker #14
- Updated obsoleted function call in locale module (user_preferred_language -> user_preferred_langcode).
- Implemented configuration context for user mails to see how it looks like using this, for which I had to refactor the user_mail() function, dropping _user_mail_text(). Otherwise we need to create the config factory twice and anyway that function is not needed anymore as it is invoked only from user_mail().

gdd’s picture

Issue tags: +feature freeze

Tagging for feature freeze

vasi1186’s picture

I think this issue implements a very nice feature!

Here is some feedback from my point of view:

+++ b/core/lib/Drupal/Core/Config/ConfigEvent.phpundefined
@@ -14,10 +14,16 @@ class ConfigEvent extends Event {
+  public function __construct(Config $config, ConfigFactory $factory) {
     $this->config = $config;
+    $this->factory = $factory;

I didn't see the $factory declared in the class, like the $config is. I think we should also declare it as a class member.

Another thing is about the way how the ConfigFactory is used. I see in the get method of the ConfigFactory this call, where the Config class is instantiated:

$config = new Config($name, $this->storage, $this);

My question is: do we really need to pass the ConfigFactory object when constructing Config objects? I've also seen that the ConfigEvent is using now a ConfigFactory in order to be able to get later some context information, like here:

    if (!$event->getFactory()->getContext('config.admin')) {
      $config = $event->getConfig();
      if (isset($conf[$config->getName()])) {
        $config->setOverride($conf[$config->getName()]);
      }
    }

From my point of view, a Factory should have one single purpose: making the creation of objects easier by avoiding the use of "new". I think in this case, the ConfigFactory is used also for other things, ConfigFactory objects are sent as parameters, etc... Maybe I don't have yet the full picture of the system, but I guess we could somehow split this in having a true ConfigFactory that only creates Config objects, and the other data we could put it in a data container, or some context object, and possibly sent as a parameter somehow.

YesCT’s picture

I think this might be waiting for @heyrocker or someone with CMI experience to look at the new patch in #16

Would it make sense to keep this issue a little smaller and put some of the improvements from #16 in a separate/follow-up issue?

Gábor Hojtsy’s picture

I think the updates in #16 look good.

YesCT’s picture

YesCT’s picture

#16: 1763640-config_context-16.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +D8MI, +sprint, +language-config, +feature freeze

The last submitted patch, 1763640-config_context-16.patch, failed testing.

Jose Reyero’s picture

@vasi1186 #18

From my point of view, a Factory should have one single purpose: making the creation of objects easier by avoiding the use of "new". I think in this case, the ConfigFactory is used also for other things, ConfigFactory objects are sent as parameters, etc... Maybe I don't have yet the full picture of the system, but I guess we could somehow split this in having a true ConfigFactory that only creates Config objects, and the other data we could put it in a data container, or some context object, and possibly sent as a parameter somehow.

Yes, you are right.

What we are doing here is some oversimplification sticking the Context object into the Factory. Maybe we could fix it by different naming (Factory > Manager?), maybe having a Factory and a Context object (but so far I've failed bo implement this in a clean way, since it makes everything more complex as we need to be passing around two objects instead of one all the time)

I'm giving the Context/Factory option another try today but really, looking for some fresh ideas about this.

Jose Reyero’s picture

Reworked half to the patch to address the architectural issues pointed out in #18, so we have now:
- Configuration Context (ConfigContextInterface) being a class on its own (extending KeyValueStoreInterface)
- Context injected in the container, and other configuration parameters (storage, dispatcher) added as part of the context (these are the parameters we need to pass around to build the ConfigFactory and Config objects, so it makes sense to write them).
- config() taking an optional Context parameter so we can override the default context in the container for new config objects. This is used in user_mail() and also contexts are used in config_sync_changes()
- Modules can create classes extending ConfigContext to define new contexts of their own. See Drupal/user/UserConfigContext

This implies some API functional changes and new features:
- Configuration plugins (event listeners) are notified when new runtime contexts are created so they can add their stuff into the context. The event is 'config.context'.
- Config event listeners are notified now when we install a new configuration (which I think was a missing feature).
- Modules can override specific configuration objects at any time by adding them to the context. See GlobalContext (which takes $conf overrides by reference now) and ConfigOverrideSubscriber which will handle now any override and not only global $conf ones (that's why it was renamed from ConfigGlobalOverrideSubscriber).
The new API to get configuration for a user looks like this, see user_mail():

  $mail_config = config('user.mail', new UserConfigContext($params['account']));

Overall I think this provides some extremely flexible architecture for modules to alter or react upon configuration changes, it is truly extendable since modules can define their own ConfigContext objects, and also not that complex because Context groups all the parameters we need to build new configuration objects.

Next I will be updating the issue summary.

Jose Reyero’s picture

Status: Needs work » Needs review
Jose Reyero’s picture

Issue summary: View changes

Updated issue summary.

Jose Reyero’s picture

Issue summary: View changes

Updated for changes in #25

Jose Reyero’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

FileSize
57.95 KB

Created this class picture for the patch, please include in summary :)

CMI factory.png

Jose Reyero’s picture

Title: Introduce config factory and context to make config accessible in different languages » Introduce config context to make config consistent and accessible in different languages

Updated title and summary once again.

Jose Reyero’s picture

Issue summary: View changes

Updated issue summary.

Jose Reyero’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue tags: +Regression

Tagging as a regression, explained in the updated issue summary with the first para:

The configuration system can already store language specific override data. If locale module is not enabled, this data is not accessible (not a problem). If locale module is enabled, only the data localized to the page's interface language is accessible (which is the problem). No other language version is accessible on the page. This is a regression, because we cannot even send user emails in various languages on a page that is a basic feature of user and contact modules.

YesCT’s picture

Looks like next steps for this are

  • a review to check the api docs core gate (which I can do, but others are welcome)
  • to get a general code review (which I can attempt, others definitely welcome)
  • a review by someone knowledgable in CMI
  • someone to read the issue summary to see if everything will be clear when a committer comes to see this
  • a clarification if there are steps to test, or if this is just tested by the bot and the tests in the patch
das-peter’s picture

Status: Needs review » Needs work

Looks like this needs a re-roll. With the patch applied I get some fatal errors.
The patch can be applied to the latest 8.x but an installation fails because there's no user object to clone in user_template_preprocess_default_variables_alter().
And also if I apply the patch to a installed page I get the error Fatal error: Class 'Drupal\Core\EventSubscriber\ConfigGlobalOverrideSubscriber' not found in core/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 140

das-peter’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
41.41 KB

I've figured out the location that needed to be adjusted. Hope this is done correct.

das-peter’s picture

Fixed some minor coding standard thingies.

YesCT’s picture

does not apply. I followed instructions at: http://drupal.org/patch/reroll

I'm re-rolling.
conflict:

<<<<<<< HEAD
      if (module_exists($module) && module_hook($module, 'config_import_' . $op)) {
        $old_config = new Config($name, $target_storage);
=======
      if (module_hook($module, 'config_import_' . $op)) {
        $old_config = new Config($name, $target_context);
>>>>>>> applying patch from http://drupal.org/node/1763640#comment-6776496

I merged it as:

      if (module_exists($module) && module_hook($module, 'config_import_' . $op)) {
        $old_config = new Config($name, $target_context);

No interdiff since this is a re-roll.

Status: Needs review » Needs work

The last submitted patch, config_context-1763640-34.patch, failed testing.

YesCT’s picture

I'm rerolling this again ( for #1849004: One Service Container To Rule Them All I think ).

YesCT’s picture

Status: Needs work » Needs review
FileSize
18.59 KB
43.03 KB

Preface, This will have to be re-rolled. But I'm posting it now so I don't lose it.

Summary, this is docs fix, mostly for Definition of -> Contains, adding \ on name spaces, typing some @params, adding types to function call parameters, etc. There is one actual question about the code below.

--- a/core/includes/bootstrap.inc
+++ b/core/includes/bootstrap.incundefined

@@ -2226,6 +2226,7 @@ function _drupal_error_handler($error_level, $message, $filename, $line, $contex
+

unrelated accidental change.

+++ b/core/includes/config.incundefined
@@ -179,14 +192,16 @@ function config_sync_get_changes(StorageInterface $source_storage, StorageInterf
 function config_sync_changes(array $config_changes, StorageInterface $source_storage, StorageInterface $target_storage) {
+  $target_context = new AdminContext('config.target', $target_storage);
   foreach (array('delete', 'create', 'change') as $op) {
     foreach ($config_changes[$op] as $name) {
+      $config = new Config($name, $target_context);
       if ($op == 'delete') {
-        $target_storage->delete($name);
+        $config->delete();
       }
       else {
         $data = $source_storage->read($name);
-        $target_storage->write($name, $data);
+        $config->setData($data)->save();
       }
     }
   }
/**
 * Writes an array of config file changes from a source storage to a target storage.
 *
 * @param array $config_changes
 *   An array of changes to be written.
 * @param Drupal\Core\Config\StorageInterface $source_storage
 *   The storage to synchronize configuration from.
 * @param Drupal\Core\Config\StorageInterface $target_storage
 *   The storage to synchronize configuration to.
 */
function config_sync_changes(array $config_changes, StorageInterface $source_storage, StorageInterface $target_storage) {
  $target_context = new AdminContext('config.target', $target_storage);
  foreach (array('delete', 'create', 'change') as $op) {
    foreach ($config_changes[$op] as $name) {
      $config = new Config($name, $target_context);
      if ($op == 'delete') {
        $config->delete();
      }
      else {
        $data = $source_storage->read($name);
        $config->setData($data)->save();
      }
    }
  }
}

This is not a new function but since we are changing it, maybe we should fix the one line summary being over 80 chars. I changed it to
* Writes an array of config file changes from a source to a target storage.

Q

Here is my question though: In the function, why create a new one, in order to delete it?
If you think it is worth it, add a comment to make it clear.

+++ b/core/includes/install.core.incundefined
@@ -319,9 +319,11 @@ function install_begin_request(&$install_state) {
+    $container->register('config.context', 'Drupal\Core\Config\Context\ConfigContext')
       ->addArgument(new Reference('config.storage'))
       ->addArgument(new Reference('dispatcher'));
+    $container->register('config.factory', 'Drupal\Core\Config\ConfigFactory')
+      ->addArgument(new Reference('config.context'));

The factory has a reference to the context which has a reference to the storage. Hmm. OK.

+++ b/core/includes/install.core.incundefined
@@ -319,9 +319,11 @@ function install_begin_request(&$install_state) {
diff --git a/core/lib/Drupal/Core/Config/Config.php b/core/lib/Drupal/Core/Config/Config.php

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -385,21 +382,15 @@ public function delete() {
diff --git a/core/lib/Drupal/Core/Config/ConfigEvent.php b/core/lib/Drupal/Core/Config/ConfigEvent.php

+++ b/core/lib/Drupal/Core/Config/ConfigEvent.phpundefined
@@ -3,9 +3,9 @@
 use Symfony\Component\EventDispatcher\Event;
-use Drupal\Core\Config\Config;
 
 class ConfigEvent extends Event {
+

since we are already changing the use, adding the newline after the class, and adding whole methods. I added the missing @file and the newline at the end of the class too.

--- a/core/lib/Drupal/Core/Config/ConfigFactory.php
+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined

@@ -25,44 +25,29 @@
 class ConfigFactory {
 
   /**

major changes to this small file, so I'm going to fix the file comment too.

Status: Needs review » Needs work

The last submitted patch, config_context-1763640-36.patch, failed testing.

YesCT’s picture

I have to leave for a few hours. So just posting what I got so far... in case someone wants to jump in and re-roll it.

668 git checkout 8.x
669 git status
670 git pull --rebase
671 git reset --hard
674 git checkout -b check-36
675 git apply --index config_context-1763640-36.patch
679 git log
680 git checkout -b alittlebitago aa676a59dc6
681 git apply --index config_context-1763640-36.patch
682 git commit -m "Applying from config_context-1763640-36.patch"
683 git rebase 8.x
684 mvim core/includes/bootstrap.inc
685 history

function drupal_container(Container $new_container = NULL) {
  // We do not use drupal_static() here because we do not have a mechanism by
  // which to reinitialize the stored objects, so a drupal_static_reset() call
  // would leave Drupal in a nonfunctional state.
  static $container;
  if (isset($new_container)) {
    $container = $new_container;
  }
<<<<<<< HEAD
=======
  if (!isset($container)) {
    // This is only ever used by the installer and by run-tests.sh.
    // @todo Remove this entire section once these have been converted to use a
    //   kernel.
    $container = new ContainerBuilder();

    // Register active configuration storage.
    $container
      ->register('config.cachedstorage.storage', 'Drupal\Core\Config\FileStorage')
      ->addArgument(config_get_config_directory(CONFIG_ACTIVE_DIRECTORY));
    // @todo Replace this with a cache.factory service plus 'config' argument.
    $container
      ->register('cache.config', 'Drupal\Core\Cache\CacheBackendInterface')
      ->setFactoryClass('Drupal\Core\Cache\CacheFactory')
      ->setFactoryMethod('get')
      ->addArgument('config');

    $container
      ->register('config.storage', 'Drupal\Core\Config\CachedStorage')
      ->addArgument(new Reference('config.cachedstorage.storage'))
      ->addArgument(new Reference('cache.config'));

    // Register configuration object factory.
    $container->register('config.subscriber.globalconf', 'Drupal\Core\EventSubscriber\ConfigOverrideSubscriber');
    $container->register('dispatcher', 'Symfony\Component\EventDispatcher\EventDispatcher')
      ->addMethodCall('addSubscriber', array(new Reference('config.subscriber.globalconf')));
    $container->register('config.context', 'Drupal\Core\Config\Context\GlobalContext')
      ->addArgument(new Reference('config.storage'))
      ->addArgument(new Reference('dispatcher'));
    $container->register('config.factory', 'Drupal\Core\Config\ConfigFactory')
      ->addArgument(new Reference('config.context'));

    // Register staging configuration storage.
    $container
      ->register('config.storage.staging', 'Drupal\Core\Config\FileStorage')
      ->addArgument(config_get_config_directory(CONFIG_STAGING_DIRECTORY));

    // Register the service for the default database connection.
    $container->register('database', 'Drupal\Core\Database\Connection')
      ->setFactoryClass('Drupal\Core\Database\Database')
      ->setFactoryMethod('getConnection')
      ->addArgument('default');
    // Register the KeyValueStore factory.
    $container
      ->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
      ->addArgument(new Reference('service_container'));
    $container
      ->register('keyvalue.database', 'Drupal\Core\KeyValueStore\KeyValueDatabaseFactory')
      ->addArgument(new Reference('database'));

    $container->register('path.alias_manager', 'Drupal\Core\Path\AliasManager')
      ->addArgument(new Reference('database'))
      ->addArgument(new Reference('keyvalue'));

    // Register the EntityManager.
    $container->register('plugin.manager.entity', 'Drupal\Core\Entity\EntityManager');
  }
>>>>>>> Applying from config_context-1763640-36.patch
  return $container;
}
Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
40.38 KB

Rerolled the patch against latest head.

I cannot provide a clean interdiff since I've messed up the order of the patches, however this is only an update from #33 to the latest changes in drupal_container(), as the stuff there has been moved to CoreBundle.

YesCT’s picture

OK. I think we need to see if the patch from 36/37 applies to that re-roll.

YesCT’s picture

Or, decide that those changes are totally ok for a follow-up, which I'm ok with.

Jose Reyero’s picture

@YesCT, sorry I was in a rush this morning and missed that ones.
That will be in next reroll if we need one more.

gdd’s picture

+++ b/core/includes/config.incundefined
@@ -179,14 +192,16 @@ function config_sync_get_changes(StorageInterface $source_storage, StorageInterf
 function config_sync_changes(array $config_changes, StorageInterface $source_storage, StorageInterface $target_storage) {
+  $target_context = new AdminContext('config.target', $target_storage);
   foreach (array('delete', 'create', 'change') as $op) {
     foreach ($config_changes[$op] as $name) {
+      $config = new Config($name, $target_context);
       if ($op == 'delete') {
-        $target_storage->delete($name);
+        $config->delete();
       }
       else {
         $data = $source_storage->read($name);
-        $target_storage->write($name, $data);
+        $config->setData($data)->save();
       }
     }
   }

Is a context really necessary here? I don't see why we need it, we are writing directly to the storage engine and don't need to worry about overrides taking over right?

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -51,34 +50,32 @@ class Config {
-  public function __construct($name, StorageInterface $storage, EventDispatcher $event_dispatcher = NULL) {
+  public function __construct($name, ContextInterface $context) {

I'm not a huge fan of the fact that we now need to have a context if we just want to access a different storage for a Config object. I understand it architecturally, but as a dev it would be nice not to have to deal with that extra layer if I don't have to. Not sure there's a good answer here.

+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.phpundefined
@@ -0,0 +1,40 @@
+    parent::__construct($storage ?: drupal_container()->get('config.storage'), drupal_container()->get('dispatcher'));

Can we put the setting of $storage into its own line? Having the ternary in the function parameter like this is really confusing. There's at least one more of these in the patch it would be nice to see changed.

+++ b/core/modules/config/config.api.phpundefined
@@ -124,3 +124,25 @@ function hook_config_import_delete($name, $new_config, $old_config) {
+/**
+ * Deletes configuration upon synchronizing configuration changes.
+ *
+ * This callback is invoked when a new configuration factory is created and
+ * allows a module to take over the synchronization of configuration data.
+ *
+ * Modules should implement this callback if they manage configuration data
+ * (such as image styles, node types, or fields) which needs to be prepared and
+ * passed through module API functions to properly handle a configuration
+ * change.
+ *
+ * @param Drupal\Core\Config\ConfigFactory $factory
+ *   A configuration object containing the new configuration data.
+ */
+function hook_config_factory($factory) {
+  if (!$factory->getContext('locale.language')) {
+    // Add user's language when in user's context.
+    if ($account = $factory->getContext('user.account')) {
+      $factory->setContext('locale.language', user_preferred_language($account));
+    }
+  }
+}

This seems to mostly apply to ConfigEntity type of config (like image styles as mentioned) but it doesn't seem to be implemented anywhere? I don't really get this.

+++ b/core/modules/system/system.admin.incundefined
@@ -1992,7 +1992,7 @@ function system_site_maintenance_mode($form, &$form_state) {
-  config('system.maintenance')
+  system_config('system.maintenance')

We are going to have to be really careful when reviewing patches to make sure system_config() is used properly, as well as making it really clear to module/patch authors when it should and shouldn't be used. This could lead to some really ugly bugs.

I'd still like to see at least one of the other CMI maintainers review this before it goes in. I know sun has been pinged several times, I'll try and see if I can get alex on it.

Jose Reyero’s picture

About context in config_sync_changes:
@heyrocker,

Is a context really necessary here? I don't see why we need it, we are writing directly to the storage engine and don't need to worry about overrides taking over right?

I think any module dealing with config overrides will want to know when the configuration has changed (actually for the locale module we need to know to keep translations up to date).
So since there's no generic config hooks for that I figured out this is also a good usage of context.
One more thing plugins may want to know is whether the administration is updating the configuration or it is the result of some install/update process, and that's also information 'context' can provide.
So let's say we are killing a few birds with one stone :-)

About Config class constructor:

I'm not a huge fan of the fact that we now need to have a context if we just want to access a different storage for a Config object. I understand it architecturally, but as a dev it would be nice not to have to deal with that extra layer if I don't have to. Not sure there's a good answer here.

I don't think this will make it more complex (than passing separate storage and even dispatcher) and once again, we'll be able to address another -and I think this one is important- architectural issue, which is having a factory for config objects but still needing to create some objects outside of our factory (which is still possible anyway, just not needed anymore).
Looking it from a different perspective, config listeners need to know 'context' to act upon config objects. So this is also a 'feature completion' issue, either they have some context always or they don't.

Actually I'm thinking in next steps -but these ones better for cleanup/consistency phase I din't want to make it more complex here- we may be able to replace half of the code in config.inc by proper usage of different Contexts and Storages which may work to encapsulate all this code in classes.

Code style, Admin Context constructor.

Can we put the setting of $storage into its own line? Having the ternary in the function parameter like this is really confusing. There's at least one more of these in the patch it would be nice to see changed.

Ok.

About hook_context_factory().

This seems to mostly apply to ConfigEntity type of config (like image styles as mentioned) but it doesn't seem to be implemented anywhere? I don't really get this.

Oops, this is a leftover, need to clean this up.
We don't need it -nor any other hook- anymore since config listeners can now act upon 'config.context' event.

- config('system.maintenance')
+ system_config('system.maintenance')
We are going to have to be really careful when reviewing patches to make sure system_config() is used properly, as well as making it really clear to module/patch authors when it should and shouldn't be used. This could lead to some really ugly bugs.

I think this is a handy shorthand they will be happy to use, but yes, I agree usage must be clear. Or maybe a better naming. I just thought this one makes sense since configuration updates are kind of handled by system module (system_settings_form)
I'll review the docs again, but let me know if you have any specific suggestion.

Really, in my ideal world, trying to update a config object retrieved for a regular page context (not admin, nor update, nor sync) would trigger an exception.
And this btw, knowing beforehand which config is to be updated, which one is RO could allow us some nice performance optimizations because most of the time we dont' really need a RW storage controller.
-- I think this could be implemented later as a DX improvement.

I'd still like to see at least one of the other CMI maintainers review this before it goes in. I know sun has been pinged several times, I'll try and see if I can get alex on it.

Yes, me too, that would be helpful.

(Waiting on whether you think all this makes sense, and hopefully someone else can review this, before doing a reroll)

Jose Reyero’s picture

@heyrocker,
Just to clarify, about using 'Context' in config_sync_changes(), I thought that would be some example usage of it, but if you feel it's not the right place to use, I'll just drop that part, which is not the goal of this patch anyway.

Other simplifications, that wouldn't harm the 'essence' of this patch may include getting rid of that Install/Update contexts and just run with AdminContext/GlobalContext/UserContext.

Anonymous’s picture

my first reaction to this patch is - are we sure this is necessary?

The configuration system can already store language specific override data. If locale module is not enabled, this data is not accessible (not a problem). If locale module is enabled, only the data localized to the page's interface language is accessible (which is the problem). No other language version is accessible on the page. This is a regression, because we cannot even send user emails in various languages on a page that is a basic feature of user and contact modules.

doesn't something like the below function to load configuration for an arbitrary language cover this use case?

function locale_config($name, Language $language) {
  $locale_name = 'locale.config.' . $language->langcode . '.' . $name;
  $config = config($name);
  if ($override = $config->getStorage()->read($locale_name)) {
    $config->setOverride($override);
  }
  return $config;
}

don't the existing load events allow modules to do whatever they like to config values at runtime:

class LulzOverrideSubscriber implements EventSubscriberInterface {
  public function configLoad(ConfigEvent $event) {
    $config = $event->getConfig();
    if ($config->getName() == 'system.site') {
      $config->setOverride(array('name' => 'lulz ' . uniqid()));
    } 
  }
  static function getSubscribedEvents() {
    $events['config.load'][] = array('configLoad', 30);
    return $events;
  } 
}

as for editing the values in .yml without overrides, how about we add a new method to Config, like ->getTheRealDataFromYmlPlz() ? naming aside, if the requirement boils down to 'get me the data from .yml', why don't we just do that, rather than add all the things?

Gábor Hojtsy’s picture

@beejeebus: that is a very good question. First of all the configuration overrides system was designed to be able to handle override data of any kind. Drupal 7 and before have at least two prominent and widely used modules that need overrides: domain module and organic groups. We can introduce a simple helper function to just consider the language overrides but obviously core will never consider the domain or organic group overrides, although domain module and organic groups are routinely used to override core settings. So the underlying thinking in this patch is that the calling code does not really know which overrides will take effect so it provides more general "context" information which is used in the overrides system to figure out if language/domain/group/etc. overrides apply.

don't the existing load events allow modules to do whatever they like to config values at runtime

Yes, well, imagine contact module. If its a user's contact form, the module sends up to two emails in different languages, one in the recipients language and one in the sender's language. Similarly are importing users to the site and need to send emails to all the new users imported, you are sending out tens or hundreds of emails all in various languages. It is not really possible to set up these overrides based on one global setup for the page, we need to change the overrides applied based on the "contextual information" available.

Of course we personally care primarily for language, so our examples and the patch updates are mainly evolving around that. Unfortunately we have not been very successful in recruiting reviewers from groups and/or domain module or any other module needing specific overrides in different contexts.

Anonymous’s picture

Gábor - thanks for the reasonable reply.

We can introduce a simple helper function to just consider the language overrides but obviously core will never consider the domain or organic group overrides, although domain module and organic groups are routinely used to override core settings. So the underlying thinking in this patch is that the calling code does not really know which overrides will take effect so it provides more general "context" information which is used in the overrides system to figure out if language/domain/group/etc. overrides apply.

i'm not sure i get this. are you saying that the bulk of calls to config() won't provide a $context param? in which case domain/OG will act globally on events for given config objects - just like they can now?

or that most calling code will know about and provide a non-default context, but a 'generic' one? or ... ?

feels a bit unclear - is calling code generally expected to provide context information?

Yes, well, imagine contact module. If its a user's contact form, the module sends up to two emails in different languages, one in the recipients language and one in the sender's language. Similarly are importing users to the site and need to send emails to all the new users imported, you are sending out tens or hundreds of emails all in various languages. It is not really possible to set up these overrides based on one global setup for the page, we need to change the overrides applied based on the "contextual information" available.

to be able to send an email to a user (or list of users) in different languages, you need the users and their languages. so it's possible, with a 9 line helper function, to do this without a single change to the config system.

i get that we're trying to make something more powerful than that, i guess i just don't think this complexity is worth it at this point.

one of the main reasons we put the global $conf override in was because of domain module (i remember speaking to agentrickard about this very thing at denver).

as long as the core CMI code doesn't get in the way of more powerful functionality in contrib, i think we should err on the side of simple for D8, and bring things that work from contrib in to core in D9.

Gábor Hojtsy’s picture

i'm not sure i get this. are you saying that the bulk of calls to config() won't provide a $context param? in which case domain/OG will act globally on events for given config objects - just like they can now?

We considered areas where we expect language to be variant in this patch. I did not look at other areas and what kind of contexts would be needed there. I can easily imagine for organic groups that not the 100% of the page belongs to the group, there are navigation elements, lists of groups, etc. where one would need to switch group contexts for accuracy.

to be able to send an email to a user (or list of users) in different languages, you need the users and their languages. so it's possible, with a 9 line helper function, to do this without a single change to the config system.

Problem is we need the "context" to trickle down. Eg. when you send an email, you need the email template in a specific language, then the email template has tokens, which need to pull the values in that language (eg. they refer to a node title, then we need to load that node and take the title in the language of the context needed or a simpler one that is often used in email tokens, the site name would need to be taken in the context). Without passing around the config instance or being able to set a temporary context override for the config how do you imagine the context (such as language) trickling down?

The problem we are trying to solve here is that lots of parts of config could vary with different contexts (such as language). In the same http request, we'd need the same config with different contexts, and we don't want to pepper views, contact, rules, etc. with language conditions. You are suggesting our best bet is to add language specific code only to all the modules and ignore variance of other contexts (domain, groups) and/or their conflict resolution with language contexts?

Gábor Hojtsy’s picture

Consider another "simple" example, field configuration. Nodes are translated via their fields, but if a field is not translatable, the display of the field can still be translated. Eg. a size field for a product would be "Small", "Large", "Huge". The size of the product the node represents is not language dependent, but the textual print-out of the size is. So if the node is displayed in its Hungarian translation that would need to size "Méret" for Size and "Kicsi", "Nagy" and "Óriás" respectively for the sizes. So that is field configuration translation. When the node creation form is displayed, the right language version should appear, that is probably simplest, because it is likely a global context for the page. When the field config is edited, you usually want to avoid overrides, and edit the original values (otherwise translators would have permission to change allowed values, etc). When the node is displayed, the field config needs to be used in the node's language. When you have a view displaying nodes in different languages, then you need to use the same field config in various languages on the same page. So the language context trickles down from the node to the field display, etc.

I think the problem with using "9 line helper function" type of things you mention specially for language when it is needed, is that it also says you'll never write reusable API functions that are used in various "contexts" (but not take a "context" as their argument). If they do take a context, then we need to introduce this context on some level anyway (even if only language), and then every method which deals with saving, loading, displaying, rendering, etc. configuration would need to include arguments for language (and/or other contexts which might vary the configuration). The idea behind using a factory to get a config object specific to the context is similar to the new field API approach where can get a translation of an entity and then treat it as the original entity with the same methods. You can send that instance along and it will behave like the original but focused to the "context" you requested it to be in.

Anonymous’s picture

When the node is displayed, the field config needs to be used in the node's language. When you have a view displaying nodes in different languages, then you need to use the same field config in various languages on the same page. So the language context trickles down from the node to the field display, etc.

ok, so this looks like a tricky problem. i guess i'm just missing how this patch solves that? the context is only scoped per-config object, so trickle down can only mean passing down config objects with the right context set? isn't that what you are telling me we don't want, and this system is built to avoid?

this example:

+ $mail_config = config('user.mail', new UserConfigContext($params['account']));

the context (and any overrides based off that) is only valid to that single config object?

sorry if these are dumb questions.

Gábor Hojtsy’s picture

@beejeebus: yes, this is an "infrastructure" issue where we want to lay down the foundations to allow for this functionality, like how CMI was introduced and conversions are still happening. All API functions that would be reused multiple times on a page and/or for administration or display (so basically lot of code) will either need to take the config object or will need to take a context passed around. It is pretty important to get a solution for this problem in sooner than later (even if the solution is not what is proposed) so we can actually work on converting code. Thanks for your reviews and continued discussion, highly appreciated :)

Anonymous’s picture

ok, thinking on it. i suspect i'm not going to come up with a better answer, but i'll try.

@Gábor - thanks for helping me understand.

catch’s picture

@Gabor, please explain why domain access and organic groups can't make do with the global overrides system that already exists?

The domain is global (certainly much more global than language is), and organic group context is too.

Gábor Hojtsy’s picture

@catch: the global context assumption with domain and groups both assume there is never a case where different content/functionality would need to be displayed on the same page from varying domains/groups applying the group/domain specific overrides. If we intend to keep those limitations for Drupal 8 and we don't assume there is any other context-dependent override system, which would need this backend, then we can limit our thinking to languages only.

catch’s picture

@catch: the global context assumption with domain and groups both assume there is never a case where different content/functionality would need to be displayed on the same page from varying domains/groups applying the group/domain specific overrides.

That limitation seems OK to me. I'd also be surprised if there's not a way to force local overrides if you really need it (i.e. like swapping out $conf temporarily then forcing a config load by clearing static caches). Would be good to get feedback from agentrickard and/or Amitaibu here.

Jose Reyero’s picture

@catch,

The first case for context is the one in Drupal core, where e-mails need to be sent to different users using different user preferences. Language in this case, but there may be others in the future like country, group, role, etc.. (providing there's some contrib module switching settings for that contexts).

So what we are doing here is first abstracting the idea of 'switchable contexts' (1) so we don't make any assumption on what's going to take to build the right context for a user. Locale module may need to know about users, but User module doesn't need to know about language. This is how we think will work for different arbitrary contexts.

Now for groups:
Provided we may have different settings (config overrides) for different groups, there are multiple cases in which you may want more than one in the same page request, two examples:
• Cron execution sending out mails to different groups.
• A post intended for two or more groups may trigger notifications for these 2 different groups that may need different language, country settings, urls, etc...

So the second step in this patch is, once we have identified some use cases for it, declaring an explicit context for whatever runs with different configuration to the main page request. (2)

Right now the only option we have is modules making fuzzy decisions about global overrides based o the current path and this what could be fixed by this.

On one side, modules overriding configuration based on context don't need to rely on globals anymore, as they have an explicit context they can rely upon. And on the other side context switching has start and end points so we won't end up with leftover global variables/overrides anymore.

On top of this, as the global Context is an object in DIC, we still can switch the global context if there's a good reason for it. As I see it, global $conf overrides are still handy for manually overriding configuration from settings.php but any other use of it has always been an ugly hack we should try to stay away from.

Anonymous’s picture

re #56 and #58:

doesn't this mean that all code that could vary from the global context for domains/groups *needs to have an explicit context passed to all config() calls*? isn't that like 90% of drupal? i just don't see how this is going to work without explicit setting and passing around of correct context/config objects, because lots and lots and lots of code calls in to other code that relies on config() calls.

the patch allows contrib modules to react to config() context in completely arbitrary ways. so depending on your enabled modules, the 'do i need to care about this config() call varying by context' question will change. which is to say i have NFI how core is supposed to get this right other than to propagate the correct context to all config() calls.

i propose a simpler, hard-boiled language support solution for config.

attached patch gives an idea of what i'm talking about. there's a tricky issue with the DIC which is not introduced here with regard to language and request scope, so i've just added a mythical 'language_service' to CoreBundle::build(). (we can't rely on language until we have a request...)

the change that matters for this issue is:

diff --git a/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
index 3d2fd4a..4ae656d 100644
--- a/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
@@ -26,7 +26,7 @@ class LocaleConfigSubscriber implements EventSubscriberInterface {
    */
   public function configLoad(ConfigEvent $event) {
     $config = $event->getConfig();
-    $language = language(LANGUAGE_TYPE_INTERFACE);
+    $language = $config->getLanguage();
     $locale_name = $this->getLocaleConfigName($config->getName(), $language);
     if ($override = $config->getStorage()->read($locale_name)) {
       $config->setOverride($override);

so if we set language on the ConfigFactory in the container, and allow code to change that during the request, then all of the 'run with the user's language until i say stop' functionality is doable.

Gábor Hojtsy’s picture

All right, so I understand several of the reviewers on this issue state that domain or group contexts should stay page-global and it should not be possible to for example send notifications to people with different notification settings in different groups in the same HTTP request by design. Ok. That assumption clearly simplifies the functionality/flexibility offered and therefore greatly simplify the DX of the solution.

@beejeebus: You don't actually set this language here anywhere, so how do you imagine that. A global page language scope would be established somehow and then if some place wants to use a different language, it would do:

$old_language = ... // get old language
$config->setLanguage($new_language);
// code to deal with config
$config->setLanguage($old_language);

Is that the imagined code flow for temporarily overriding language? So then any APIs used, hooks invoked, etc. under "code to deal with config" would use the temporarily set language, and if it would itself want to change the language, that could also only be temporary until it comes back here and the code sets it back.

That is the proposed API?

Anonymous’s picture

@Gábor - yes, that's exactly it. basically, set the language at the factory level for config when you know you don't want the page-level language.

i don't think we'd be able to cover all cases where we'd want that functionality via explicit passing of a language/context, because 'do something in this language' may call in to code that is many layers deep.

Gábor Hojtsy’s picture

Yeah, ok, all right. I personally believe we 100% need the language variance possibility solved (otherwise it is a definite regression bug) and if there is no support for making other variances work and if it is exponentially worse DX then I'm fine with a much simpler language-only solution. We at least tried to be more general but it did not fly with most reviewers. Such is life.

Anonymous’s picture

ok, i'll work on making the patch in #59 actually work, and port the example user_mail() implementation to it. will no doubt need some help from some DIC experts, because the language / config dependencies at early bootstrap are tricky.

Anonymous’s picture

FileSize
4.21 KB

here's an update on #59 that gets us a bit closer. i'm getting "PHP Fatal error: Uncaught exception 'Symfony\\Component\\DependencyInjection\\Exception\\ServiceCircularReferenceException' with message 'Circular reference detected for service "config.factory", path: "config.factory".'" errors, even though i'm not touching CoreBundle, instead using language_default() and a TODO.

definitely need some help from DIC gurus at this point.

Status: Needs review » Needs work

The last submitted patch, 1763640-64.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
4.6 KB

attached patch gets past the circular reference error (seems i have to make sure to make Language optional), but dies somewhere in the installer.

posting in case anyone else can figure it out before i do.

Anonymous’s picture

FileSize
5.44 KB

ok, so that was easy. just needed to handle the 'new Config()' call sites in config.inc.

attached patch gets through install and 'click around a bit' test.

Status: Needs review » Needs work

The last submitted patch, 1763640-67.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
6.8 KB

ok, here's a patch that set's the user's language on the ConfigFactory in user_mail().

Anonymous’s picture

FileSize
6.8 KB

ok, typo in Config object, should be getLanguage() not getLanuage(), that should fix at least the locale config override test.

Status: Needs review » Needs work

The last submitted patch, 1763640-70.patch, failed testing.

katbailey’s picture

Somewhat related - I just created a fairly hand-wavy issue about OOPifying the language system: #1862202: Objectify the language system. It's been on my mind to look into doing this since the bundles patch went in (which is when the request-dependent language_manager service came into being) but it's a daunting task, to say the least ;-)

Jose Reyero’s picture

@Gábor Hojtsy,
I don't really see here a rejection to the idea of configuration context, but more a general "idea looks ok, implementation may be too complex.". So I would take it as a call for simplification more than any other thing.

@beejeebus,
As I see it your patch takes a fully different approach to the"translated configuration" issue and also it doesn't address at all the issue of inconsistent values in admin forms.
I think we better open a new issue for the new patch, which does something very specific (translated configuration), name it properly, etc..
And btw, keep an eye on (upcoming, maybe) config caching, that may clash with all of these, #1187726: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects)

In the meanwhile I may give a try here to a simplified version of the original "configuration context" patch.

sun’s picture

The one-off context arguments to config() do not look really feasible to me. As others mentioned already, the end result would essentially be that we'd have to specify a context for every single call to config() (or pass a config context object forward through the entire system).

I wonder whether we could use a similar approach as the DI Container's Scopes for context here?

(We can't use it directly, since it works differently, but the idea and pattern of persistent, nested scopes that can be entered and left could be borrowed.)

Essentially, you add and enter a scope, and all subsequent operations are within the scope, until you leave the scope. Scopes can be added and stacked. A new value for an existing scope overrides that scope (but does not leave the existing).

$config = drupal_container()->get('config');

# Init.
$config->enterScope('language', language_default());

# Contrib init.
$config->enterScope('group', og_get_group());
$config->enterScope('domain', drupal_container()->get('domain'));

# Send a mail to multiple users.
foreach ($recipients as $account) {

  $config->enterScope('language', $account->preferred_language);

  drupal_mail('user_notify', 'account_expired', $account);

  $config->leaveScope('language'); // This only leaves the last 'language' scope in the stack, not all.
}
Gábor Hojtsy’s picture

@sun/@beejeebus: both of you are missing a mechanism to tell set up a scope/context for config to ignore all overrides, so the original values are accessible in some way (to edit them, to export them, etc). @sun: also your scopes look pretty similar to Jose's contexts with the exception is that Jose's use instances of typed objects and you use a string key with an arbitrary value.

Anonymous’s picture

re. #75, that problem predates context, and deserves it's own issue IMO.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.09 KB

here's a patch that fixes the last remaining fail.

the diff from #70 is that if we find a language via language_negotiation_method_invoke(), we set it on the config factory in the container as the default.

we need this because the locale config override no longer does the dirty with global scope to get the language.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/includes/config.incundefined
@@ -257,11 +257,11 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
-        $old_config = new Config($name, $target_storage);
+        $old_config = new Config($name, $target_storage, language_default());
...
-        $new_config = new Config($name, $target_storage);
+        $new_config = new Config($name, $target_storage, language_default());

What is the role of these language_default()s here?

+++ b/core/includes/language.incundefined
+++ b/core/includes/language.incundefined
@@ -466,11 +466,23 @@ function language_negotiation_method_invoke($method_id, $method = NULL, $request

@@ -466,11 +466,23 @@ function language_negotiation_method_invoke($method_id, $method = NULL, $request
+    // Set the negotiated language as the default for Config objects created by the
+    // ConfigFactory.
+    drupal_container()
+      ->get('config.factory')
+      ->setLanguage($language);
+    return $language;

Why is this set inside an obscure API function like language_negotiation_method_invoke()? Why not where the results are actually used (and set with the DIC for the locale system as well). This seems like a pretty obscure place to do this.

Also, even as the above comments say, there are multiple types of languages negotiated (in core there are three), and not all of them are necessarily negotiated to the same language. So what this code ends up doing is it sets the config language to whichever the last negotiated language was (which is probably not the same as the interface language that had the code that you removed down below).

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -46,10 +47,15 @@ class ConfigFactory {
+    // TODO: always inject language, once we figure out how to handle language
+    // and the DIC in early bootstrap phases.

TODO should be @todo right?

+++ b/core/modules/user/user.moduleundefined
@@ -1989,8 +1989,28 @@ function user_view_multiple($accounts, $view_mode = 'full', $langcode = NULL) {
+
+  $config_language = drupal_container()
+    ->get('config.factory')
+    ->getLanguage();
+  $user_langcode = user_preferred_langcode($params['account']);
+
+  if ($user_langcode != $config_language->langcode) {
+    // Make sure that we get configuration values for the user's language, not
+    // the site default language.
+    drupal_container()
+      ->get('config.factory')
+      ->setLanguage(language_load($user_langcode));
+  }
+
...
+
+  if ($user_langcode != $config_language->langcode) {
+    drupal_container()
+      ->get('config.factory')
+      ->setLanguage($config_language);
+  }
 }

I find it pretty puzzling that this would be the "simple" API where anybody who would need to switch language would need to copy-paste this 20 lines of boilerplate, get the old language, compare to the new one, set the new one, set it back, etc. This is lots of code to copy-paste all the time IMHO.

sun’s picture

I still think that the primary problem space we need to solve is the persistence of contexts (through scope stacks or similar) as mentioned in #74.

@beejeebus' alternative approach essentially shares the same architectural problem as @Jose's approach — supplying a particular context for every single config() call just simply won't work out, since Drupal is modular, which inherently means that the context has to be passed forward to any possible subsequently invoked code (of which there can be a lot).

The idea of stacked scopes essentially follows the architectural design of Symfony's HttpKernel and its notion of sub-requests. Essentially, we're entering a scope, and within that scope, certain contextual parameters apply, and they continue to apply until we leave the scope. Upon leaving a scope, the parent scope (if any) is re-entered.

Scopes can be leveraged not only by the language subsystem, but by everything that might involve contextual overrides.

re: #75: Administrative/original context:
I don't see a problem at all there — it's just another scope layer that is added to the existing stack before the form is built and processed, and the scope is left as soon as we want to leave it. It is certainly a special scope, for which we might have to provide a separate method à la ::enterOriginalScope(), which essentially loops over the entire stack of existing scopes and resets all contexts to null/zero. It wouldn't have a corresponding leave method though, since the added scope is merely another scope in the stack, which can be left as any other scope.
Furthermore, entering that scope could be completely optional and wouldn't necessarily have to be enforced for all administrative tasks; instead, each block could simply control on its own whether the user wants to enter the original scope or not. (Doing so could potentially be controlled through an ajaxified scope switcher widget in the UI.)

Jose Reyero’s picture

About @beejeebus's patch #77,
I agree with @Gábor it is not a workable / reusable approach. The patch really contains in itself the proof of how complex it is to use such language, it's not generic enough for allowing any other kind of 'context', and maybe the biggest concern is that every module using it needs to handle the language itself (as opposed to language being handled in a single module, locale).

I agree with @sun about the need of scopes and stacks here (#74, #79) though I think the original patch (#40) is a good base for doing it because it allows switching the 'default' context (that is into DIC) and also the shorthand function with a context parameter can work on that model if we think of it as 'switch context/get config object/switch back context'

My plan is to work on #40 to a) address @sun's concerns implementing his idea of stacked scopes, and b) simplify it as much as possible and decouple it from event listeners, storage, etc so it is just about 'adding' context.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.15 KB

attached patch tries to address the review in #78

- introduce config_factory_set_language() so callers don't need to do so much boilerplate

- move the language initialisation on the config factory to PathSubscriber::onKernelRequestLanguageResolve (doesn't feel right, open to suggestions how to make it better, but kind of thinking fixing it for real lives in #1862202: Objectify the language system)

- fix the TODO

- the language_default() in config.inc are because we don't use the factory there, so we need to pass a Language object ourselves. perhaps i should comment?

Status: Needs review » Needs work

The last submitted patch, 1763640-81.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.33 KB

doh! attached patch tells php about the Language param config_factory_set_language().

Gábor Hojtsy’s picture

+++ b/core/includes/config.incundefined
@@ -257,11 +273,11 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
-        $old_config = new Config($name, $target_storage);
+        $old_config = new Config($name, $target_storage, language_default());
         $old_config->load();
 
         $data = $source_storage->read($name);
-        $new_config = new Config($name, $target_storage);
+        $new_config = new Config($name, $target_storage, language_default());

I think comments here would be good. These are the only places where this applies?

Is it true that we want language overrides in the import case? Sounds dubious to me.

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.phpundefined
@@ -88,6 +88,15 @@ public function onKernelRequestLanguageResolve(GetResponseEvent $event) {
+    // Set the negotiated language as the default for Config objects created by
+    // the ConfigFactory.
+    $language = drupal_container()
+      ->get('language_manager')
+      ->getLanguage(LANGUAGE_TYPE_INTERFACE);
+    drupal_container()
+      ->get('config.factory')
+      ->setLanguage($language);

Is this too early to use the config_factory_set_language() API or you wanted to avoid it in the path subscriber? Documenting why would be good. Otherwise it looks like code duplication.

+++ b/core/modules/user/user.moduleundefined
@@ -1893,8 +1893,16 @@ function user_view_multiple($accounts, $view_mode = 'full', $langcode = NULL) {
+
+  // Make sure that we get configuration values for the user's language, not
+  // the site default language.
+  $config_language = config_factory_set_language(language_load(user_preferred_langcode($params['account'])));
+
...
+
+  // Set the language back to the site default.
+  config_factory_set_language($config_language);

I don't think this is going to to universally work. The user preferred language is already computed one level up (eg. in http://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fun...), so computing it again here is duplicated work.

Also, that code in the caller also deals with configuration which is not yet considered in this language context even though it applies to this email.

This looks like doing it at a place that is too low level and after several factors for the email were already decided.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Gábor Hojtsy’s picture

@beejeebus opened a separate issue for the "admin context" aka raw config data accessibility at #1868028: Raw (original) config data is not accessible.

Anonymous’s picture

ok, i'm just holding this up now, and i don't see much support for the idea of handling language only.

lets ignore my patches, and get back to the more powerful, more complex system developed by jose.

the only issue i have with #40 is that it still sets context per config object. that design requires rewriting all code that wants to start an operation with any nested config() calls to explicitly pass down either the context or the config object. i don't think that's viable, so i suggest we try to make something work which sets the context on the config factory, and injects that into config objects.

that is, basically what my patches do with $language, but allowing for arbitrary overrides by modules on the injected $context object.

sound viable?

chx’s picture

OK let's get back to it. How are you going to explain when you need system_config and when not? Cos the answer seems to be that if the config object might end up saved you need system_config. Am I right?? If yes, we need something better :/

Gábor Hojtsy’s picture

Title: Introduce config context to make config consistent and accessible in different languages » Introduce config context to make original config and different overrides accessible

Slightly retitling. Talked to @beejeebus, he is looking to reroll Jose's patch and make it use the scope concept he introduced with his previous patches and also advocated by @sun.

@chx: yes, system_config() would be used if you want to deal with your pure config data, avoiding any interference with overrides such as $conf, languages, domains, groups, whatever. Such as when you don't what overrides to be saved back, you'd use this for the form.

Jose Reyero’s picture

FileSize
1.57 KB

@beejeebus,
Sounds good. About simplifications see #46

Gábor Hojtsy’s picture

Category: feature » task
Issue tags: +epic

Refiling as task given we are plugging in missing pieces (eg. the bug at #1868028: Raw (original) config data is not accessible).

catch’s picture

What about config_raw() or similar, that seems like it'd explain a bit better what it does? Could we also use this for the cases where we're reading raw config from disk during container rebuilds? It looks like we're going to end up with several things on the container that are in config and have these raw calls (i.e. any configurable config override + module list + ?) so if that's what we have to do it'd be good to have an API for it.

Gábor Hojtsy’s picture

@catch: I'm not sure if you propose to change how system_config() behaves but renaming it to config_raw() is totally fine IMHO.

catch’s picture

From the diagram, it looks like an AdminContext gets passed to avoid overrides. For this to be usable for container rebuilds too, we'd need to just load the configuration from files directly bypassing everything except the storage layer (i.e. don't initialize the context system at all). In effect I think this is the same thing though.

chx’s picture

IMO but please ignore me, system_config is an extremely confusing way to handle #1868028: Raw (original) config data is not accessible . In the complex world of Drupal, if I pass around data which contains config, I will have absolutely no idea whether to use config or system_config and indeed it is possible that depending on which modules are enabled and configured one or the other is the appropriate. This is not a good solution. But then again, I might be too cautious, so please, go ahead.

Gábor Hojtsy’s picture

@chx: well, system_config() is just a shortcut. Once beejeebus comes around to rerolling the patch and integrating his scope solution from earlier (that is also advocated by @sun), once you set a scope for config() calls to ignore context (AKA make all subsequent config() calls to behave like system_config() until the scope is changed), then this problem seems to be solved?!

Anonymous’s picture

Status: Needs work » Needs review
FileSize
40.72 KB

here's a reroll of #40, hopefully i haven't butchered it too much, to get things rolling again.

Anonymous’s picture

and here's the minimum changes to #97 to show the 'inject the context you want on the container' idea rather than 'pass the context/config to all the things'.

Anonymous’s picture

chx pointed out that we don't want ->load() any more in config() - removed that.

chx’s picture

To clarify my problem: let's say i got a config value displayed and I have an override in $conf. I want to be cool and I add in place editing. When you click edit are you going to see a different version to what's displayed? And if you edit, that seemingly doesn't take effect at all? What is even the desired workflow for this?

You can make this more twisted by having more than one config value on the form and not all of them overridden. I know we ditched automated config editing but I think a helper which makes a form element readonly would be helpful. Perhaps as simple as a #process callback which checks for presence in the overriddendata for a given config object / key?

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-99.patch, failed testing.

chx’s picture

Maybe the #process callback should add the overridden value to the description to decrease the wtf.

Gábor Hojtsy’s picture

@chx: I see your in-place editing concern but I don't see any good ways to fix it. If you run organic groups, it could override your site logo per group, so you might have hundreds of alternate values for site logos. When going to edit it, we would somehow need to add the whole context of the original page you came from so the admin page could first run the same logic to result in the same override. (As said elsewhere $conf based overrides are not static either and can have any level of logic behind them even). I am not sure how that could work since an override could depend on the path, the time of day, cookies or whatever. I think the whole override system is not in very good company with in-place editing or discoverability of where you can edit the values in general.

Anonymous’s picture

- fixed the fact that config_factory_set_context() didn't exist :-\
- ported the calls in config.inc to use config_factory_set_context() instead of setting context on the config objects

Anonymous’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-104.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
40.19 KB

- rollback the use of $config objects in config_sync_changes() and use the storage objects
- pass context directly to $config objects config_import_invoke_owner()

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-107.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
64.67 KB

- rollback the use of $config objects in config_sync_changes() and use the storage objects (for realz)
- pass context directly to $config objects config_import_invoke_owner() (for realz)
- use ContextInterface type hint for config_factory_set_context() instead of ConfigContext

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-109.patch, failed testing.

Anonymous’s picture

sigh, #109 was full of stupid.

Anonymous’s picture

Status: Needs work » Needs review

sigh, #109 was full of stupid.

Gábor Hojtsy’s picture

@chx, @catch, @Jose, @sun, @heyrocker: your review feedback would be very welcome :)

chx’s picture

I really like that it breaks the language circular dependency cleanly. // Set config overrides by reference so they can be added later. this I do not get, why do we want to change global $conf?

Gábor Hojtsy’s picture

@chx: my understanding is "we" (as in this patch or this code) does not want to change $conf, however, any code that would run after GlobalContext's constructor would not be able to modify conf values globally if we take the array by value. I assume this would be initialized very early and other modules that want to change settings would be able to run their code later.

Jose Reyero’s picture

I think this is looking pretty well, just some issues that should be easy to fix:

Issues with the caching keys:

$cache_key = get_class($this->getContext()) . ':' . $name;

The key for caching is not unique, i.e. contexts for different users would have the same key. Maybe we should either:
a) have some Context::getName() method that would produce a unique key per context:
b) Not caching configuration for contexts other than default context

Constants in ConfigContext class besides not being very well named, I think they complicate the code further, maybe we should just get rid of them.

+/**
+ * Defines the base configuration context object.
+ *
+ * A configuration context object provides a key/value store that can be used
+ * as a parameter to get customized configuration objects.
+ */
+class ConfigContext extends MemoryStorage implements ContextInterface {
+
+  /**
+   * Predefined key, will be TRUE for administrative contexts.
+   */
+  const ADMIN = 'config.admin';
...

About config_factory_set_context().
• Could it be shorter, like 'config_set_context' ?
• To switch back the context we could use no parameter and assume the context set in DIC

About using hooks, (hook_config_context_info), I think the config system should not depend on hooks, because it needs to be initialized too earlier, way before module loading. Anyway, what are we using this hook for?

Remove the code related to hook_config_factory() which is a left over.

--- a/core/modules/config/config.api.php
+++ b/core/modules/config/config.api.php
@@ -124,3 +124,25 @@ function hook_config_import_delete($name, $new_config, $old_config) {
   return TRUE;
 }
 
+/**
+ * Deletes configuration upon synchronizing configuration changes.
+ *
+ * This callback is invoked when a new configuration factory is created and
+ * allows a module to take over the synchronization of configuration data.
+ *
+ * Modules should implement this callback if they manage configuration data
+ * (such as image styles, node types, or fields) which needs to be prepared and
+ * passed through module API functions to properly handle a configuration
+ * change.
+ *
+ * @param Drupal\Core\Config\ConfigFactory $factory
+ *   A configuration object containing the new configuration data.
+ */
+function hook_config_factory($factory) {
...

@chx,

// Set config overrides by reference so they can be added later.
this I do not get, why do we want to change global $conf?

Modules in D7 used to change global $conf anytime for all kind of overrides. Maybe we can get rid of this later but for now it should be as D7 backwards compatible as possible, until we decide we don't need it anymore.

About fixing that 'circular dependencies', not sure about that, but in any case by using these contexts, since we have cleanly marked the config objects to be updated (config_system) maybe we can get rid completely of configuration overrides later.

gdd’s picture

Here's another reroll to HEAD, I also removed the hook_config_factory() code from config.api.php because it was one of the conflicts and its going away anyways.

I'm liking this new patch a lot. Moving setting the context into the factory is a good improvement.

The key for caching is not unique, i.e. contexts for different users would have the same key. Maybe we should either:
a) have some Context::getName() method that would produce a unique key per context:
b) Not caching configuration for contexts other than default context

Instinctively I lean towards b) but it would be a pretty bad performance hit for any sites using contexts so we probably should figure out a way to generate unique keys.

I'd still love to hear from sun and/or alexpott here but overall I'm feeling good about this.

Gábor Hojtsy’s picture

I verified the issue summary still contains an up to date class/interface hierarchy graph. Looks like the code examples need updating for the new scope approach, but other than that it looks good :)

Gábor Hojtsy’s picture

Issue summary: View changes

Fix code example and image.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Updated issue summary with up to date code examples. Since heyrocker and @reyero are happy, @beejeebus posted the integrated patch and @chx did not find anything to do damage control about, this seems to be a wide agreement to me. In fact the scoping of contexts was implemented based on @sun's suggestion even.

All that said, marking this one ready to go :)

chx’s picture

> Modules in D7 used to change global $conf anytime for all kind of overrides.

Note: this is already broken, if you want this to work then file an issue. Changing $conf will not affect any existing config, you need to reinit and/or clear the factory cache for that config. If noone gets to it, eventually I will because references are ick.

Jose Reyero’s picture

Status: Reviewed & tested by the community » Needs work

re @chx #121,
Agreed, you're right it won't work anyway, so we should remove that reference. Back to "needs work".

+class GlobalContext extends ConfigContext {
+
+  /**
+   * Overrides Drupal\Core\Config\ConfigContext::__construct().
+   */
+  public function __construct(StorageInterface $storage, EventDispatcher $event_dispatcher) {
+    global $conf;
+    // Set config overrides by reference so they can be added later.
+    $this->data[self::OVERRIDE] = &$conf;
 
Gábor Hojtsy’s picture

@chx/@Jose Reyero: do you suggest we copy $conf instead of referencing? @Jose: are you working on the update?

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
675 bytes
39.16 KB

Rerolled the patch for latest updates in core, it didn't apply anymore.
Changed global $conf overrides, not by reference anymore, as explained in #121 #122

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-124.patch, failed testing.

Gábor Hojtsy’s picture

All the fails are in ConfigOverrideTest, so probably highly related to the patch.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
40.89 KB

Yeah, obviously, since we are not passing global conf by reference now, we need to refresh it if we update the values.

Fixed the test.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the update, chx's concern was resolved, so looks good to go again :)

catch’s picture

Would be good to see profiling data for the extra event listener being added. I don't think we have a choice, but still would be good to know.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.phpundefined
@@ -0,0 +1,40 @@
+ * Definition of Drupal\Core\Config\AdminContext.

I think this is Contains ... now

+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.phpundefined
@@ -0,0 +1,40 @@
+   * Overrides Drupal\Core\Config\ConfigContext::__construct().

\Drupal\...

+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.phpundefined
@@ -0,0 +1,40 @@
\ No newline at end of file

Needs newline

These things happen everywhere in the patch. Please fix them :). Keep going!

Gábor Hojtsy’s picture

All right, I found docs for the Contains. I did not find docs for the "Overrides \Drupal\..." and it does not seem to be it is winning out in grepping either, eg (not necessarily representative):

core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.php:   * Overrides Drupal\Core\Entity\EntityInterface::get().
core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.php:   * Overrides Drupal\Core\Entity\EntityInterface::uri().
core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.php:   * Overrides Drupal\Core\Entity\EntityInterface::id().
core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.php:   * Overrides Drupal\Core\Config\Entity\ConfigEntityBase::createDuplicate().
core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.php:   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::getExportProperties();
Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
40.7 KB

Rerolled for the "Contains" and the missing newlines.

YesCT’s picture

http://drupal.org/node/1354#namespaces might be what you are looking for.
In the past, usually the \ was omitted.
but now
"If you refer to a class/interface that is not in the current file's use/namespace declaration context, or that is in the global namespace, prefix it with the fully-qualified namespace name (starting with a backslash)."

Patches are going in without it, so this is not blocking. But thought the reference might help.
For an example though, see @jhodgdon #1877632-25: Improve comments and clean-up code for EntityNG and the TypedData API

Gábor Hojtsy’s picture

All right then, changed @param Drupal\... to @param \Drupal\... and Overrides Drupal\... to Overrides \Drupal\....

Dries’s picture

I reviewed this patch and it looks good to me. It took my a while to figure it out though; I don't have any suggestions right now but I wish it could have been made a bit easier to discover.

Gábor Hojtsy’s picture

@catch: can you provide more specifics as to what you see would be best profiled? How it affects a general page or something specifically tailored in CMI?

catch’s picture

General page before/after would be plenty yeah.

Anonymous’s picture

unless someone beats me to it, i'll post some xhprof data tomorrow.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Looks like this is still needs review.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +feature freeze, +epic

The last submitted patch, 1763640-config-context-134.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@beejeebus: have you been able to look into this? Thanks!

Anonymous’s picture

here's a reroll, couldn't get #134 to apply.

Status: Needs review » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -feature freeze, -epic

The last submitted patch, 1763640-143-config-context.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#143: 1763640-143-config-context.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +feature freeze, +epic

The last submitted patch, 1763640-143-config-context.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
41.6 KB

ok, this patch should get beyond installation on the bot.

Status: Needs review » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -feature freeze, -epic

The last submitted patch, 1763640-config-context-147.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +feature freeze, +epic

#147: 1763640-config-context-147.patch queued for re-testing.

Anonymous’s picture

alright. NFI idea why it failed one time then passed the next, but now i'll do some runs and post some xhprof data.

Gábor Hojtsy’s picture

@justin.randell: Thanks, any xhprof results? :)

Anonymous’s picture

sorry, nope. i'm not going to get to this soon :-(

gdd’s picture

Status: Needs review » Reviewed & tested by the community

I took a look at this today. On the default home page from a fresh install, results were pretty similar between sites with and without the patch. On 10 page loads, wall time hovered around 530ms, with highs up to 720ms and lows down to 450ms regardless of whether or not the patch was applied. No significant difference at all. So I'm taking this back to RTBC.

sun’s picture

Hm. I didn't look at this for a long time...

I think we badly need to move forward on this topic, but at the same time, I think I found a range of larger problems with the current implementation (as noted below). So I'm not sure what the best strategy is.

I fear that moving forward with the patch as-is will introduce some weird/confusing behavior in HEAD, which doesn't look exactly right in some areas and which could perfectly have the potential of tripping up other, poor/innocent contributors who're working on other aspects of the system. At the same time, not moving forward with this as-is, could easily mean that the facility might be delayed for another month or so... Tough.

I don't have an answer for that, but here's what I found in reviewing the latest patch:

+++ b/core/includes/config.inc
@@ -179,10 +196,11 @@ function config_sync_get_changes(StorageInterface $source_storage, StorageInterf
 function config_sync_changes(array $config_changes, StorageInterface $source_storage, StorageInterface $target_storage) {
+  $target_context = new AdminContext('config.target', $target_storage);

Looking at this code in isolation, it's not clear to me why the config import process hard-codes an AdminContext. This could use an elaborate inline comment that explains "why".

+++ b/core/includes/config.inc
@@ -249,6 +267,8 @@ function config_import() {
+  $target_context = new AdminContext('config.target', $target_storage);
+  $source_context = new AdminContext('config.target', $source_storage);

+++ b/core/includes/install.core.inc
@@ -335,9 +335,11 @@ function install_begin_request(&$install_state) {
+    $container->register('config.context', 'Drupal\Core\Config\Context\ConfigContext')
       ->addArgument(new Reference('config.storage'))
       ->addArgument(new Reference('event_dispatcher'));

It looks odd that the constructor parameters do not match up.

1) AdminContext seems to get a string identifier as first argument, but ConfigContext does not.

2) ConfigContext gets a config storage as first argument, but AdminContext takes that second.

3) Not having looked futher into this patch, I almost figure that ConfigContext actually is a ConfigContextFactory (and thus a misnomer), which in turn would explain why it takes different constructor arguments.

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -89,16 +89,13 @@ class Config {
+  public function __construct($name, ContextInterface $context) {
...
+    $this->context = $context;
+    $this->storage = $context->getStorage();

@@ -478,20 +475,10 @@ public function delete() {
-  public function getStorage() {

I do not understand why the storage for a configuration object would be tied to a context?

The storage should always be the same, for all config objects; injected as a service. Whether the current context is "foo", "admin", or "bar, shouldn't have an impact on which configuration storage engine is used.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -48,33 +42,27 @@ class ConfigFactory {
+    $cache_key = get_class($this->getContext()) . ':' . $name;

@@ -86,8 +74,9 @@ public function get($name) {
+      $cache_key = get_class($this->getContext()) . ':' . $name;

@@ -108,14 +97,37 @@ public function reset($name = NULL) {
+    $old_cache_key = get_class($this->getContext()) . ':' . $old_name;
+    $new_cache_key = get_class($this->getContext()) . ':' . $new_name;

Minor: Looks like we could cache the class name of the active context?

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -108,14 +97,37 @@ public function reset($name = NULL) {
   public function rename($old_name, $new_name) {
...
+      $this->cache[$old_cache_key] = clone $config;
...
+      $this->cache[$new_cache_key] = $config;

I wonder whether it is valid to "rename" a config object from one context to another? (which essentially seems to be happening here?)

+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.php
@@ -0,0 +1,40 @@
+use Drupal\Core\KeyValueStore\MemoryStorage;
...
+use Symfony\Component\EventDispatcher\EventDispatcher;

Trivial: Appears to be unused.

+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.php
@@ -0,0 +1,40 @@
+  public function __construct($name, StorageInterface $storage = NULL) {

Can we globally rename $name to $context_id?

This caused me quite some confusion... (see below)

+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.php
@@ -0,0 +1,40 @@
+    $this->set(self::ADMIN, $name);

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContext.php
@@ -0,0 +1,84 @@
+   * Predefined key, will be TRUE for administrative contexts.
+   */
+  const ADMIN = 'config.admin';

$name is not TRUE, so the actual usage conflicts with its documentation?

(scratch that)

+++ b/core/lib/Drupal/Core/Config/Context/AdminContext.php
@@ -0,0 +1,40 @@
+    parent::__construct($storage ?: drupal_container()->get('config.storage'), drupal_container()->get('event_dispatcher'));

Both of these should be injected...?

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContext.php
@@ -0,0 +1,84 @@
+ * A configuration context object provides a key/value store that can be used
+ * as a parameter to get customized configuration objects.
...
+class ConfigContext extends MemoryStorage implements ContextInterface {

+++ b/core/lib/Drupal/Core/Config/Context/GlobalContext.php
@@ -0,0 +1,32 @@
+class GlobalContext extends ConfigContext {
...
+    $this->data[self::OVERRIDE] = $conf;

I wanted to mention that ConfigContext::$data is undocumented, but...

...extends KeyValue\MemoryStorage?

I can't see why I should understand that? There doesn't seem to be any code - aside from get() and set() - that would actually use a context as a key/value storage?

Just get/set on their own are definitely not a reason to extend a KeyValue storage. That's very basic state management on + within a class. No need for a "storage layer" in between.

+++ b/core/lib/Drupal/Core/Config/Context/ObjectContext.php
@@ -0,0 +1,41 @@
+  public function __construct($name, $object) {
+    // Set the object name for reference.
+    $this->set(self::OBJECT, $name);
+    $this->set($name, $object);
+    parent::__construct(drupal_container()->get('config.storage'), drupal_container()->get('event_dispatcher'));
+  }

In general, I do not understand why the constructor logic always looks identical... normally, this means that the logic should be performed by whichever-class that is calling/constructing these classes, instead of inside-out?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOverrideTest.php
@@ -52,10 +53,11 @@ function testConfOverride() {
+    drupal_container()->get('config.context')->set(ConfigContext::OVERRIDE, $conf);

@@ -93,8 +95,9 @@ function testConfOverride() {
+    drupal_container()->get('config.context')->set(ConfigContext::OVERRIDE, $conf);

Tests should always use $this->container() instead of drupal_container().

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
@@ -18,6 +18,23 @@
+  /**
+   * Initialize configuration context with language.
+   *
+   * @param \Drupal\Core\Config\ConfigEvent $event
+   *   The Event to process.
+   */
+  public function configContext(ConfigEvent $event) {

configContext() is a weird method name. I would not have expected it to be the initialization method for a context.

Looking some lines below, I see that the method name is manually registered as an event listener. But shouldn't we have a shared + common interface for all config contexts throughout Drupal, using some more sensible method names? ;)

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
@@ -18,6 +18,23 @@
+    if (!$context->get('locale.language')) {
+      // Add user's language for user context.
+      if ($account = $context->get('user.account')) {
+        $context->set('locale.language', language_load(user_preferred_langcode($account)));
+      }

Given that ConfigContext currently holds a storage, too, this really looks confusing — at first sight, I thought this code would read in a config object within that context (because, storage...), but apparently:

ConfigContext->get() == KeyValue\MemoryStorage->get() != Config->get()

Ugh. :-/

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
@@ -25,11 +42,13 @@ class LocaleConfigSubscriber implements EventSubscriberInterface {
   public function configLoad(ConfigEvent $event) {
...
+    $context = $event->getContext();
+    if ($language = $context->get('locale.language')) {
+      $config = $event->getConfig();
+      $locale_name = $this->getLocaleConfigName($config->getName(), $language);
+      if ($override = $context->getStorage()->read($locale_name)) {
+        $config->setOverride($override);
+      }
     }

Hm. I thought this kind of "actual" config context code would be vastly simplified with the new contexts...?

Why does this code still need to figure out on its own whether it is active, and additionally apply itself, on every single config.load event?

+++ b/core/modules/system/system.admin.inc
@@ -1378,7 +1378,7 @@ function system_modules_uninstall_submit($form, &$form_state) {
 function system_site_information_settings($form, &$form_state) {
-  $site_config = config('system.site');
+  $site_config = system_config('system.site');

@@ -1484,7 +1484,7 @@ function system_site_information_settings_validate($form, &$form_state) {
 function system_site_information_settings_submit($form, &$form_state) {
-  config('system.site')
+  system_config('system.site')

It doesn't feel right that the same context has to be entered twice for the same operation (for settings forms here).

This means that any config() call that happens in between form building + rendering + processing + validation + final submission has to manually figure out whether it is operating in the correct config context, and conditionally adjust itself to use the proper functions/methods... Did we account for hook_form_alter(), #process, #theme, #validate, #element_validate, and everything else here?

The context should be set once for the entire operation; i.e., before the form constructor is invoked. And the context should be left after drupal_build_form() completed the entire processing and form operation.

+++ b/core/modules/system/system.module
@@ -3441,6 +3442,37 @@ function system_admin_compact_page($mode = 'off') {
+function system_config($name) {
+  static $context;
+  if (!isset($context)) {
+    $context = new AdminContext('system.config', drupal_container()->get('config.storage'));

A pure PHP static is definitely wrong here, because that leaks into all tests, and from one test into the next.

At minimum, we need a drupal_static here. But we actually do not introduce new drupal_statics in D8 anymore... By the looks of this function, shouldn't this be a registered service?

+++ b/core/modules/user/user.admin.inc
@@ -292,8 +292,8 @@ function user_admin_account_validate($form, &$form_state) {
 function user_admin_settings($form, &$form_state) {
-  $config = config('user.settings');
-  $mail_config = config('user.mail');
+  $config = system_config('user.settings');
+  $mail_config = system_config('user.mail');

contextless_config() (or similar) would be a bit more self-descriptive...

"system" doesn't really have any meaning.

+++ b/core/modules/user/user.module
@@ -8,6 +8,7 @@
+use Drupal\user\UserConfigContext;

@@ -701,6 +702,18 @@ function user_format_name($account) {
 /**
+ * Implements hook_config_context_info().
+ */
+function user_config_context_info() {
+  return array(
+    'user.account' => array(
+      'title' => t('User account'),
+      'class' => '\Drupal\user\UserConfigContext',
+    ),
+  );
+}

Either I'm missing something, or this info hook must be completely obsolete...? I can't any other implementation in this patch?

+++ b/core/modules/user/user.module
@@ -1748,35 +1761,27 @@ function user_view_multiple($accounts, $view_mode = 'full', $langcode = NULL) {
+  // Get configuration objects customized for this user, that may be localized
+  // for the user's language if the locale module is enabled.
+  $config_context = config_factory_set_context(new UserConfigContext($params['account']));
+  $mail_config = config('user.mail');
...
+  // Set the configuration context back to the original.
+  config_factory_set_context($config_context);

I really thought of this code flow to be just this:

$config = config('user.mail');
$config->enterContext('user', $params['account']);

// ... send mail...

$config->leaveContext();

I.e., from a DX/user perspective, don't make me think about importing + creating entire classes, namespaces, and whatnot.

Instead, let me just enter the context, let me perform what I want to perform, and leave the context.

I'm not a fan of the additional procedural functions we're introducing for this facility — why can't it be an implicit facility on the returned Config object?

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +feature freeze, +epic

The last submitted patch, 1763640-config-context-147.patch, failed testing.

Gábor Hojtsy’s picture

It is hard to believe Drupal\openid\Tests\Upgrade\OpenIDAuthmapUpgradePathTest fails would be related :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#147: 1763640-config-context-147.patch queued for re-testing.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +feature freeze, +epic

The last submitted patch, 1763640-config-context-147.patch, failed testing.

alexpott’s picture

The patch attached tries to address the architectural issues brought up by sun in #154

  • Decouples config storage and config context
  • Creates additional config context services on the container instead of a proliferation of classes extending ConfigContext (AdminContext, ObjectContext, GlobalContext are gone)
  • Adds a ConfigContextFactory to handle the creation of ConfigContext objects
  • ConfigContext no longer extends MemoryStorage
  • Adds tests for user language overrides
  • Changes tests to DrupalUnitTestBase
  • Removes uninvoked hook

I haven't yet managed to return a config context setting facility on the config object due to the way the context is injected into the factory and config objects are cached here - and I don't know if this is worth pursuing. One change brought about by this patch is that the following is possible:

    $config_context = drupal_container()->get('config.context.user')->setAccount($account);
    $config = config('config_test.system', $config_context);

Additionally this patch hasn't addressed sun's issues with system_config() and some other things from his review eg. the listener method names.

The interdiff is not a precise interdiff to #147 as this patch required a reroll but it is indicative of the changes.

alexpott’s picture

Status: Needs work » Needs review

Please testbot :)

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-162.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
49.87 KB
6.03 KB

Patch attached resolves the test failures by

  • adding a listener to the request event on the locale event subscriber to set the context language
  • removing the injection of global $conf on the container and moving it to a new GlobalConfigContext object as this was preventing the container from compiling during the views enable during system_update_8046()

The interdiff is not a precise interdiff to #162 as this patch required a reroll but it is indicative of the changes.

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-165.patch, failed testing.

alexpott’s picture

The issue that testbot has caught is this:
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "locale_config_subscriber", path: "locale_config_subscriber -> config.context". in Symfony\Component\DependencyInjection\Container->get() (line 255 of /Users/alex/dev/sites/drupal8alt.dev/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php).

To reproduce:

  1. Enable locale module
  2. Then enable shortcut module

I'm bamboozled as to why this has only caused 1 test failure...

alexpott’s picture

Status: Needs work » Needs review
FileSize
53.58 KB
10.5 KB

This patch should fix the test failure in #165...

The config.context service needs to be tagged with persist as there can only be one on the container - since is contains the global overrides. This stops the circular dependency from arising when a config installation is performed.

The patch also renames the config.context.admin service to config.context.free (as in... free of overrides).

Additionally I've added tests to ensure global overrides are not contaminating config during installation and import as injecting config.context.free into the config factory during these processes was causing bugs that were not found by the existing tests.

EDIT: Made the post hopefully a bit more understandable... I was tired :)

Anonymous’s picture

i like #168, overall it looks like a really nice cleanup.

however, it seems to have completely broken the scoping? that is, we're now only able to set config context on individual config objects? sun points out some problems with that here:

It doesn't feel right that the same context has to be entered twice for the same operation (for settings forms here).

This means that any config() call that happens in between form building + rendering + processing + validation + final submission has to manually figure out whether it is operating in the correct config context, and conditionally adjust itself to use the proper functions/methods... Did we account for hook_form_alter(), #process, #theme, #validate, #element_validate, and everything else here?

The context should be set once for the entire operation; i.e., before the form constructor is invoked. And the context should be left after drupal_build_form() completed the entire processing and form operation.

maybe the changes were trying to address sun's later point:

I really thought of this code flow to be just this:

$config = config('user.mail');
$config->enterContext('user', $params['account']);

// ... send mail...

$config->leaveContext();

I.e., from a DX/user perspective, don't make me think about importing + creating entire classes, namespaces, and whatnot.

Instead, let me just enter the context, let me perform what I want to perform, and leave the context.

I'm not a fan of the additional procedural functions we're introducing for this facility — why can't it be an implicit facility on the returned Config object?

not sure. if we've decided to force all callers of config() to care about $context, so be it. i'm assuming moving back to the design we decided to ditch from earlier patches is a deliberate choice, so leaving at needs review.

Gábor Hojtsy’s picture

@beejeebus: I don't think we want to go back to the previous design where we did not enter/leave contexts but instead needing to specify contexts. Looks like what @sun proposes is a drupal_get_admin_form() or something along those lines that does a context enter then a drupal_get_form() and then a context leave. And then the same for all instances where drupal_get_form() is not directly invoked (eg. config entities). Sounds like a whole can of worms to go into :D

alexpott’s picture

The reason I've left it as having to declare what context you want each time you use config() is that persistent context seems fraught with danger. Say the form creation sets the context on system.file to be override free and then validation and submission assume the config is in this context... What happens if a form alter changes this or actually needs to use it and assumes that overrides are applying...

I just can't see how we can get around having to explicitly declare what context you want if it is not the default.

Callers to config don't need to care about context if they want the default context. But if they do want to use a special context then doing it here allows us to cache the config objects with their contexts on the factory.

Gábor Hojtsy’s picture

@alexpott: problem is then you need to add to every single API layer *above* config an argument to specify and pass down contexts, and need to always keep it around manually. Ie. any code you might want to use in different contexts.

alexpott’s picture

The patch attached re-implements the scoping as suggested #169 and #170.

The patch borrows from sun's suggestion for the DX.

    // Enter the override free context.
    $config_factory = drupal_container()->get('config.factory');
    $config_factory->enterContext(drupal_container()->get('config.context.free'));

    //... do stuff with config() ... all calls will use the override free context

    // Exit the override free context.
    $config_factory->leaveContext();
Gábor Hojtsy’s picture

@beejeebus, @sun: any comments / concerns on the fixes that @alexpott proposed based on your previous comments?

Anonymous’s picture

#173 looks good to me.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

discussed this with alexpott in IRC. i think this is ready to fly.

Gábor Hojtsy’s picture

All righto. I noticed that in this patch, there is no direct language context anymore, one can only set language via a user account or via the negotiated interface language. I think that is fine for a first patch and we can get a concrete language context in later to make it possible for modules to switch to specific languages if they need to (without faking a user account to do it :). I don't think there is any hurry in doing that in this patch, it is much more important to set up this solution in core.

Updating the class graph in the issue summary with this new one.

Gábor Hojtsy’s picture

Issue summary: View changes

Update for config_factory_set_context()

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

I fully edited the issue summary too to correspond to the current proposal. The code example for using contexts ended up like this (code directly lifted from the patch with plenty added comments to explain):

// First get the config factory.
$config_factory = drupal_container()->get('config.factory');

// Instantiate a user context to be used for the override.
$user_config_context = drupal_container()->get('config.context.user');

// Make the factory enter the user specific context for this account.
$config_factory->enterContext($user_config_context->setAccount($account));

// Now the configuration retrieved (and any subsequent config() calls in
// API functions) will work with the context on the top of the context
// stack (that we most recently entered).
$mail_config = config('user.mail');

// Use token_replace(), etc. here on the mail configuration to replace
// with values proper for the context we entered (eg. the site name or
// slogan properly translated to the language we use).

// Reset config context to the prior value on the stack before leaving
// this operation. This ensures any wrapping code will return to the
// context that was set prior and our user specific context will not
// persist.
$config_factory->leaveContext();

I suspect we'll work on refining this boilerplate later but that should not hold back the introduction of the facility IMHO. More experience with this will help with figuring out how to best use this. And hopefully more people looking at it :)

Gábor Hojtsy’s picture

Uh, tag loss bug.

Dries’s picture

Issue tags: -feature freeze

I'm not sure where the 'feature freeze' tag comes from; not sure what it means ... In any event, as far as I'm concerned this is more like a bug report. It can go in after the feature freeze.

I looked at the patch and there are a few things I'm not 100% about yet.

1) I'd prefer to see us add a paramater to config() instead of introducing system_config(). Thoughts on that?

2) It is also not clear why we keep the config.context.user in the container.

+  $config_factory = drupal_container()->get('config.factory');
+  $user_config_context = drupal_container()->get('config.context.user');
+  $config_factory->enterContext($user_config_context->setAccount($params['account']));

I'd prefer to see the last line split in two lines:

+ $user_config_context->setAccount($params['account']);
+  $config_factory->enterContext($user_config_context);

Otherwise, this looks pretty close to being RTBC. Good work.

alexpott’s picture

1. The thing about system_config() is that it is trying to make it clear that we're going to do something at the scope of the 'config system' ...
Consider the following options...

// The current solution.
$config = system_config('system.site');

// Add a parameter like bool $use_override_free_context to config().
$config = config('system.site', TRUE);

// Have a procedural wrapper round the container stuff.
config_enter_override_free_context();
$config = config('system.site');

// Just use the container.
drupal_container()
  ->get('config.factory')
  ->enterContext(drupal_container()->get('config.context.free'));
$config = config('system.site');

I'm not sure which of these is the best to be honest... adding a parameter lto config() does not look that clear from a DX perspective. Maybe system_config() could have a better name...


2. Good point... config.context.user does not need to be on the container. Patch attached does this.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

@alexpott: Yeah, so what system_config() does is (a) enters the free context (b) returns the value. Due to its similar naming to config(), it does not seem like it would have longer lasting effect like entering free context. This was introduced in response to @sun above vs. the previous need to enter this context for all form functions (and then leave), for admin forms. This enters the context in the builder and then keep that. I think something along the lines of the following would work better:

config_enter_override_free_context();
$config = config('system.site');

Ideally more something like:

config_context_enter('config.context.free');
$config = config('system.site');

config_context_enter('config.context.user', $params['account']);
$config = config('system.site');

That would get us one unified way to enter a context (config_context_enter() could take arbitrary number of arguments to pass on to the context factory to pass onto the context instance). Right now we have this extremely simple system_config() and the much more convoluted context entering elsewhere, so if you want the context free case, you have something very simple, then otherwise you have many lines of boilerplate to copy.

@Dries: the reason for this to be targeted at feature freeze is because it is admittedly a feature on the config system. Yes, it is a missing piece that causes bugs. But it also needs some time to mature and cristalize while the APIs are still adaptable, and we need the integration phase in core to integrate with places not yet adapted to context. Similarly like we did with config schemas. API questions like the above can probably be endlessly debated, and we'll likely change the context API at least one time even after this is committed. Judging from other areas where I worked on so far in Drupal 8.

alexpott’s picture

Patch attached implements new function config_context_enter() that excepts a single argument of either a service on the DIC or a class name so...

$config = system_config('system.site');

// becomes...
config_context_enter('config.context.free');
$config = config('system.site');

... and ...

$config_factory = drupal_container()->get('config.factory');
$user_config_context = drupal_container()->get('config.context.factory')->get('Drupal\user\UserConfigContext');
$config_factory->enterContext($user_config_context->setAccount($params['account']));
$mail_config = config('user.mail');

// becomes...
$user_config_context = config_context_enter("Drupal\\user\\UserConfigContext");
$user_config_context->setAccount($params['account']);
$mail_config = config('user.mail');

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-183.patch, failed testing.

Gábor Hojtsy’s picture

Massive test fails seem to be unrelated. Eg. errors with "possibly out of disk space".

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -epic

#183: 1763640-config-context-183.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +epic

The last submitted patch, 1763640-config-context-183.patch, failed testing.

alexpott’s picture

The failure in ConfigCRUDTest.php was due to fixing the try/catch in config_import() by adding the use Drupal\Core\Config\ConfigException;! Adapted test to work now this is working...

I have no clue why Drupal\system\Tests\Menu\MenuRouterTest is failing - locally it fails with Menu router 218 passes, 6 fails, 6 exceptions... not just a single fail :(

alexpott’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks like that was intermittent then :) Fixes Dries' concerns elegantly IMHO.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Erm, wait, looking at preparing a change notice now I realize we should have a config_context_leave() for parity. Now that regular Drupal code would not have the config factory around to leave it, and the code updates show you still need to get it from the container then. Looks like we improvd enteroing the context but not leaving.

alexpott’s picture

Status: Needs work » Needs review
FileSize
59.06 KB
2.28 KB

And here's the config_context_leave() function...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Huge thanks for the quick turnaround and your dedication to this issue. :) Looks great and unified now. And if others don't like the immediate syntax, they can still have a say for a few months. So we get some real life use practice. Thanks again!

Status: Reviewed & tested by the community » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -epic

The last submitted patch, 1763640-config-context-192.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +epic

#192: 1763640-config-context-192.patch queued for re-testing.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

passes, so setting back to rtbc according to @Gabor in #193

alexpott’s picture

Once again proving that unless an issue has 200+ comments... it's not worth doing :)

Here's a reroll now that the snapshot patch has landed... the interdiff contains more confusion than help so the interdiff attached is the change I had to make to fix a test.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -epic

The last submitted patch, 1763640-config-context-197.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

#197: 1763640-config-context-197.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1763640-config-context-197.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +epic

#197: 1763640-config-context-197.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Still looks good!

Gábor Hojtsy’s picture

Issue tags: +Avoid commit conflicts

Also adding avoid commit conflicts to try and avoid further painful rerolls.

webchick’s picture

Assigned: Unassigned » catch
Issue tags: -Avoid commit conflicts

Even though it looks like Dries's feedback was resolved (thanks!), assigning to catch for one last look at the architecture.

plach’s picture

Issue tags: +Avoid commit conflicts

Restoring lost tag.

Gábor Hojtsy’s picture

#197: 1763640-config-context-197.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/config.incundefined
@@ -24,6 +24,10 @@
 function config_install_default_config($type, $name) {
+  // Use the override free context for config importing so that any overrides do
+  // not change the data on import.
+  config_context_enter('config.context.free');
+
   // If this module defines any ConfigEntity types then create an empty
   // manifest file for each of them.
   foreach (config_get_module_config_entities($name) as $entity_info) {
@@ -47,6 +51,11 @@ function config_install_default_config($type, $name) {

@@ -47,6 +51,11 @@ function config_install_default_config($type, $name) {
     $remaining_changes = config_import_invoke_owner($config_changes, $source_storage, $target_storage);
     config_sync_changes($remaining_changes, $source_storage, $target_storage);
   }
+  // Exit the override free context and ensure that any new manifest data is
+  // available.
+  drupal_container()->get('config.factory')
+    ->leaveContext()
+    ->reset('manifest');

Why can't this use config_context_leave(). I know there's the reset('manifest') but that could be a parameter?

+++ b/core/includes/config.incundefined
@@ -99,6 +108,54 @@ function config($name) {
+function config_context_enter($context_name) {
+  if (drupal_container()->has($context_name)) {
+    $context = drupal_container()->get($context_name);

This looks like it could move to a container-aware service?

config.context.free

I like config.context.free, it's very self-explanatory which the 'system' context wasn't.

+++ b/core/includes/config.incundefined
@@ -230,9 +288,19 @@ function config_import() {
+ ¶
+    // Exit the override free context and ensure that any new manifest data is
+    // available.
+    drupal_container()->get('config.factory')
+      ->leaveContext()
+      ->reset('manifest');

This is the same as earlier, could definitely do with consolidation into a helper.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -108,14 +117,76 @@ public function reset($name = NULL) {
+   */
+  public function getCacheKey($name, ContextInterface $context) {
+    return $name . '.' . $context->getUuid();

Why does the cache key use a dot? Every other cache key normally uses a colon.

Also the class is missing parameter documentation on these methods.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -108,14 +117,76 @@ public function reset($name = NULL) {
+
+  /**
+   * Gets all the cache keys that match the provided config name.
+   *
+   * @return array
+   *   An array of cache keys that match the provided config name.
+   */

$name needs documenting.

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContext.phpundefined
@@ -0,0 +1,122 @@
+ * A configuration context object provides a key/value store that can be used

Is it actually providing a key value store, or just an array? We have a key/value store API in core so this is a bit confusing.

I'm not really sure about usercontext being a top-level thing, isn't it possible we'd need context for other entities (say field label translation based on node language) - if we do then the user context could extend a generic entity context. However so far it's the only one that's needed so could just be follow-up.

Gábor Hojtsy’s picture

Issue tags: -Regression, -D8MI, -sprint, -language-config, -epic

@catch: Thanks for the review, good stuff!

- re reset manifest, that could be a slippery slope. Is this the only thing that should be possibly done in a chained setup or would there be other possible arguments?
- as for user context being top level, earlier proposals (eg. #27 above) had a generic object context which then was further specialised into a user context; I think this can be introduced later once we have some practice; as it stands now we are more just going in circles here

Looks like your feedback can be fixed quickly and we can get this in and get more improvements in later based on feedback from the wider developer base :) Superb!

Gábor Hojtsy’s picture

Issue tags: +Regression, +D8MI, +sprint, +language-config, +epic

Restore lost tags.

catch’s picture

Issue tags: -Regression, -D8MI, -sprint, -language-config, -epic

Hmm another thing here which makes me uncomfortable:

+  // Get configuration objects customized for this user, that may be localized
+  // for the user's language if the locale module is enabled.

This is the only place in the patch that the user context is used, because the user that the mail goes to isn't necessarily the same as the one triggering the mail sending in the request. I think this needs extra clarification that this is only done here because the user being used to send the mail is the one from $params - i.e. that you should never, ever set this just because you're calling config and the function does something based on the current user.

plach’s picture

Issue tags: +Regression, +D8MI, +sprint, +language-config, +epic

(What would Gabor do?)

Restore lost tag.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -sprint
FileSize
59.57 KB
5.54 KB

Implemented most of catch's suggestions from @207

@catch said on irc that moving config_context_enter() to a container-aware service could be explored in a later patch.

With regards to the manifest config object resetting - I've moved this to a much better place.

alexpott’s picture

Patch to address @catch's comment in #210

plach’s picture

Issue tags: +sprint

These tags don't stick :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good to me, especially cleaning up leaving context. :) Looks like this satisfies @catch's concerns too (as well @Dries' from above), so at least with two core committers reviewing the patch, it should hopefully be an easy way in now :)

YesCT’s picture

Is there anything in the recent changes we could update in the issue summary, in the meantime?

YesCT’s picture

Issue summary: View changes

Heavily updated issue summary based on latest patches.

Gábor Hojtsy’s picture

@YesCT: just updated summary with current code examples. http://drupal.org/node/1763640/revisions/view/2560078/2578696

YesCT’s picture

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

#213: 1763640-config-context-213.patch queued for re-testing.

catch’s picture

Title: Introduce config context to make original config and different overrides accessible » Change notice: Introduce config context to make original config and different overrides accessible
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Looks much better. Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Woot, thanks!

Gábor Hojtsy’s picture

Title: Change notice: Introduce config context to make original config and different overrides accessible » Introduce config context to make original config and different overrides accessible
Priority: Critical » Major
Status: Active » Fixed

Added a change notice at http://drupal.org/node/1928922 (also hooked in #1646580: Implement Config Events and Listeners, and storage realms for localized configuration where the original override system was added since that was significantly amended in this commit). Documented the two altogether in http://drupal.org/node/1928898 (which is linked from the change notice). I believe this should be good for a change notice.

Thanks again for the herculean effort, it is highly appreciated. Now we are onto #1905152: Integrate config schema with locale, so shipped configuration is translated to actually integrate the config override system for languages with something useful for people :)

Gábor Hojtsy’s picture

Issue tags: -sprint
pounard’s picture

Issue tags: +sprint

Random notes from a short review done with latest git version pulled.

ConfigFactory.php line 163:

  public function leaveContext() {
    if (count($this->contextStack) > 1) {
      array_pop($this->contextStack);
    }
    return $this;
  }

Can just be:

  public function leaveContext() {
    array_pop($this->contextStack);
    return $this;
  }

array_pop() does not emit any warning when trying to pop an empty array.

ConfigFactory.php line 170:

  /*
   * Gets the cache key for a given config name in a particular context.
   *
   * @param string $name
   *   The name of the configuration object.
   * @param \Drupal\Core\Config\Context\ContextInterface $context
   *   The configuration context.
   *
   * @return string
   *   The cache key.
   */
  public function getCacheKey($name, ContextInterface $context) {

Missing * in first line.

ConfigFactory.php line 147:

  /**
   * Gets the current config context.
   *
   * @return \Drupal\Core\Config\Context\ContextInterface $context
   *   The current configuration context.
   */
  public function getContext() {
    return end($this->contextStack);
  }

It is not documented what happens if the stack is empty. So the caller might just chain method calls onto this and trigger some PHP fatal errors. Why not using a null context object for when the stack is empty?

For example, the global override is not here (which is actually saving us from any WSOD), Config::__construct() could inherit from an empty context, then WSOD when calling Config::notify().

ContextInterface.php line 25:

  /*
   * Initialises a config context for use.
   *
   * Creates a unique context identifier, adds data and notifies system about
   * the new context.
   *
   * @param string $context_key
   *   The key that is used to set context data.
   * @param mixed $data
   *   The context config data.
   *
   * @return \Drupal\Core\Config\Context\ConfigContext
   *   The config context object.
   */
  public function init($context_key, $data);

Missing * in first line.

ConfigFactory::get() caches config objects overriden by specific contextes, but ConfigFactory::leaveContext() does not clear the cache accordingly, this is what I would call a memory leak.

ConfigFactory::get() caches multiple versions of the same config object, isn't it too much in memory? Let's say we have an average of more or less 30 config objects (what I measured some days ago on a fresh D8 install from git) and half of them are being hit in various contextes (language and user for example) would I have N + 1/2N * M config objects into memory (where N is 30 and M the number of contextes my page encounters in my example 2, which leads to a total count of 60). This is a relatively small number, but I guess that once I'd have enabled 60 modules for example, this would lead to a stupidely high number of instances and a lot of data duplication.

Aren't you afraid that those hundreds of init() calls and events being raised would cost *a lot* in term of execution time? For example, if the LocaleConfigSubscriber reacts upon every one of them, this is quite a lot. It seems drastically unoptimized.

If context are stacked, are the overrides on the current config object I get too? Suppose that I stack a UserContext, with a language, then a NodeContext, which has none (example) what happens if the LocaleConfigSubscriber attempt a get('user.account') on the NodeContext, fetches nothing, and ignore the UserContext? What will be my language?

Gábor Hojtsy’s picture

Title: Introduce config context to make original config and different overrides accessible » Followup: Introduce config context to make original config and different overrides accessible
Priority: Major » Normal
Status: Fixed » Needs review
Issue tags: -Avoid commit conflicts
FileSize
1.1 KB

@pounard: Welcome! Thanks for the review!

Attached patch fixes the comment issues and the array_pop() simplification proposed.

As to what context to return if there is none, I'm not sure we should be graceful here an return with some kind of hardcoded context. If the code did not enter a context that it wants to leave or use, then returning a baked in context would make it much harder to debug. Should we throw a config context specific exception here to make this situation clear?

For the ConfigFactory::get() "memory leak", the way it currently implemented, if you enter the same context again later, your values are already cached. If we clear out values when leaving contexts, that could lead to performance issues as well. For example, you are in a user context and enter a free context inside then leave that to return to the user context. Then any values previously cached for that user context would need to be fetched again. Is that better?

Same stands for the multiple copies cached in memory problem.

As for the init() calls invoking events, would it be better to fire those events on concrete value access? Then depending on the number of sub-keys you accessed, it would be even more events fired. BTW locale already attaches to the load event because it needs to read specific override files based on config key. Any suggestions for improvement?

As for combinations of contexts, such as when a global and a language specific content applies (or does not), I'd love if we could do more experimentation and figure out what do we want exactly. It is great we have this in core now that we can experiment with ways people want to use it and make it adapt as needed.

I'm not sure if all these should be in this issue. We are already at the nice 226th comment.

alexpott’s picture

Hmmm... the point of the count > 1 is to ensure that we always have a context... i.e. at least one item in the stack.

plach’s picture

Correctly if I'm wrong, but it seems to me that the previous code was ensuring that there always was at least one element in the stack. Now it can be emptied.

plach’s picture

(crosspost)

Maybe an inline comment to clarify?

pounard’s picture

#228, #227 yeah right, I missed that, sorry.

pounard’s picture

But this raise a consistency issue: why should we keep the last one without even knowing what it is? Practical use case is (I guess) the global override context right? Why isn't that documented?

Gábor Hojtsy’s picture

Now documented.

pounard’s picture

Way better, thanks. I'm still very concern by my other notes:
* Does a new context hides the previous one completly? If so, there's a real consistency problem.
* What amount of memory all this will consume, considering that core alone already loads maybe 50 configuration files on home page with a few content and some language enabled, what will happen when we'll have 20, 50, 100 modules enabled on site?

Gábor Hojtsy’s picture

@pounard:

Does a new context hides the previous one completly? If so, there's a real consistency problem.

I'm not sure why you mean here. Contexts and overrides are in many ways independent. The free context should ensure overrides are not applied (reviewing this in more detail with @alexpott on IRC this turns out not entirely true, indeed!). Other contexts however don't really have a tight control as to what overrides apply. When the config events are fired, event subscribers can override based on values in the current context or based on global values (such as locale does based on global page negotiated language and the global $conf override does based on the global array). So there can indeed be multiple overrides applicable, and I guess it depends on the weights of the event listeners as to which one would win. So in a user context, if there was no user specific override, then there could be a user language specific override, and if that did not happen to be available then there still could be a page language specific override, and if that is not there, there could still be a global $conf override. The current code may not fully support this, but I believe the architecture does. Documented some more things to help you understand for example how the language override is computed based on context.

Gábor Hojtsy’s picture

@pounard: I've opened #1929136: Fix override-free context, move global config overrides back to an event listener for the override/context bugs that I fully agree were oversights. Also opened #1929140: Explore performance of config caching per context for the performance concern that needs more exploration. It depends on various factors. I'll be sure to work on #1929136: Fix override-free context, move global config overrides back to an event listener more later this week, if nobody else picks it up. More info and ideas on #1929140: Explore performance of config caching per context are especially welcome.

I think we should use this issue to get this existing patch with the docs fixes in and then do the more involved discussions on the followups. That would let people jump in without being overwhelmed with the pre-existing 240x comments.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

The changes in #234 look great...

/me must remember to use /** in future!

pounard’s picture

Status: Reviewed & tested by the community » Needs review
+    // If there is a user set in the current context, set the language based on
+    // the preferred language of the user. Otherwise set it based on the
+    // negotiated interface language.

Problem is, it actually should be If there is a user set in the current context stack? I don't see the point if not, this means that any code at any moment can break any context: for example if I am displaying a node, and I have a NodeContext, and the node context doesn't give any language, my langauge manager would revert the current language to default, and forget the user provided one, but I could still want to display field labels within the node using the config translated labels.

The context free context is non existant per definition, instead of fetching a context free context-config we should fetch a real context-free-config (i.e. a config object with no context set, i.e. null or a null implementation, or a global state default context or whatever). Entering a non context is not entering a context, but fetching the instance outside of any context, a non-context should not be stacked into contextes?

I understand how it works, and what it does, but it still sounds very weird to proceed like this. For forms, for example, what you need is a context-free raw instance, but for building the form you might want the context-full config instance at the same time (for example, a context-full instance for fetching a translated form element title, while the context-free instance would give you the non translated value to put into the exact same field). With the current stack having both at the same time seems not possible.

pounard’s picture

Status: Needs review » Reviewed & tested by the community

Oups cross-post @Gabor (sorry I don't have the accent on my keyboard) thanks, I will see into all of these followups.

Gábor Hojtsy’s picture

@pounard: yeah, this issue used to provide a stack-less context system with very different characteristics, and the stack was added due to popular request. It happens that the feedback of those who speak up is considered. I think some of your concerns are resolved with #1929136: Fix override-free context, move global config overrides back to an event listener so that global overrides will not be a context, but it would go back to an event listener as before. Then whether context properties should be inherited across the stack unless specifically set on the level is an interesting point. Let's move there once we clean up the global stuff first in #1929136: Fix override-free context, move global config overrides back to an event listener. Opened #1929176: Possibly inherit context properties between levels of the context stack for the inheritance issue so we don't loose track.

/me feels great that there is more feedback going and we can examine things in more detail :)

pounard’s picture

As a side note, I'm afraid that trying to efficiently stack context values will be difficult, we may experience heavy performance problems if we need to chain calls in different contextes in order to fetch data. But that's another question, see you around in the other issues then, I'll leave this one alone. #240 \o/

Status: Reviewed & tested by the community » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -epic

The last submitted patch, minor-context-updates-234.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

#234: minor-context-updates-234.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, minor-context-updates-234.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

#234: minor-context-updates-234.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +epic

The last submitted patch, minor-context-updates-234.patch, failed testing.

YesCT’s picture

patch has comment fixes.
applies cleanly locally to a fresh git pull --rebase
also installs fine locally.

applies and installs on simplytest.me (dreditor has a simplytest.me button for patches)

retesting again.

YesCT’s picture

Status: Needs work » Needs review

#234: minor-context-updates-234.patch queued for re-testing.

Dave Reid’s picture

Status: Needs review » Needs work

Another thing to add to the followup/cleanup:

  /**
   * Constructs a LocaleConfigSubscriber object.
   *
   * @param \Drupal\Core\Config\Context\ConfigContext $config_context
   *   The config context service.
   * @param \Drupal\Core\Language\LanguageManager $language_manager
   *   The language manager service.
   */
  public function __construct(LanguageManager $language_manager, ContextInterface $config_context) {
    $this->languageManager = $language_manager;
    $this->defaultConfigContext = $config_context;
  }

These documented parameters are in reverse order compared to reality.

pounard’s picture

Another note, in config.inc there is several methods which are missing a * in comments first lines, I can't remember which ones exactly.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.97 KB

Fixed the ones pointed out by @Dave Reid and @pounard. Should be back to RTBC as per above.

YesCT’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.09 KB
8.6 KB

I went back through #213 and the recent follow-up to look for any other standards things...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Those look good too. I'm sure we'll find plenty others later just like with any other area of Drupal. Let's get these fixed and get to more substantial fixes in #1929136: Fix override-free context, move global config overrides back to an event listener :)

podarok’s picture

#251 +1 RTBC

pounard’s picture

+1 RTBC

Status: Reviewed & tested by the community » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -epic

The last submitted patch, minor-context-updates-251.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +epic

#251: minor-context-updates-251.patch queued for re-testing.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc as per #252, #253, #254 :) ... there was a random testbot failure... and this patch only updates comments and formatting!

Gábor Hojtsy’s picture

Assigned: catch » jhodgdon

This is all docs, so assigning to jhodgdon instead of catch.

alexpott’s picture

The docs changes here look fantastic! +1 for rtbc lets get this done... (after all some of this is my bad)

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Needs work

This looks almost perfect! I'd be ready to commit it if
LocaleConfigSubscriber::onKernelRequestSetDefaultConfigContextLocale() docs had:
- First line needs to start with Sets not Set
- @param is missing the variable name of the parameter ($event) at end of line

And in general, we try to use real words in documentation, so I'd be more comfortable if everywhere it says "config" it said "configuration" instead.

If this was a code patch, I'd probably let these go, but since this patch is purely a docs cleanup, let's get it right. Thanks!

das-peter’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
8.7 KB

Re-rolled to meet the mentioned points in #260.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

clemens.tolboom’s picture

My installation (drush sql-drop + browser install) fails with apache error log

PHP Fatal error:  Class 'Drupal\\Core\\EventSubscriber\\ConfigGlobalOverrideSubscriber' not found in /Users/clemens/Sites/drupal/d8/www/core/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 142

due to the commit from #221

0612c66 - catch <catch@35733.no-reply.drupal.org> Feb 27, 2013

Changing

$container->register('config_global_override_subscriber', 'Drupal\Core\EventSubscriber\ConfigOverrideSubscriber')

into (removing 'global_')

$container->register('config_override_subscriber', 'Drupal\Core\EventSubscriber\ConfigOverrideSubscriber')

makes the installer work. Should this be a new issue?

Gábor Hojtsy’s picture

@clemens.tolboom: did you also drop your files dir? As of #1929136: Fix override-free context, move global config overrides back to an event listener ConfigOverrideSubscriber is not even in core anymore.

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
YesCT’s picture

Status: Needs work » Needs review
FileSize
8.26 KB

Here is the reroll. There were three conficts, I kept two from this issue, one added a helper function and changed a comment, another changed a comment. the third, I kept the upstream since the other issue changed the functionality so kept their new comment about it.

Status: Needs review » Needs work

The last submitted patch, minor-context-updates-266.patch, failed testing.

alexpott’s picture

+++ b/core/modules/user/lib/Drupal/user/UserConfigContext.phpundefined
@@ -26,8 +26,16 @@ class UserConfigContext extends ConfigContext {
+  public function setUuid() {
+    // Use the user's uuid to identify the config context.
+    $this->uuid = $this->get(self::USER_KEY)->uuid();
+  }

This function needs to go!

YesCT’s picture

oops. yep. #1929136: Fix override-free context, move global config overrides back to an event listener took it out and this issue did NOT add it!
patch coming.

YesCT’s picture

Status: Needs work » Needs review
FileSize
758 bytes
8.03 KB
YesCT’s picture

@alexpott spotted some more. I double checked these are from lines introduced from #213.
Just adding \ to the Implement \Drupal...

Status: Needs review » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -epic

The last submitted patch, minor-context-updates-271.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

#271: minor-context-updates-271.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +epic

The last submitted patch, minor-context-updates-271.patch, failed testing.

YesCT’s picture

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContext.phpundefined
@@ -1,4 +1,4 @@
-<?php
+

what did I do? heh. fixing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
315 bytes
9.93 KB
YesCT’s picture

FileSize
1011 bytes

by the way, question regarding the namespace stuff...

I know use statements do not have the \

what about these (see attached).

jhodgdon’s picture

RE #277 - http://drupal.org/coding-standards/docs#classes -- if you use a namespace in docs, start with backslash.

The second line in your interdiff is code and I have no comment on that.

YesCT’s picture

from http://drupal.org/node/1353118

When specifying a class name in a string, use its full name including namespace, without leading \.
Escape the namespace separator in double-quoted strings: "Drupal\\Context\\ContextInterface"

Do not escape it in single-quoted strings: 'Drupal\Context\ContextInterface'

As stated elsewhere, single-quoted strings are generally preferred.

So.. here is another. these should also just be changed lines from #213

YesCT’s picture

@jhodgdon Yeah, that 'Defaults to' is confusing, because it's giving the value of the variable class, ... so I guess that could go either way. (Technically, the Defaults to in the @param is not needed. See #1916652: defaults to in 1354 doc standards for optional args)

clemens.tolboom’s picture

Status: Needs review » Reviewed & tested by the community

@Gábor Hojtsy: just did a git pull and now 'cannot reproduce'. So #1929136: Fix override-free context, move global config overrides back to an event listener did the trick. Tnx.

clemens.tolboom’s picture

Status: Reviewed & tested by the community » Needs review

wtf: i did not do RTBC?!?

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Config/Context/ConfigContextFactory.phpundefined
@@ -49,7 +49,7 @@ public function __construct(EventDispatcher $event_dispatcher) {
-      $class = "Drupal\\Core\\Config\\Context\\ConfigContext";
+      $class = 'Drupal\Core\Config\Context\ConfigContext';

Personally I much prefer this as you can cut and paste the value into an IDE and automatically open the file that contains the class.

Patch looks good - thanks you for work!

jhodgdon’s picture

Everything in this patch looks good to me too. Thanks! I am not sure if I'll have time to commit it anytime soon (going to be out of town for a few days) so if someone else wants to, please do. If no one else wants to, assign to me and I'll take care of it sometime next week.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks all.

Tor Arne Thune’s picture

Title: Followup: Introduce config context to make original config and different overrides accessible » Introduce config context to make original config and different overrides accessible
Priority: Normal » Major
Gábor Hojtsy’s picture

Issue tags: -sprint

Perfect, thanks! Now I can move this off of the D8MI focus board. Great work everyone!

Status: Fixed » Closed (fixed)
Issue tags: -Regression, -Configuration system, -D8MI, -language-config, -epic

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary, added a follow-ups section and a couple follow-ups. might need to open more.

Gábor Hojtsy’s picture

Tags :)