Problem/Motivation
With our current system, it is difficult to define mappings and dependencies between certain systems. We require a system to handle not only the dynamic creation of objects, but also the definitions of what services we require.
Proposed resolution
Dependency Injection is a method to help decouple systems. Symfony provides a Dependency Injection component which we should use to handle our global objects, as well as define the mappings of which classes we want to use.
As a demonstration of how this API is used, this issue moves the $language_interface, $language_content, and $language_url global objects into this system everywhere they're used in core/includes/*, but defers to follow-up issues the cleanup of these global variables from the rest of core and the migration of other globals.
Remaining tasks
Review and get this patch RTBC... Once this patch is in, we can consider the following:
- #1510686: Replace remaining references to global $language_interface with drupal_container()
- #1512310: Replace $language_url with Dependency Injection
- #1512308: Replace $language_content with Dependency Injection
- #1511482: Bootstrap for the Dependency Injection Container
- #1512424: Add a LanguageInterface, and property setters/getters to Language class
- #1515196: Standardize retrieval of "global" variables to be at beginning of function
- Once the Kernel patch is in, bring the
drupal_container()
Container instance in as a property of the Kernel object so that we have proper scope of all our Drupal objects - Use an Event Dispatcher/module hook to have objects and classes register themselves as services
User interface changes
None.
API changes
This brings in a ContainerBuilder singleton which allows us to handle the mappings between service classes and objects. There is a drupal_container() function which returns the desired service object. We will probably get rid of that once the Kernel patch goes in.
Alternative solutions
We considered using Pimple as a simpler alternative to Symfony's container, but decided on Symfony's because it has two important features not used in this initial patch but that we expect to need in followup work:
- Its configuration can be serialized (Pimple uses closures that cannot be serialized), which makes it possible to dump configuration to disk/database/whatever and reload easily without a massive rebuild process on each request.
- It has support for scoping to force regeneration of various items that depend on other items, which allows for easy context-switching within a request (e.g., for per-block subrequests or other code that wants to context switch). Pimple does not have any such built-in capability.
Comment | File | Size | Author |
---|---|---|---|
#142 | drupal8.path-test.142.patch | 543 bytes | sun |
#136 | 1497230-136.patch | 4.87 KB | effulgentsia |
#128 | translation_test.patch | 1.29 KB | RobLoach |
#124 | 1497230-124.patch | 1.25 KB | effulgentsia |
#113 | 1497230-113-bootstrap-error-handling.patch | 561 bytes | Anonymous (not verified) |
Comments
Comment #1
RobLoachHad the wrong Container object. This one should be it.
Comment #2
RobLoachAdding D8MI since the initial example goal for this would be to replace the global $language_interface with $container-get('language_interface').
Comment #3
Crell CreditAttribution: Crell commentedThis should be use'd and then referenced with its short name.
I am also not sure I'd really subclass the container. The container should NOT manage its own singleton. It should be managed externally by... this very function, which should then be returning the container instance, not forwarding a method.
Private variables are against coding standards. Use protected. Or, in this case just don't have this class, as we don't need it. :-)
If we're going to turn the language information into a proper class, it should be a proper class with methods rather than public values that could change at any time without warning.
One of the reasons to use proper methods is because supplying both English and en (or whatever for the language in question) is needlessly redundant.
We also need to determine how one registers a service with the container. My thinking at present, at least short-term, is a module-specific class named for the module that contains a setup method that will register whatever is needed on the container. (Since it's not a hook we can use it as soon as the class loader is available.) That's similar to what Symfony does, but it has a cached multiple pass approach for better performance. We'll get there, but one step at a time.
Comment #4
plachTagging as top priority D8MI stuff.
Comment #5
Gábor HojtsyAgreed with @Crell on language related points. language_save() takes a language object and fills in the missing pieces much like node_save() does, and it fills in language names based on language code even if language name is not provided. So we don't have a OO interface for this, but it would be a step back to have this constructor approach (again). Also, regarding the namespace placement, we have language.module in Drupal 8 handling all language stuff, and we might rename locale module as well to "interface_translation" or somesuch. So "Locale" as a namespace is not really D8-compliant. Suggested: Language. I can't vouch for the rest of the patch.
Comment #6
RobLoachI agree. I'd really opt to have as little public functions and static variables as possible. We want PSR-0 compatibility, and that's why I went with the Singleton approach until the Kernel patch is in. Once the Kernel is in, we'll be able to have the $container ContainerBuilder instance as an instance in the Kernel.
Once the Kernel
I had these as public variables to keep backwards compatibility and my sanity. I was assuming that once we get the initial dependency injection stuff in, we could clean it all up and use Language properly.
This is another thing that's blocked by the Kernel patch. Symfony uses a dispatcher to register events to make the ContainerBuilder define and register the objects themselves. So, once the Kernel is in, we can have something like....
Since the dispatcher would tell the dispatcher about the container, we could have the dispatcher register the services. #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel would be super duper nice.
Drupal\Core\Language sounds good. Thanks for taking a look, Gabor! The ContainerBuilder also has documentation about namespacing the variables in the container with a "." separator, so maybe something like...
Again, the name of the service itself doesn't matter, as long as we decide something and stick with it.
Comment #7
Crell CreditAttribution: Crell commentedBased on my conversations with Fabien, I'm not sure I'd make the container an attribute of the kernel, especialy since the kernel will likely live in the container. I also would not make the kernel events the place for other modules to register container objects, as that tightly couples the kernel and container. The container should already be built by the time the kernel does its thing.
As far as naming conventions go, Symfony uses dot-separated IDs. We should probably do the same.
Comment #8
RobLoachShould we use a Factory class instead of a static "drupal_container()" function? Using a static means that there could only be one Container instance.
Comment #9
Crell CreditAttribution: Crell commentedNo, just one uber-container-instance, which in practice will be the case anyway. There's nothing stopping you from creating your own instance somewhere if you want. And, in practice, it's better practice to pass the container to objects that will need the container as an injection to the object rather than having them call container() anyway. It's a transition hack, even if it's one that survives D8 code freeze. :-)
Comment #10
EclipseGc CreditAttribution: EclipseGc commentedI'm 100% with crell on #9 here.
Comment #11
RobLoachSo nice...
Comment #12
RobLoachEclipseGC asked for a "Just Drupal" patch for review's-sake.
Comment #14
RobLoachShould be $container->register()->setProperty() instead to save lines of code and the use statement above.
For a constructor, we should merge in an $options array. This would let us use setArguments rather than setProperties.
2 days to next Drupal core point release.
Comment #15
pdrake CreditAttribution: pdrake commentedThis version of the patch uses language_default to extend the language class with default values and adds a test to ensure the dependency injected language provider has all of the properties of the $GLOBALS language object. Drupal only patch included for easier review.
Comment #17
pdrake CreditAttribution: pdrake commentedTrying again. Hopefully this one passes tests. Previous patch apparently only passes tests on a multilingual site.
Comment #18
pdrake CreditAttribution: pdrake commented#17 apparently still had the same problem of only passing on a multilingual site. This one should pass on both single language and multilingual sites.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedI posted just the Symfony component on #1465584-22: Review external library dependencies in core. I have some minor comments on the Drupal code in #18, but I'll wait until we can pass around 5k instead of 300k patches with each other. In general, +1 to the idea of using dependency injection, Symfony's implementation, and starting with the global language objects use-case.
Comment #20
RobLoachComment #22
RobLoachhttp://drupal.org/files/drupal-1497230-20.patch is the patch without the component.
Comment #23
pdrake CreditAttribution: pdrake commentedIf we are ultimately replacing the procedural ($_GLOBAL) language handling code with the Language object, is there a reason that procedural code should be setting the default state of the Language object, rather than the default state of the Language object being inherent? I realize the way I did that with initialize() in #18 wasn't pretty (since it depended on external procedural code), but the pattern follows Symfony\Component\HttpFoundation\Session, Symfony\Component\HttpFoundation\Request, etc objects in that a function on the object can be called to build the default state of the object, rather than procedural code explicitly setting the default state of the object.
Comment #24
RobLoachUnderstood :-) . The language initialization is pretty gross to work with, and it's difficult to keep the coupled code coupled and the decoupled code decoupled. Pushing those decisions to the Container rather the Language object itself is probably the way to go.
That's a tough call to make. I built the Language class with the English defaults because that's what language_default() does (the only difference is that new Languages come with $language->default == FALSE). What we have to do is tell the Container to register the Language objects with the correct properties, and this is where your handy
$language->extend()
method comes in :-) .Do you think there's anything else missing in this patch? It could use a review or two, but I think it's looking pretty good! What are your thoughts?
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedI like this patch, but it doesn't demonstrate any (non test) code using it. Is there any client code out there that we can change from using $_GLOBALS['language_interface'] to drupal_container()->get('language_interface')? Even better, can we change all client code to do that and remove the global?
Comment #26
pdrake CreditAttribution: pdrake commentedAttached is a patch that replaces use of the language globals in core/include/*. It seems to work for tests, but drush doesn't like it (because of the way drush "boots" drupal).
Comment #28
plachWell, actually the initialization of the language globals for multilingual sites involves making use of the full language negotiation subsystem, so it's no surprise that a single method needs to call external code here.
Actually I don't see a true benefit in having the language negotiation code confined in the Language object for two reasons:
Obviously both of the two bullets imply porting the current initialization system to PSR-0 classes, which I'll be happy to work on ;)
Some remark on the last patch:
All these comments look pretty redundant. Also usually we don't keep track in comments about previous solutions, not sure about the real benefit here.
@Crell confirmed we want to switch to the dot notation. Do we want to postpone this until all the globals are gone or is this just an oversight?
I think a @todo about removing this line when getting rid of globals might be useful here, since we have a different behavior wrt multilingual sites, where the language object is used to initialize the Drupal\Core\Language\Language object.
Here and below, see the first comment.
Actually we got rid of the "provider" terminology in favor of the "negotiation method" one. However what is actually being injected here is the pure Language object.
We could use the constant here too.
Missing space before the parenthesis.
Typo and missing trailing dot.
-2 days to next Drupal core point release.
Comment #29
pdrake CreditAttribution: pdrake commentedI presume the language negotiation code will ultimately be performed elsewhere in the process (possibly during routing as part of a Negotiation library - I believe wamilton is working on that) and the Language object will simply inherit its default settings from somewhere. I agree that negotiation does not need to be part of the object.
Thanks for the patch review. I added some fixes for failing tests as well as resolving a number of the issues you pointed out.
The purpose of the somewhat-redundant comments was so that we can see the direction this is headed, comparing what I have done with what existed before and can be removed whenever it is appropriate (probably during the removal of all language globals). The comparison tests that ensure the DI Language object is the same as the GLOBAL language object may be removed at that time as well.
The switch to dot notation (eg language_interface to language.interface) should be trivial once all language globals are eliminated as it can be accomplished by updating the constants.
Comment #30
plachComment #31
plachInteresting :)
Ok, makes sense.
Changing the values in constants implies working on the upgrade path, since those values determine the name of the variables use to store the fallback configuration. Probably we should perform the switch while moving the negotiation system to PSR-0, since the Drupal-specific part will need an update function anyway.
Minor thing, sorry, but I just realized also the example code above might use constants, unless we assume that we won't always have constants for service ids. In which case the example would be right in general, but misleading for language-specific usages.
-2 days to next Drupal core point release.
Comment #33
cosmicdreams CreditAttribution: cosmicdreams commentedA Dependency Injection container has a far reaching and super awesome impact on many parts of Drupal. The biggest improvement I think DI can help with is the Configuration Initiative, so I'm tagging this issue for that.
With a DI, we can have an active store for configuration and a single place to pull that active configuration during the process of generating exportable configuration files.
A DI also makes it easier to extend Drupal, because it provides a place to put pointers to functions. So if we had a tag for Plugins in Core I'd tag this issue for that too.
In short, this is HUGE.
Comment #34
cosmicdreams CreditAttribution: cosmicdreams commentedAlso, we need a DI if we're going to replace all the globals.
Comment #35
pdrake CreditAttribution: pdrake commentedTrying again. I believe this resolves the remaining failing tests - guess we'll see...
Comment #36
RobLoachComment #37
RobLoachIf we reset the Container, we will loose all our objects that we want to keep persistent in the Container, and have to call all of our initialize functions whenever
drupal_static_reset()
is called. Because of this, I think using drupal_static() for drupal_container() is the wrong way to go. We should probably usedrupal_container($reset)
instead, until at least we have a proper place to keep the Container instance.Once the Kernel patch goes in, the Container instance could become a property of the Kernel object. This means we'd have proper scope of all our Drupal objects depending on which Kernel instance we're using. For now though, it seems like
drupal_container($reset)
is the way to go.-29 days to next Drupal core point release.
Comment #38
RobLoach1497230-37-justdrupal.patch is the patch with only Drupal stuff. Here it is with the Dependency Injection component for the test bot.
Comment #39
pdrake CreditAttribution: pdrake commentedSo, I definitely see how avoiding drupal_static for the dependency injection container will avoid numerous difficulties. I do think that ultimately there will need to be multiple containers, or the container will need to be context-aware. In this case (Language), the object should exist within the context of a request (or subrequest) but not across requests (or subrequests). So, it should live and die with the Request object. Other objects (Database? Session?) may rightfully persist across subrequests if they are handled within a single PHP process. Since the context of the container object is not the same as the context of drupal_static variables, removing the drupal_static is logical.
Comment #40
EclipseGc CreditAttribution: EclipseGc commentedAnd this is why there's a whole stacking system involved so that new containers can be used and populated across subrequests, giving you the ability to always get back to the request that started the whole thing. I think we're making this too complicated, I think symfony has a methodology for doing this stuff already. Crell is probably the right person to ask at this point though.
Eclipse
Comment #41
Crell CreditAttribution: Crell commentedThe Symfony container's "scope" system is for exactly that. I don't know at the moment how it works internally (fabpot, lsmith, or Stof could perhaps enlighten us?), but it is designed to handle exactly what #39 talks about.
Comment #42
RobLoachAdded another follow up to have objects register themselves: #1511482: Bootstrap for the Dependency Injection Container.
Comment #42.0
RobLoachUpdated
Comment #43
Crell CreditAttribution: Crell commentedWith the comments in the latest patch indicating the transitional state of the ugly bits, I am comfortable with #37/#38. Since we want that DIC sooner rather than later, I'll be aggressive with it. :-)
Comment #44
andremolnar CreditAttribution: andremolnar commentedThis isn't a blocker for getting DI in, and will ultimately be a follow up issue around the language system in general, but I have reservations with the default Language class having default name and langcode values.
The other defaults are arguably sensible, but make me uncomfortable to a lesser extent.
Comment #45
catchI don't understand why these comments are everywhere. Once it's in core, it's de-facto not 'new', and we never have comments referring to what used to happen, that's what change notices are for.
stdClass?
Instead of or as well as a @todo, can we open that follow-up issue as a major task so it's tracked? Same with the note in the test talking about removing the duplicate test.
I don't see any discussion here as to why we'd use Symfony's DIC instead of Pimple, did that discussion happen during a sprint or irc somewhere? Since Pimple was being considered before, it'd be good to know why Symfony's was chosen. If there's no particular reason, then it'd be good to document that too.
Generally +1 for trying to move towards dependency injection, although I'd be more comfortable with committing this if there were some follow-up issues linked from the issue summary. Also +1 for not worrying about scope/stacking etc. with the initial patch for now, I'd rather see efforts go towards properly injecting dependencies everywhere in the first place, then it's less of an issue anyway.
Also this is a small patch in terms of the Drupal changes, but it's going to touch just about everything in Drupal core by the time the process is done, even if it's not finished by Drupal 10 in the end, so I'm assigning to Dries to take a look.
Comment #46
RobLoachTwo of them are up there, the one to remove the remaining $language_interface is: #1510686: Replace remaining references to global $language_interface with drupal_container().
We talked about Pimple during the WSCCI sprint with Fabien during Drupalcon Denver, and before even considering Symfony's Dependency Injection component, I was under the impression that we'd use Pimple instead. I really do like Pimple's design, but there were some use cases brought up during the sprint where Symfony's Dependency Injection component was what we wanted. Instead of having useful functions like below, it comes down to callable anonymous functions, which might become cumber-sum to use. Because of this, Fabien was advocating we use Symfony instead of Pimple.
Again, that's a loose example, but you can see why Symfony's Dependency Injection is the preferred method.
There's the one I listed above to clean up both the comments and remaining references to $language_interface, and then there's #1511482: Bootstrap for the Dependency Injection Container. I'll make a few more follow ups to clean $language_content and $language_url.
So we should remove those comments?
Comment #47
EclipseGc CreditAttribution: EclipseGc commentedSymfony's dependency injection container is a bit more capable than pimple, plus I don't think we were ever seriously considering pimple, we just hadn't had time to dig into Symfony's yet and it was easy to get Pimple running to play with and wave our hands and say "it'll be sorta like this". I think overall it makes better sense to utilize Symfony's though given the number of other Symfony components we're embracing, and there's no technical reason not to. I'm pretty sure that's the entirety of the logic on that decision :-D
Comment #47.0
EclipseGc CreditAttribution: EclipseGc commentedUpdated issue summary.
Comment #48
RobLoachCleaned up the code comments, like catch mentioned. Also added the following to the issue summary: #1512310: Replace $language_url with Dependency Injection and #1512308: Replace $language_content with Dependency Injection.
Comment #49.0
RobLoachUpdated issue summary.
Comment #50
RobLoachSetting this back to RTBC as catch's notes have been addressed. This patch also updates the @todo comment with a link directly to #1512424: Add a LanguageInterface, and property setters/getters to Language class.
Comment #51
Crell CreditAttribution: Crell commentedPimple's use of closures actually makes it a bit easier to work with than Symfony's, and it's a vastly more light-weight implementation. However, the Symfony container has two rather important benefits over Pimple:
1) Its configuration can be serialized (closures cannot be serialized), which makes it possible to dump configuration to disk/database/whatever and reload easily without a massive rebuild process on each request.
2) It has support for scoping to force regeneration of various items that depend on other items. Pimple does not have any such concept. (This serves the same purpose as the "stacking" logic from the Context API patch.)
While we're not using either of those capabilities in this patch, we know that we will want to sooner or later.
Comment #52
neclimdulActually there's a 3rd benefit that is the underpinnings of both of Crell's points and that's lazy instantiation. Because you can register class names and constructor arguments(and other startup stuff) Symfony's DI Container waits until its requested to instantiated the injected element which means if somehow you never use the language system, it would never be loaded. This is not true of pimple because you instantiate and register full objects.
Comment #53
Crell CreditAttribution: Crell commentedneclimdul: Eh, that's completely untrue. The whole point of the closures in Pimple is to NOT instantiate anything until you need to. :-) Pimple is just as capable of lazy-instantiation as the Symfony component is.
Either way, let's stop talking about it and commit this thing. :-)
Comment #54
effulgentsia CreditAttribution: effulgentsia commentedHere and elsewhere, I don't think we need this code comment. The line of code should be clear on its own. If it's not, we should rename the function, but for now, I think we should just remove the comment, and defer function name bikesheds to a follow-up.
The comment and line of code disagree on "." vs. "_", and neither matches the actual code in the patch (which uses a constant).
This doesn't explain why we want this. How about "We do not use drupal_static() here because we do not have a mechanism by which to reinitialize the stored objects, so a drupal_static_reset() call would leave Drupal in a nonfunctional state."
Here and the same code in update.php:
1. We usually don't use abbreviations like $lang in Drupal, and in this case, I don't think it describes what this variable actually is. If we leave Language::_construct() alone, this variable should be $options.
2) If variable_get('language_default') returns something other than the Language class's default public property values, then this results in a mismatch between $container->get($type) and language_default(). So, perhaps this should be replaced with "->addMethodCall('extend', array($default));"?
Based on my comment for the previous code block, we should test more here: one test after a variable_del('language_default') where $expected contains all the default property values in the Language class, and one after a variable_set('language_default') to something other than English and making $expected equal to that.
Comment #54.0
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #54.1
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedI updated the "Proposed resolution" part of the summary with the info from #51. Unassigning Dries until #54 is addressed.
Comment #56
pdrake CreditAttribution: pdrake commentedI've done my best in this version of the patch to address the additional issues raised by effulgentsia. This version significantly changes the tests, but does not introduce a single test in which the variable language_default is modified and the Language object is re-tested. The reason for this is that the Language object is implicitly scoped to the Container. Adding such a test and expecting the Language object to respect the new defaults would require implementing Scope(s) within this patch. If that is in-scope for this issue, it can certainly be done.
Comment #57
pdrake CreditAttribution: pdrake commentedFor easier review, this is the above patch with just the Drupal changes (Symfony classes excluded).
Comment #58
pdrake CreditAttribution: pdrake commentedComment #60
pdrake CreditAttribution: pdrake commentedI sure messed that patch up. Trying again.
Comment #62
pdrake CreditAttribution: pdrake commentedSorry for spamming everyone on this issue. Trying again.
Comment #64
pdrake CreditAttribution: pdrake commented#62: 1497230-62.patch queued for re-testing.
Comment #65
RobLoachOverall, I believe this is good to go. With the different default language test, cleaned up code comments, and a defined path of what follows, we're now fully accommodating catch's and effulgensia's notes. Back in the RTBC queue! Took some notes when reviewing:
Looks a bit weird with the constant, but that'll be fixed with #1510686: Replace remaining references to global $language_interface with drupal_container().
This comment does make more sense than what we had before. Thanks for the note, Alex.
Nice job on putting the different default language test together.
-1 days to next Drupal core point release.
Comment #66
effulgentsia CreditAttribution: effulgentsia commentedSome more cleanup.
Comment #68
effulgentsia CreditAttribution: effulgentsia commentedComment #70
effulgentsia CreditAttribution: effulgentsia commentedD'oh. I uploaded the #68 interdiff with the wrong extension which testbot didn't like. Here's #68 again to keep bot happy.
Comment #71
RobLoachOne note, but other than that, I think it's good. What are your thoughts?
Not sure how I feel about using the short-hands when they cross the 80 character width mark, but I think I can cope.
-1 days to next Drupal core point release.
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedI'm happy to reroll if people want shorter lines, but my opinion is that as much as possible, 1 line of code should express 1 unit of thought. Setting a variable in 1 line that is only used by the immediately following line and nowhere else in the function is taking 2 lines to express 1 unit of thought. I also assume that most people develop on relatively large screen sizes, so 80 chars is not a magic limit. It is for documentation, but not for code, IMO, unless we have coding standards that say otherwise.
In most places where I shortened what was in #62, it was for performance (don't get the object until it's needed) rather than reducing line count, so if desired, we can re-add lines as long as we do so in the innermost possible code blocks.
Comment #73
yched CreditAttribution: yched commentedErrs on the side of nitpicking, but one thing I actually like about globals is the convention of scoping them at the beginning of a function. Makes dependencies clearer, close to the function signature.
I suggest maybe we adopt the same convention with stuff from the DIC, assign them to vars at the beginning of a function for the rest of the code to use ?
Comment #74
effulgentsia CreditAttribution: effulgentsia commentedDrupal HEAD doesn't follow this convention very consistently. Grepping the codebase for "global $" results in 325 matches in 99 files. Grepping for "$GLOBALS" results in 137 matches in 45 files. So if our intent is to be tracking dependencies at the beginning of functions, our current success rate is only 70%.
That's not so good for performance in functions like t(), l(), and many others where micro-optimizations matter and where the service object might only be needed within some code blocks. If we want a good way of tracking service dependencies, how about a PHP docblock directive?
Comment #75
Crell CreditAttribution: Crell commentedThe use of the drupal_container() wrapper itself I understand to be a temporary measure. Temporary for the entire Drupal 8 lifecycle, maybe, but still temporary. :-) Any objects that are spawned by the container that have dependencies should have those provided by the container to their constructors, by design. So I wouldn't worry about optimizing the function approach too much. We should handle that case by case. Having drupal_container()->get('whatever') randomly in the middle of a function is no worse than having whatever() in the middle of function, which is what we do now just about everywhere.
Comment #76
RobLoachShort-hand or not, drupal_container() or not, we should probably just get this in and clean up any follow up stuff in one of the many remaining tasks. RTBC again.
Comment #77
effulgentsia CreditAttribution: effulgentsia commentedThanks for bearing with me guys. Ok, back to Dries. I'm now feeling pretty confident he'll accept this.
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedComment #79
cosmicdreams CreditAttribution: cosmicdreams commentedCreated #1515196: Standardize retrieval of "global" variables to be at beginning of function so that this issue can move forward. Will add to the description of #1497230: Use Dependency Injection to handle object definitions
Comment #79.0
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated issue summary.
Comment #80
RobLoach#70: 1497230-68.patch queued for re-testing.
Comment #80.0
RobLoachadd follow up issue so we can keep track of the reorganization of code as per the comments of http://drupal.org/node/1497230#comment-5827086
Comment #80.1
RobLoacha
Comment #81
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#70: 1497230-68.patch queued for re-testing.
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedJust a reroll of #68 for moving of files common.test and locale.inc. No substantive changes here. "needs review" for bot, but if it passes, should go back to RTBC.
Comment #84
EclipseGc CreditAttribution: EclipseGc commentedper 83
Comment #85
tstoecklerTwo minor cosmetic issues. Because this issue blocks a lot of progress in other areas, I am leaving this at RTBC. I will post a follow-up patch once this is committed.
This code could be simplified if we initialize
$language = language_default();
up-front and then override$language
if language_multilingual() is TRUE. Then we would not have to duplicate the$container->register()
block.Is there a reason this is split into multiple lines?
Comment #86
RobLoachI agree, but we were trying to keep the language system exactly the same as it is now, and just stick the Container in as a direct swap in replacement for $GLOBALS. We could clean up the language system initialization itself later on in #1510686: Replace remaining references to global $language_interface with drupal_container().
It's hard to find the line where the short-hand should be used vs using a simple $language definition. Do you know if we have any documentation on that? In general, I lean towards the long-hand, but I'm okay with using the short-hand. I could re-roll this one tonight if we want to short-hand this one.
Comment #87
RobLoachHere's a re-roll with the short-hand that tstoeckler mentioned.
Comment #88
RobLoachPushing the priority to major as we have some stuff in the plugin system that could benefit from having the container.
Comment #89
effulgentsia CreditAttribution: effulgentsia commentedFor the past month, I've been a major proponent of using the dependency injection container for some of the plugin system work, but in a conversation yesterday with neclimdul, EclipseGc, and merlinofchaos, we found a way to make progress without it. We may yet need it before we're done, but at least for now, it's not a blocker. Regardless, I agree that this is major priority.
Comment #90
EclipseGc CreditAttribution: EclipseGc commentedThere are various other improvements waiting in the wings that could use this, and could also use as much time as possible to play with it. The sooner this goes in, the better.
Comment #91
Dries CreditAttribution: Dries commentedI reviewed this patch twice now, had a chance to think about it more, and decided it looks good. I just committed it to 8.x.
We should proceed with it for now; if we want to switch to a simpler dependency injection system we can later. It is abstracted out enough; it shouldn't be hard if that is what we decide to do. For now, it is nice to have the extra functionality available.
Thanks all!
Comment #92
RobLoachWhoop! Doesn't look like this was pushed :-)
http://drupal.org/node/3060/commits
Comment #93
Gábor HojtsySeems like that just happened. http://drupal.org/commitlog/commit/2/0ca408e1d32fea988d1b64e40da4aef1b36...
Comment #94
Gábor HojtsyStill needs a change notice :) Reopened for that. Rob do you want to take it away? :)
Comment #95
RobLoachHmmm, probably needs some work and some more examples. What are your thoughts? http://drupal.org/node/1539454
Comment #96
Gábor HojtsyLooks good IMHO. Added a little more notes there.
Comment #97
cosmicdreams CreditAttribution: cosmicdreams commented@Rob Loach suggested that we create an issue for each $GLOBAL that needs to be updated to use dependency injection. So here ya go:
Follow ups:
#1539598: Modify update.php to use Dependency Injection for language
#1539602: Modify common.inc to use Dependency Injection for language
#1539608: Modify comment module to use Dependency Injection for language
#1539614: Modify language module to use Dependency Injection for language
#1539622: Modify locale module to use dependency injection for language
#1539626: Modify node module to use dependency injection for language
Comment #98
webchickThis went in without any documentation of those properties. Tsk, tsk. :)
Comment #99
Gábor HojtsyBased on chat with @webchick, she meant the Language class properties.
Comment #100
webchickTo clarify:
...is a no-no. We need PHPDoc above each one of those that explains what it does. http://api.drupal.org/api/drupal/globals/8 hopefully has some copy/pasteable language.
Comment #101
RobLoachwebchick: That's planned for #1512424: Add a LanguageInterface, and property setters/getters to Language class.
Comment #102
webchickHm. I'm not really comfortable with side-stepping gates on core patches. There's no reason documentation couldn't be added here and then adjusted as necessary over at the other issue.
Comment #103
RobLoachShould we merge that issue into this one then? In the follow up issue, those properties will become
protected
and be replaced with getName(), getDirection(), etc with actual proper documentation. The major point of this issue wasdrupal_container()
, not Language, to avoid bikesheds.Comment #104
RobLoachActually, let's turn this in a META parent.
Comment #105
ebeyrent CreditAttribution: ebeyrent commentedI agree with webchick in #100 - at the very least, there should be documentation for each var. To do this properly though, there should be getters and setters for each of those data members.
Comment #106
Anonymous (not verified) CreditAttribution: Anonymous commentedthis broke scripts that look like that. not sure how this fits, like whether we care at all, but yeah. seems ungood to me. digging around now trying to figure out why.
Comment #107
Anonymous (not verified) CreditAttribution: Anonymous commentedok, so this seems to have broken error handling early in bootstrap. not setting $_SERVER['REMOTE_ADDR'] causes a notice in this script:
and here's the output:
Comment #108
Anonymous (not verified) CreditAttribution: Anonymous commentedtalked to catch in IRC, he suggested this be a major because it's a regression.
Comment #109
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's a fairly hacky test-only patch that shows the regression.
Comment #111
Crell CreditAttribution: Crell commentedDo we actually support such scripts? Really? And if so, are we better off just punting on it since there are already active patches in progress that will totally change the way you'd write such scripts to begin with?
Comment #112
cosmicdreams CreditAttribution: cosmicdreams commentedTwo questions here
1) Even though
drupal_container()->get(LANGUAGE_TYPE_INTERFACE)
is the only service that seems to be registered (in update.php) other services such as:drupal_container()->get(LANGUAGE_TYPE_CONTENT)
anddrupal_container()->get(LANGUAGE_TYPE_URL)
are called. Should those be added to this patch?2) Where in the code should they be registered?
Comment #113
Anonymous (not verified) CreditAttribution: Anonymous commentedre #111: this is not about the snippet script anymore, i just started looking when stuff that used to work blew up in my face.
re #112: two excellent questions.
so the general issue is that if you call drupal_container() looking for something that's not yet registered, we blow up. so any system, like, say, translating strings, that relies on stuff in the container, can blow up when called early during bootstrap.
did t() used to work early in the bootstrap?
as it is now t() doesn't work until LANGUAGE_TYPE_INTERFACE is registered with our DIC, which doesn't happen until late in the bootstrap cycle. so we can either move the registration earlier, or say that you have to be careful calling t() early in the bootstrap, or ...
new patch just adds a call to t() in hook_boot() in the translation_test module, which blows up.
Comment #115
moshe weitzman CreditAttribution: moshe weitzman commentedWe worked very hard during D7 to make t() safe to call anytime in Drupal (installer excepted). This is a regression.
Comment #116
Anonymous (not verified) CreditAttribution: Anonymous commentedthanks moshe.
so, one option is to make t() aware that the DIC may blow up in it's face and add some code like this:
seems really ugly, but works.
it seems unsafe to move drupal_language_initialize() before _drupal_bootstrap_database(), because the list of enabled languages is fetched from db.
Comment #117
lotyrin CreditAttribution: lotyrin commentedThis same thing blows up drush:
Comment #118
lotyrin CreditAttribution: lotyrin commentedThe try-catch from #116 doesn't seem to resolve the issue for drush.
Comment #119
cosmicdreams CreditAttribution: cosmicdreams commentedIt makes more sense to me to have the DI service registrations be performed earlier in the bootstrap. As early as possible. But I don't know where that would be.
Comment #120
plachWith respect specifically to language the plans are to migrate the list of enabled languages to CMI. The negotiation process might need to acess the DB, depending on the configured negotiation methods, but assuming that usages of the language service will happen only after the DB is initialized might make sense. Hence we could rely on lazy-initialization to cope with this.
Hence on the language negotiation side we could try to move everything up in the bootstrap process, right after CMI/variables are available. Obviously this is a mid-term solution, not a quick fix.
Comment #121
Anonymous (not verified) CreditAttribution: Anonymous commentedCMI in core requires the db to work. active store === the database.
this is in flux, but just want to make clear that as of now, 'move to CMI' does not break the dependency on a loaded db.
Comment #122
plachMmmh, you are right, forget #120 then.
Comment #123
RobLoachThe easy fix is to ->register() the default languages early. We could take it further later on and have the Container load its initial bootstrap configuration from a file:
We'll need some early default bootstrap stuff to take advanced of the container during the bootstrap, having it load from a file might make it easier to handle later on.
Comment #124
effulgentsia CreditAttribution: effulgentsia commentedI suspect #123 will be correct in the long run (not necessarily XML based), along with compiling. But there's a lot we need to figure out to make that happen. In the meantime, how's this?
Comment #125
effulgentsia CreditAttribution: effulgentsia commentedComment #126
effulgentsia CreditAttribution: effulgentsia commentedRe #116, you can also call drupal_container()->has(), or pass ContainerInterface::NULL_ON_INVALID_REFERENCE as a 2nd param to get(). But I think setting up the container with necessary services (e.g., #124) is better than making functions have to worry about lack of service availability.
Comment #127
RobLoachI'm not sure about calling language_default() directly as we don't want it to call variable_get() so early. The default new Language() parameters might actually be okay to live with, until the Language system is initialized. If multilingualization is on, then calling ->register() again will just swap out the definition. So, maybe just:
-18 days to next Drupal core point release.
Comment #128
RobLoachPut in some inline comments in the test about why we're doing it.
Comment #129
lotyrin CreditAttribution: lotyrin commentedThis looks good and solves the boot t() case, as well as the minimal script and drush. Even includes a test.
Comment #130
lotyrin CreditAttribution: lotyrin commentedI was mistaken, drush is not resolved by #128.
Comment #131
webchick:(
Comment #132
clemens.tolboomDrush is expecting the t() function to be valid too early in the bootsstrap.
In drush #1540110: Drush calling t() before D8 dependency injection construct is available I patched drush to make sure the classloader has at least some namespaces registered.
drush test for the existence only of the t-function but t() now depends on the classloader being properly initialized.
(my 2 cents)
Comment #133
cosmicdreams CreditAttribution: cosmicdreams commentedIn that case #128 sounds like a proper follow up patch to this issue. Since the tests pass this issues sounds like it is RTBC
Comment #134
clemens.tolboomShould drush use get_t() instead? Or one could argue the t-function should not be available yet: that is move it to another file?
Or we need to adjust the bootstrap having the class loader configured properly?
Comment #135
tim.plunkettYes, so back to RTBC from #129. It doesn't 100% solve drush's problem, but it is needed.
Comment #136
effulgentsia CreditAttribution: effulgentsia commentedWhat do you all think of this? This ensures that as long as drupal_classloader() is called, it has core/vendor/Symfony and core/lib/Drupal registered. This still doesn't fully solve the Drush issue, because Drush is calling t() prior to bootstrapping to DRUPAL_BOOTSTRAP_CONFIGURATION, but this would allow Drush to simply call drupal_classloader() early in its process and then have t() work. I considered making drupal_container() call drupal_classloader() so that Drush wouldn't even have to do that, but that would be wrong, because drupal_classloader() relies on variable_get(), so Drupal should not call it prior to DRUPAL_BOOTSTRAP_CONFIGURATION: that really does need to be Drush's choice to do that.
Comment #137
sunI like that patch. Simplifies things.
Lastly, let's please create a new issue for whatever-meta discussion/container is required. This issue is/was about a concrete functional change and the comments in here are anything but meta. ;) Thanks. Removing the prefix from the issue title.
Comment #138
RobLoachGreat idea! Let's hit that up in #1511482: Bootstrap for the Dependency Injection Container.
Comment #139
sunAll DI follow-up issues mentioned in this issue are now tagged accordingly:
http://drupal.org/project/issues/search/drupal?issue_tags=Dependency%20I...
If there's a need for a META issue, please create one.
Comment #140
sund'oh, sorry for the noise.
Comment #141
catchOK it looks like the drush issue is in progress, and the follow-up here looks fine. Committed/pushed to 8.x and moving this back to fixed.
Comment #142
sunThe original commit in #87 caused a fatal regression:
drupal_language_initialize() cannot be invoked more than once anymore.
This causes a fatal error on test(bot) clients, which is not caught due to very unfortunate testing system circumstances.
Running PathMonolingualTestCase locally exposes the error - the test "passes", but Simpletest (UI) ends with a PDOException.
Attached patch fixes the trigger, but not the cause. I'm fine with committing this stop-gap fix to unblock other patches, but we need to resolve the actual cause.
Comment #143
RobLoachConfirmed... And with #1550866: Remove obsolete drupal_language_initialize(),
drupal_language_initialize()
will likely just go away.Comment #144
jthorson CreditAttribution: jthorson commentedNote to committers: Please let me know when this is in.
I've got a testbot patch which should detect something like and prevent it from sneaking through in the future, but don't want to apply it until the PDO Exception is gone from HEAD tests ... as adding it now would cause HEAD to fail and freeze all other D8 patch testing.
Comment #145
webchickcatch is AFK all weekend, and presumably this qualifies as the kind of 8.x patch I can commit, since it's fixing totally broken stuff.
So, committed and pushed to 8.x. Thanks! :)
Comment #146
sunThanks! Created the required follow-up: #1552744: Bootstrap for the Dependency Injection Container and make sure SimpleTest abides to it
Comment #147
cosmicdreams CreditAttribution: cosmicdreams commentedDon't see this commit with git log. Is this someone I just need to wait and see later or is the fact I'm not seeing it mean that it wasn't really committed to Drupal 8 yet?
Comment #148
tim.plunkettIt's here, no worries: http://drupalcode.org/project/drupal.git/commit/56ded31
Comment #149
cosmicdreams CreditAttribution: cosmicdreams commentedAh thanks Tim I see it now.
Comment #151
sunComment #151.0
sunfdsfd