merged branch evillemez/master (PR #3557)

fabpot · fabpot · commit 460c18195165 · 2012-04-19T11:56:35.000+02:00
Commits ------- b7b26af [DependencyInjection] Added, implemented and tested IntrospectableContainerInterface::initialized() Discussion ---------- [DependencyInjection] IntrospectableContainerInterface::initialized() Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Added, implemented and tested `IntrospectableContainerInterface::initialized()`, which allows checking for whether or not a service id has actually been loaded, without forcing it to load. This could be implemented in several places to prevent loading a service when it's only needed if it has been previously loaded - for example the SwiftmailBundle's event listener for the `kernel.terminate` event, which currently forces Swiftmail to load on every request to check for messages to send, even if it was not previously loaded. --------------------------------------------------------------------------- by fabpot at 2012-03-11T09:10:32Z Do you have any other examples in mind where it would be useful? --------------------------------------------------------------------------- by stof at 2012-03-11T12:39:22Z @fabpot 2 exemples: - the Swiftmailer listener to avoid loading the initialization code of Swiftmailer in each kernel.terminate event just to check that there is no pending message in the spool (if the spool has never been retrieved, it cannot have messages) (this is the use case we discussed on IRC last night) - closing Doctrine connections and Monolog handlers in the shutdown method without creating them if they were not used --------------------------------------------------------------------------- by stof at 2012-03-11T12:39:53Z However, I don't like the name of the method --------------------------------------------------------------------------- by lsmith77 at 2012-03-11T13:26:34Z sounds very useful --------------------------------------------------------------------------- by evillemez at 2012-03-11T17:00:39Z @fabpot another example: * Forcing a session to write early, if it was previously loaded - but not having to load the session to check, thus potentially forcing a database connection (if that's how the session is being handled) when it's not needed. @stof Would more or less verbose be better? Like, `isServiceLoaded()` or just `loaded()`.... or a better word than "loaded" ? :) --------------------------------------------------------------------------- by stof at 2012-03-11T17:03:11Z @evillemez My issue is the word ``loaded``. I don't think it is clear about what it means here --------------------------------------------------------------------------- by lsmith77 at 2012-03-11T17:04:24Z one thing we should also keep in mind here is the scope of the service. BTW: there are also a couple of CS violations in your PR --------------------------------------------------------------------------- by fabpot at 2012-03-11T17:07:43Z @stof: I agree that we should think of a better name: `initialized()` or `exists()` (to differentiate from `has()`)? @evillemez: After choosing the name, can you work on actually using the new method for some of the use cases mentioned in this PR? --------------------------------------------------------------------------- by lsmith77 at 2012-03-11T17:20:12Z what i meant with "scope" was if we are only talking about services instantiated in the current scope? but i guess there is no way to handle anything else anyway. as for name .. i think ``instantiated()`` is more fitting than ``initialized()``. ``exists()`` could be confused with ``has()`` imho --------------------------------------------------------------------------- by stof at 2012-03-11T17:26:06Z The current implementation only works for container-scoped services. It does not make sense for prototyped-scope services anyway (as the container does not keep them) but supporting scoped services will be tricky IMO --------------------------------------------------------------------------- by mvrhov at 2012-03-11T17:34:21Z hasBeen(Used|Called|Initialized), --------------------------------------------------------------------------- by evillemez at 2012-03-11T17:54:43Z The next day or two I'm only around for an hour or so here and there, I may be a little slow to respond. @stof @lsmith77 I agree with either `instantiated()` or `initialized()`, I also think `exists()` might be easily confused with `has()` @lsmith77 Besides the opening/closing brace placements, was there anything else? @fabpot I would happily implement it in the Swiftmail bundle - as of now that's the only area where I know absolutely it would be useful. The Session example I mentioned was an issue I ran into working on another app in another framework, and I'm not familiar yet with the internals of the Monolog/Doctrine Bundles. But if people could point me in the right direction I'd be willing to check it out. I'm still relatively new to some of the Symfony2 internals, so if there are obvious things I'm missing, please don't hesitate to point them out. :) --------------------------------------------------------------------------- by evillemez at 2012-03-11T18:00:29Z @lsmith77 Oh... I think there were some tab issues I didn't see in my editor, I'll fix those too. --------------------------------------------------------------------------- by stof at 2012-03-11T18:13:03Z The places where it should be used for Doctrine and Monolog are in separate repos anyway so it cannot be part of this PR --------------------------------------------------------------------------- by evillemez at 2012-03-12T03:38:50Z Any thoughts on `instantiated` vs `initialized`? I'm leaning towards `initialized`. How should I proceed, close this request and submit another with the changed name and fixed CS violations? --------------------------------------------------------------------------- by fabpot at 2012-03-12T07:41:11Z `initialized()` looks fine to me. Make your changes, squash your commits and then force the push to your branch (the PR will be updated automatically). --------------------------------------------------------------------------- by evillemez at 2012-03-12T20:49:17Z I was about to squash my commits to update this, but it just occurred to me that I hadn't considered the interface. Does anyone feel this method `initialized()` should be defined in ContainerInterface as well? It seems like it's generic enough that it would make sense. But I'm not immediately aware if this would cause BC breaks. --------------------------------------------------------------------------- by fabpot at 2012-03-13T11:34:33Z We cannot break BC for `ContainerInterface` as this is marked with the `@api` tag. --------------------------------------------------------------------------- by henrikbjorn at 2012-03-13T12:34:42Z Is it a BC break if we add a method? i thought only changing the already written methods would be a BC break. --------------------------------------------------------------------------- by ooflorent at 2012-03-13T12:39:44Z @henrikbjorn It will raise a fatal error if the method isn't implemented in existing class. --------------------------------------------------------------------------- by lsmith77 at 2012-03-13T13:06:26Z we could however add a new interface that extends from the previous one. --------------------------------------------------------------------------- by evillemez at 2012-03-13T15:40:39Z Are the BC breaks we are worried about for compatibility just within the Symfony repo - or in general in case others have implemented the interface elsewhere? As far as Symfony is concerned, the only class I can find that implements `ContainerInterface` is `Container`, so adding the method shouldn't be an issue. If it's an issue of principle, in case others may have implemented the `ContainerInterface`, then... yeah, it's a break, and maybe should be left out. I suppose this would mean that other places in code that type hint for `ContainerInterface` would have to change the type hint to `Container` if they want to implement the `initialized()` method. Is this more or less acceptable than updating the interface? --------------------------------------------------------------------------- by evillemez at 2012-03-14T19:17:27Z Hadn't properly squashed commits, just fixed. --------------------------------------------------------------------------- by evillemez at 2012-03-15T14:06:38Z I'm done with this PR, unless there is a consensus that I should also implement `Container::initialized()` in `ContainerInterface`. Anything else I need to do? --------------------------------------------------------------------------- by Seldaek at 2012-03-15T15:41:44Z @evillemez the common pattern for BC is to add a new interface, say IntrospectableContainerInterface or something, that extends ContainerInterface, then you can type-hint that without restricting use of competing implementations of the Container class. --------------------------------------------------------------------------- by stof at 2012-04-03T22:30:51Z @evillemez Please update this PR. It conflicts with master because of the move of tests. And the new interface suggested by @Seldaek is a good idea IMO --------------------------------------------------------------------------- by evillemez at 2012-04-04T14:57:29Z @stof I may not be able to get to this until the end of the week, but I'll do it as soon as I can. I'll rebase against the current master, and add/implement the interface. Is `IntrospectableContainerInterface ` as suggested by @Seldaek ok with everyone? Are there other features that we can think of that would be useful for this new interface? I don't want to start a precedent of adding a new interface for every new method that comes up... I understand it makes sense for not breaking backwards compatibility for something previously marked as stable, but still, it's yet another file that will likely be included on every request. --------------------------------------------------------------------------- by stof at 2012-04-04T15:35:15Z @evillemez classes used on every requests can be added to some cached bootstrap files (which are loaded during the ``$kernel->loadClassCache()`` call in your front controller) to avoid including many files through the autoloader. And for even better performances in prod, the solution is to use APC with ``apc.stat = 0``, as advocated by @lsmith77 --------------------------------------------------------------------------- by evillemez at 2012-04-15T19:00:07Z Ok, rebased against current master and implemented the interface. I didn't mark it as `@api` because I think we may want to consider if there's any other functionality that could be implemented there before declaring it stable. Sorry for the delay. My wife and I are in the process of buying a house, and some unexpected things have come up. --------------------------------------------------------------------------- by fabpot at 2012-04-18T10:59:01Z Can you rebase before I merge? Thanks.
diff --git a/src/Symfony/Component/DependencyInjection/Container.php b/src/Symfony/Component/DependencyInjection/Container.php
@@ -59,7 +59,7 @@
  *
  * @api
  */
