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
- 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.
- Introduce a context factory (\Drupal\Core\Config\Context\ConfigContextFactory) that can be used to create context object instances.
- Set a default configuration context for the page request, and use different configuration context objects for different purposes (administration, send emails to users, etc).
- 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
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
Comment | File | Size | Author |
---|---|---|---|
#279 | minor-context-updates-278.patch | 11.82 KB | YesCT |
#279 | interdiff-276-278.txt | 2.04 KB | YesCT |
#277 | interdiff-bytheway.txt | 1011 bytes | YesCT |
#276 | minor-context-updates-276.patch | 9.93 KB | YesCT |
#276 | interdiff-271-276.txt | 315 bytes | YesCT |
Comments
Comment #1
Jose Reyero CreditAttribution: Jose Reyero commentedFirst version of the patch that, to start with, should fix the issue of getting overridden values in system settings forms.
Comment #2
Gábor HojtsyPut this on the sprint.
Comment #3
Gábor HojtsyComment #4
Jose Reyero CreditAttribution: Jose Reyero commentedNew version of the patch that fixes some typo, adding the right tag for Configuration sytem
Comment #5
Gábor HojtsyElevating to major as it is a cornerstone to have any CMI language support in core.
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a documentation review of the #4 patch for when it next gets re-rolled.
This docblock needs a one line description and this second paragraph should wrap at 80 characters or less.
Can we add a type hint here... I presume it is string type?
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.
In the field patches, they refer to variables like $context as 'array $context = array()'. I think that should be done here.
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?
This needs an '(optional) ' added to start of description as well as an explanation of what happens in default case.
I believe that this docblock needs a @return directive with type hint for what $this->factory is.
Missing a @param directive here.
I am pretty sure arguments to a constructor need @param directives.
This needs a @return directive.
Needs a @return directive with type hint.
Needs a type hint and start description with '(optional)'.
What type of data is returned here? Is it an array?
Needs a string type hint for @param.
The @return directive has wrong type hint. I think it should be ConfigFactory as a type hint.
Should be 'Dispatches'. Also need to type hint #config_event_name. I think it is a string.
Needs to wrap at 80 characters.
Should be 'Deletes'.
These comments need to be rewrapped better for 80 character limit.
Missing newline at end of file.
Another missing newline at end of file.
Comment #7
tstoecklerThis could use the cool new ?: pattern I think. I.e.
$storage = $storage ?: d_c()...;
Leaving at needs review.
Comment #8
c31ck CreditAttribution: c31ck commentedPatch for the documentation remarks from #6.
Comment #9
c31ck CreditAttribution: c31ck commentedBetter patch and interdiff, disregard the patch in #8.
Comment #10
amonteroShould not $config_event_name be declared as string in the docblock as it's done elsewhere?
Also, patch no longer applies to HEAD (7b5fbf3).
Comment #11
amonteroOps, comment preview was saying I was removing "Configuration Management" tag (?). Undo.
Comment #12
Gábor HojtsyAny concerns from the CMI point of view? Any features missing or suggested to be better implemented?
Comment #13
Gábor HojtsyHopefully easier to follow title.
Comment #14
gddThis 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.
Comment #15
Gábor HojtsyThanks for the review and support! :) More reviews are welcome of course :)
Comment #16
Jose Reyero CreditAttribution: Jose Reyero commentedRerolled 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().
Comment #17
gddTagging for feature freeze
Comment #18
vasi1186 CreditAttribution: vasi1186 commentedI think this issue implements a very nice feature!
Here is some feedback from my point of view:
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:
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:
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.
Comment #19
YesCT CreditAttribution: YesCT commentedI 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?
Comment #20
Gábor HojtsyI think the updates in #16 look good.
Comment #21
YesCT CreditAttribution: YesCT commented#16: 1763640-config_context-16.patch queued for re-testing.
Comment #22
YesCT CreditAttribution: YesCT commented#16: 1763640-config_context-16.patch queued for re-testing.
Comment #24
Jose Reyero CreditAttribution: Jose Reyero commented@vasi1186 #18
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.
Comment #25
Jose Reyero CreditAttribution: Jose Reyero commentedReworked 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():
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.
Comment #26
Jose Reyero CreditAttribution: Jose Reyero commentedComment #26.0
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated issue summary.
Comment #26.1
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated for changes in #25
Comment #26.2
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated issue summary.
Comment #27
Gábor HojtsyCreated this class picture for the patch, please include in summary :)
Comment #28
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated title and summary once again.
Comment #28.0
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated issue summary.
Comment #28.1
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated issue summary.
Comment #28.2
Gábor HojtsyUpdated issue summary.
Comment #29
Gábor HojtsyTagging as a regression, explained in the updated issue summary with the first para:
Comment #30
YesCT CreditAttribution: YesCT commentedLooks like next steps for this are
Comment #31
das-peter CreditAttribution: das-peter commentedLooks 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
Comment #32
das-peter CreditAttribution: das-peter commentedI've figured out the location that needed to be adjusted. Hope this is done correct.
Comment #33
das-peter CreditAttribution: das-peter commentedFixed some minor coding standard thingies.
Comment #34
YesCT CreditAttribution: YesCT commenteddoes not apply. I followed instructions at: http://drupal.org/patch/reroll
I'm re-rolling.
conflict:
I merged it as:
No interdiff since this is a re-roll.
Comment #36
YesCT CreditAttribution: YesCT commentedI'm rerolling this again ( for #1849004: One Service Container To Rule Them All I think ).
Comment #37
YesCT CreditAttribution: YesCT commentedPreface, 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.
unrelated accidental change.
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.
The factory has a reference to the context which has a reference to the storage. Hmm. OK.
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.
major changes to this small file, so I'm going to fix the file comment too.
Comment #39
YesCT CreditAttribution: YesCT commentedI 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
Comment #40
Jose Reyero CreditAttribution: Jose Reyero commentedRerolled 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.
Comment #41
YesCT CreditAttribution: YesCT commentedOK. I think we need to see if the patch from 36/37 applies to that re-roll.
Comment #42
YesCT CreditAttribution: YesCT commentedOr, decide that those changes are totally ok for a follow-up, which I'm ok with.
Comment #43
Jose Reyero CreditAttribution: Jose Reyero commented@YesCT, sorry I was in a rush this morning and missed that ones.
That will be in next reroll if we need one more.
Comment #44
gddIs 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'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.
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.
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.
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.
Comment #45
Jose Reyero CreditAttribution: Jose Reyero commentedAbout context in config_sync_changes:
@heyrocker,
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 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.
Ok.
About hook_context_factory().
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.
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.
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)
Comment #46
Jose Reyero CreditAttribution: Jose Reyero commented@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.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedmy first reaction to this patch is - are we sure this is necessary?
doesn't something like the below function to load configuration for an arbitrary language cover this use case?
don't the existing load events allow modules to do whatever they like to config values at runtime:
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?
Comment #48
Gábor Hojtsy@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.
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.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedGábor - thanks for the reasonable reply.
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?
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.
Comment #50
Gábor HojtsyWe 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.
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?
Comment #51
Gábor HojtsyConsider 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.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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.
Comment #53
Gábor Hojtsy@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 :)
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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.
Comment #55
catch@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.
Comment #56
Gábor Hojtsy@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.
Comment #57
catchThat 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.
Comment #58
Jose Reyero CreditAttribution: Jose Reyero commented@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.
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commentedre #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:
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.
Comment #60
Gábor HojtsyAll 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:
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?
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
Comment #62
Gábor HojtsyYeah, 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.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedhere'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.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedattached 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.
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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.
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedok, here's a patch that set's the user's language on the ConfigFactory in user_mail().
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedok, typo in Config object, should be getLanguage() not getLanuage(), that should fix at least the locale config override test.
Comment #72
katbailey CreditAttribution: katbailey commentedSomewhat 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 ;-)
Comment #73
Jose Reyero CreditAttribution: Jose Reyero commented@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.
Comment #74
sunThe 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 toconfig()
(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).
Comment #75
Gábor Hojtsy@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.
Comment #76
Anonymous (not verified) CreditAttribution: Anonymous commentedre. #75, that problem predates context, and deserves it's own issue IMO.
Comment #77
Anonymous (not verified) CreditAttribution: Anonymous commentedhere'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.
Comment #78
Gábor HojtsyWhat is the role of these language_default()s here?
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).
TODO should be @todo right?
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.
Comment #79
sunI 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.)
Comment #80
Jose Reyero CreditAttribution: Jose Reyero commentedAbout @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.
Comment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedattached 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?
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous commenteddoh! attached patch tells php about the Language param config_factory_set_language().
Comment #84
Gábor HojtsyI 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.
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.
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.
Comment #85
Gábor HojtsyComment #86
Gábor Hojtsy@beejeebus opened a separate issue for the "admin context" aka raw config data accessibility at #1868028: Raw (original) config data is not accessible.
Comment #87
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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?
Comment #88
chx CreditAttribution: chx commentedOK 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 :/
Comment #89
Gábor HojtsySlightly 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.
Comment #90
Jose Reyero CreditAttribution: Jose Reyero commented@beejeebus,
Sounds good. About simplifications see #46
Comment #91
Gábor HojtsyRefiling as task given we are plugging in missing pieces (eg. the bug at #1868028: Raw (original) config data is not accessible).
Comment #92
catchWhat 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.
Comment #93
Gábor Hojtsy@catch: I'm not sure if you propose to change how system_config() behaves but renaming it to config_raw() is totally fine IMHO.
Comment #94
catchFrom 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.
Comment #95
chx CreditAttribution: chx commentedIMO 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.
Comment #96
Gábor Hojtsy@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?!
Comment #97
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's a reroll of #40, hopefully i haven't butchered it too much, to get things rolling again.
Comment #98
Anonymous (not verified) CreditAttribution: Anonymous commentedand 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'.
Comment #99
Anonymous (not verified) CreditAttribution: Anonymous commentedchx pointed out that we don't want ->load() any more in config() - removed that.
Comment #100
chx CreditAttribution: chx commentedTo 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?
Comment #102
chx CreditAttribution: chx commentedMaybe the #process callback should add the overridden value to the description to decrease the wtf.
Comment #103
Gábor Hojtsy@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.
Comment #104
Anonymous (not verified) CreditAttribution: Anonymous commented- 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
Comment #105
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #107
Anonymous (not verified) CreditAttribution: Anonymous commented- 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()
Comment #109
Anonymous (not verified) CreditAttribution: Anonymous commented- 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
Comment #111
Anonymous (not verified) CreditAttribution: Anonymous commentedsigh, #109 was full of stupid.
Comment #112
Anonymous (not verified) CreditAttribution: Anonymous commentedsigh, #109 was full of stupid.
Comment #113
Gábor Hojtsy@chx, @catch, @Jose, @sun, @heyrocker: your review feedback would be very welcome :)
Comment #114
chx CreditAttribution: chx commentedI 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?Comment #116
Gábor Hojtsy@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.
Comment #117
Jose Reyero CreditAttribution: Jose Reyero commentedI think this is looking pretty well, just some issues that should be easy to fix:
Issues with the caching keys:
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.
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.
@chx,
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.
Comment #118
gddHere'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.
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.
Comment #119
Gábor HojtsyI 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 :)
Comment #119.0
Gábor HojtsyFix code example and image.
Comment #120
Gábor HojtsyUpdated 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 :)
Comment #121
chx CreditAttribution: chx commented> 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.
Comment #122
Jose Reyero CreditAttribution: Jose Reyero commentedre @chx #121,
Agreed, you're right it won't work anyway, so we should remove that reference. Back to "needs work".
Comment #123
Gábor Hojtsy@chx/@Jose Reyero: do you suggest we copy $conf instead of referencing? @Jose: are you working on the update?
Comment #124
Jose Reyero CreditAttribution: Jose Reyero commentedRerolled 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
Comment #126
Gábor HojtsyAll the fails are in ConfigOverrideTest, so probably highly related to the patch.
Comment #127
Jose Reyero CreditAttribution: Jose Reyero commentedYeah, obviously, since we are not passing global conf by reference now, we need to refresh it if we update the values.
Fixed the test.
Comment #128
Gábor HojtsyThanks for the update, chx's concern was resolved, so looks good to go again :)
Comment #129
catchWould 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.
Comment #130
aspilicious CreditAttribution: aspilicious commentedI think this is Contains ... now
\Drupal\...
Needs newline
These things happen everywhere in the patch. Please fix them :). Keep going!
Comment #131
Gábor HojtsyAll 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):
Comment #132
Gábor HojtsyRerolled for the "Contains" and the missing newlines.
Comment #133
YesCT CreditAttribution: YesCT commentedhttp://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
Comment #134
Gábor HojtsyAll right then, changed @param Drupal\... to @param \Drupal\... and Overrides Drupal\... to Overrides \Drupal\....
Comment #135
Dries CreditAttribution: Dries commentedI 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.
Comment #136
Gábor Hojtsy@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?
Comment #137
catchGeneral page before/after would be plenty yeah.
Comment #138
Anonymous (not verified) CreditAttribution: Anonymous commentedunless someone beats me to it, i'll post some xhprof data tomorrow.
Comment #139
webchickLooks like this is still needs review.
Comment #140
Gábor Hojtsy#134: 1763640-config-context-134.patch queued for re-testing.
Comment #142
Gábor Hojtsy@beejeebus: have you been able to look into this? Thanks!
Comment #143
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's a reroll, couldn't get #134 to apply.
Comment #145
Gábor Hojtsy#143: 1763640-143-config-context.patch queued for re-testing.
Comment #147
Anonymous (not verified) CreditAttribution: Anonymous commentedok, this patch should get beyond installation on the bot.
Comment #149
Anonymous (not verified) CreditAttribution: Anonymous commented#147: 1763640-config-context-147.patch queued for re-testing.
Comment #150
Anonymous (not verified) CreditAttribution: Anonymous commentedalright. NFI idea why it failed one time then passed the next, but now i'll do some runs and post some xhprof data.
Comment #151
Gábor Hojtsy@justin.randell: Thanks, any xhprof results? :)
Comment #152
Anonymous (not verified) CreditAttribution: Anonymous commentedsorry, nope. i'm not going to get to this soon :-(
Comment #153
gddI 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.
Comment #154
sunHm. 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:
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".
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.
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.
Minor: Looks like we could cache the class name of the active context?
I wonder whether it is valid to "rename" a config object from one context to another? (which essentially seems to be happening here?)
Trivial: Appears to be unused.
Can we globally rename $name to $context_id?
This caused me quite some confusion... (see below)
$name is not TRUE, so the actual usage conflicts with its documentation?(scratch that)
Both of these should be injected...?
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.
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?
Tests should always use $this->container() instead of
drupal_container()
.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? ;)
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. :-/
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?
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.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?
contextless_config()
(or similar) would be a bit more self-descriptive..."system" doesn't really have any meaning.
Either I'm missing something, or this info hook must be completely obsolete...? I can't any other implementation in this patch?
I really thought of this code flow to be just this:
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?
Comment #155
YesCT CreditAttribution: YesCT commented#147: 1763640-config-context-147.patch queued for re-testing.
Comment #157
Gábor HojtsyIt is hard to believe Drupal\openid\Tests\Upgrade\OpenIDAuthmapUpgradePathTest fails would be related :)
Comment #158
Gábor Hojtsy#147: 1763640-config-context-147.patch queued for re-testing.
Comment #159
plachBack to RTBC.
Comment #160
alexpott#147: 1763640-config-context-147.patch queued for re-testing.
Comment #162
alexpottThe patch attached tries to address the architectural issues brought up by sun in #154
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:
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.
Comment #163
alexpottPlease testbot :)
Comment #165
alexpottPatch attached resolves the test failures by
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 duringsystem_update_8046()
The interdiff is not a precise interdiff to #162 as this patch required a reroll but it is indicative of the changes.
Comment #167
alexpottThe 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:
I'm bamboozled as to why this has only caused 1 test failure...
Comment #168
alexpottThis patch should fix the test failure in #165...
The
config.context
service needs to be tagged withpersist
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 toconfig.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 :)
Comment #169
Anonymous (not verified) CreditAttribution: Anonymous commentedi 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:
maybe the changes were trying to address sun's later point:
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.
Comment #170
Gábor Hojtsy@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
Comment #171
alexpottThe 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.
Comment #172
Gábor Hojtsy@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.
Comment #173
alexpottThe patch attached re-implements the scoping as suggested #169 and #170.
The patch borrows from sun's suggestion for the DX.
Comment #174
Gábor Hojtsy@beejeebus, @sun: any comments / concerns on the fixes that @alexpott proposed based on your previous comments?
Comment #175
Anonymous (not verified) CreditAttribution: Anonymous commented#173 looks good to me.
Comment #176
Anonymous (not verified) CreditAttribution: Anonymous commenteddiscussed this with alexpott in IRC. i think this is ready to fly.
Comment #177
Gábor HojtsyAll 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.
Comment #177.0
Gábor HojtsyUpdate for config_factory_set_context()
Comment #177.1
Gábor HojtsyUpdated issue summary.
Comment #178
Gábor HojtsyI 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):
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 :)
Comment #179
Gábor HojtsyUh, tag loss bug.
Comment #180
Dries CreditAttribution: Dries commentedI'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.
I'd prefer to see the last line split in two lines:
Otherwise, this looks pretty close to being RTBC. Good work.
Comment #181
alexpott1. 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...
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.
Comment #182
Gábor Hojtsy@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:
Ideally more something like:
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.
Comment #183
alexpottPatch attached implements new function
config_context_enter()
that excepts a single argument of either a service on the DIC or a class name so...... and ...
Comment #185
Gábor HojtsyMassive test fails seem to be unrelated. Eg. errors with "possibly out of disk space".
Comment #186
Gábor Hojtsy#183: 1763640-config-context-183.patch queued for re-testing.
Comment #188
alexpottThe 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 :(Comment #189
alexpottComment #190
Gábor HojtsyLooks like that was intermittent then :) Fixes Dries' concerns elegantly IMHO.
Comment #191
Gábor HojtsyErm, 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.
Comment #192
alexpottAnd here's the config_context_leave() function...
Comment #193
Gábor HojtsyHuge 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!
Comment #195
alexpott#192: 1763640-config-context-192.patch queued for re-testing.
Comment #196
YesCT CreditAttribution: YesCT commentedpasses, so setting back to rtbc according to @Gabor in #193
Comment #197
alexpottOnce 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.
Comment #199
alexpott#197: 1763640-config-context-197.patch queued for re-testing.
Comment #201
alexpott#197: 1763640-config-context-197.patch queued for re-testing.
Comment #202
Gábor HojtsyStill looks good!
Comment #203
Gábor HojtsyAlso adding avoid commit conflicts to try and avoid further painful rerolls.
Comment #204
webchickEven though it looks like Dries's feedback was resolved (thanks!), assigning to catch for one last look at the architecture.
Comment #205
plachRestoring lost tag.
Comment #206
Gábor Hojtsy#197: 1763640-config-context-197.patch queued for re-testing.
Comment #207
catchWhy can't this use config_context_leave(). I know there's the reset('manifest') but that could be a parameter?
This looks like it could move to a container-aware service?
I like config.context.free, it's very self-explanatory which the 'system' context wasn't.
This is the same as earlier, could definitely do with consolidation into a helper.
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.
$name needs documenting.
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.
Comment #208
Gábor Hojtsy@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!
Comment #209
Gábor HojtsyRestore lost tags.
Comment #210
catchHmm another thing here which makes me uncomfortable:
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.
Comment #211
plach(What would Gabor do?)
Restore lost tag.
Comment #212
alexpottImplemented 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.
Comment #213
alexpottPatch to address @catch's comment in #210
Comment #214
plachThese tags don't stick :)
Comment #215
Gábor HojtsyChanges 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 :)
Comment #216
YesCT CreditAttribution: YesCT commentedIs there anything in the recent changes we could update in the issue summary, in the meantime?
Comment #216.0
YesCT CreditAttribution: YesCT commentedHeavily updated issue summary based on latest patches.
Comment #217
Gábor Hojtsy@YesCT: just updated summary with current code examples. http://drupal.org/node/1763640/revisions/view/2560078/2578696
Comment #218
YesCT CreditAttribution: YesCT commentedfollow-up for part of #207 and #212
#1926968: move config_context_enter to a container-aware service
Comment #219
YesCT CreditAttribution: YesCT commentedfollow-up for the last comment from #207 and from #208
#1927006: make a generic top-level context, make user context extend that generic entity context
Comment #219.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #220
Gábor Hojtsy#213: 1763640-config-context-213.patch queued for re-testing.
Comment #221
catchLooks much better. Committed/pushed to 8.x, thanks!
Comment #222
Gábor HojtsyWoot, thanks!
Comment #223
Gábor HojtsyAdded 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 :)
Comment #224
Gábor HojtsyComment #225
pounardRandom notes from a short review done with latest git version pulled.
ConfigFactory.php line 163:
Can just be:
array_pop() does not emit any warning when trying to pop an empty array.
ConfigFactory.php line 170:
Missing * in first line.
ConfigFactory.php line 147:
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:
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?
Comment #226
Gábor Hojtsy@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.
Comment #227
alexpottHmmm... the point of the
count > 1
is to ensure that we always have a context... i.e. at least one item in the stack.Comment #228
plachCorrectly 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.
Comment #229
plach(crosspost)
Maybe an inline comment to clarify?
Comment #230
pounard#228, #227 yeah right, I missed that, sorry.
Comment #231
pounardBut 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?
Comment #232
Gábor HojtsyNow documented.
Comment #233
pounardWay 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?
Comment #234
Gábor Hojtsy@pounard:
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.
Comment #235
Gábor Hojtsy@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.
Comment #236
alexpottThe changes in #234 look great...
/me must remember to use /** in future!
Comment #237
pounardProblem 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.
Comment #238
pounardOups cross-post @Gabor (sorry I don't have the accent on my keyboard) thanks, I will see into all of these followups.
Comment #239
Gábor Hojtsy@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 :)
Comment #240
pounardAs 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/
Comment #242
YesCT CreditAttribution: YesCT commented#234: minor-context-updates-234.patch queued for re-testing.
Comment #244
YesCT CreditAttribution: YesCT commented#234: minor-context-updates-234.patch queued for re-testing.
Comment #246
YesCT CreditAttribution: YesCT commentedpatch 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.
Comment #247
YesCT CreditAttribution: YesCT commented#234: minor-context-updates-234.patch queued for re-testing.
Comment #248
Dave ReidAnother thing to add to the followup/cleanup:
These documented parameters are in reverse order compared to reality.
Comment #249
pounardAnother note, in config.inc there is several methods which are missing a * in comments first lines, I can't remember which ones exactly.
Comment #250
Gábor HojtsyFixed the ones pointed out by @Dave Reid and @pounard. Should be back to RTBC as per above.
Comment #251
YesCT CreditAttribution: YesCT commentedI went back through #213 and the recent follow-up to look for any other standards things...
Comment #252
Gábor HojtsyThose 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 :)
Comment #253
podarok#251 +1 RTBC
Comment #254
pounard+1 RTBC
Comment #256
alexpott#251: minor-context-updates-251.patch queued for re-testing.
Comment #257
alexpottBack to rtbc as per #252, #253, #254 :) ... there was a random testbot failure... and this patch only updates comments and formatting!
Comment #258
Gábor HojtsyThis is all docs, so assigning to jhodgdon instead of catch.
Comment #259
alexpottThe docs changes here look fantastic! +1 for rtbc lets get this done... (after all some of this is my bad)
Comment #260
jhodgdonThis 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!
Comment #261
das-peter CreditAttribution: das-peter commentedRe-rolled to meet the mentioned points in #260.
Comment #262
Gábor HojtsyLooks good, thanks!
Comment #263
clemens.tolboomMy installation (drush sql-drop + browser install) fails with apache error log
due to the commit from #221
Changing
into (removing 'global_')
makes the installer work. Should this be a new issue?
Comment #264
Gábor Hojtsy@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.
Comment #265
YesCT CreditAttribution: YesCT commentedI'm re-rolling this since #1929136: Fix override-free context, move global config overrides back to an event listener got committed.
Comment #266
YesCT CreditAttribution: YesCT commentedHere 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.
Comment #268
alexpottThis function needs to go!
Comment #269
YesCT CreditAttribution: YesCT commentedoops. 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.
Comment #270
YesCT CreditAttribution: YesCT commentedComment #271
YesCT CreditAttribution: YesCT commented@alexpott spotted some more. I double checked these are from lines introduced from #213.
Just adding \ to the Implement \Drupal...
Comment #273
YesCT CreditAttribution: YesCT commented#271: minor-context-updates-271.patch queued for re-testing.
Comment #275
YesCT CreditAttribution: YesCT commentedwhat did I do? heh. fixing.
Comment #276
YesCT CreditAttribution: YesCT commentedComment #277
YesCT CreditAttribution: YesCT commentedby the way, question regarding the namespace stuff...
I know use statements do not have the \
what about these (see attached).
Comment #278
jhodgdonRE #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.
Comment #279
YesCT CreditAttribution: YesCT commentedfrom http://drupal.org/node/1353118
So.. here is another. these should also just be changed lines from #213
Comment #280
YesCT CreditAttribution: YesCT commented@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)
Comment #281
clemens.tolboom@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.
Comment #282
clemens.tolboomwtf: i did not do RTBC?!?
Comment #283
alexpottPersonally 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!
Comment #284
jhodgdonEverything 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.
Comment #285
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks all.
Comment #286
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #287
Gábor HojtsyPerfect, thanks! Now I can move this off of the D8MI focus board. Great work everyone!
Comment #288.0
(not verified) CreditAttribution: commentedUpdated issue summary, added a follow-ups section and a couple follow-ups. might need to open more.
Comment #289
Gábor HojtsyTags :)