-class Container implements ContainerInterface
+class Container implements IntrospectableContainerInterface
 {
     protected $parameterBag;
     protected $services;
@@ -265,6 +265,18 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
             throw new ServiceNotFoundException($id);
         }
     }
+    
+    /**
+     * Returns true if the given service has actually been initialized
+     *
+     * @param string $id          The service identifier
+     *
+     * @return Boolean true if service has already been initialized, false otherwise
+     */
+    public function initialized($id)
+    {
+        return isset($this->services[strtolower($id)]);
+    }
 
     /**
      * Gets all service ids.
diff --git a/src/Symfony/Component/DependencyInjection/IntrospectableContainerInterface.php b/src/Symfony/Component/DependencyInjection/IntrospectableContainerInterface.php
@@ -0,0 +1,33 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection;
+
+/**
+ * IntrospectableContainerInterface defines additional introspection functionality
+ * for containers, allowing logic to be implemented based on a Container's state.
+ *
+ * @author Evan Villemez <evillemez@gmail.com>
+ *
+ */
+interface IntrospectableContainerInterface extends ContainerInterface
+{
+    /**
+     * Check for whether or not a service has been initialized.
+     *
+     * @param string $id
+     * 
+     * @return Boolean true if the service has been initialized, false otherwise
+     *
+     */
+    function initialized($id);
+    
+}
diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
@@ -193,6 +193,18 @@ public function testHas()
         $this->assertTrue($sc->has('foo.baz'), '->has() returns true if a get*Method() is defined');
     }
 
+    /**
+     * @covers Symfony\Component\DependencyInjection\Container::initialized
+     */
+    public function testInitialized()
+    {
+        $sc = new ProjectServiceContainer();
+        $sc->set('foo', new \stdClass());
+        $this->assertTrue($sc->initialized('foo'), '->initialized() returns true if service is loaded');
+        $this->assertFalse($sc->initialized('foo1'), '->initialized() returns false if service is not loaded');
+        $this->assertFalse($sc->initialized('bar'), '->initialized() returns false if a service is defined, but not currently loaded');
+    }
+
     public function testEnterLeaveCurrentScope()
     {
         $container = new ProjectServiceContainer();
