Skip to content
This repository was archived by the owner on Jun 23, 2024. It is now read-only.

chore: Migrate away from ContainerAwareInterface + Allow Symfony 7 - #80

Open
aszenz wants to merge 4 commits into
theofidry:masterfrom
aszenz:chore/fix-deprecation-on-symfony-6.4
Open

aszenz wants to merge 4 commits into
theofidry:masterfrom
aszenz:chore/fix-deprecation-on-symfony-6.4

Conversation

@aszenz

@aszenz aszenz commented Dec 9, 2023

Copy link
Copy Markdown

Partially fixes #79

#SymfonyHackday

Comment thread src/PsyshFacade.php Outdated
{
self::init();
if (!isset(self::$shell)) {
throw new RuntimeException('Cannot initialize the facade without a container.');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message should mention the shell, not the container

@aszenz aszenz changed the title chore: Migrate away from ContainerAwareInterface chore: Migrate away from ContainerAwareInterface + Allow Symfony 7 Jan 26, 2024
@aszenz
aszenz force-pushed the chore/fix-deprecation-on-symfony-6.4 branch from 18a4e73 to edbdf0a Compare January 26, 2024 15:50
@aszenz aszenz mentioned this pull request Feb 7, 2024
@aszenz

aszenz commented Feb 11, 2024

Copy link
Copy Markdown
Author

@theofidry Could you check this pr

@lstrojny

lstrojny commented Feb 28, 2024

Copy link
Copy Markdown

I’ve been running this patch in the real world for some time and I noticed a performance regression of up to 40ms on every request in environments where the bundle is enabled.

Because PsychBundle::boot() calls $this->container->get('psysh.facade'); the psych.shell service will be initialized on every request. The initialization of the facade used to be lazy but is now eager.

But: replacing it with a service closure works and makes it lazy again and removes the performance penalty.

diff --git a/resources/config/services.xml b/resources/config/services.xml
index 12617b5..4191ba1 100644
--- a/resources/config/services.xml
+++ b/resources/config/services.xml
@@ -21,8 +21,8 @@
         </service>
 
         <service id="psysh.facade" class="Fidry\PsyshBundle\PsyshFacade" public="true">
-            <call method="setShell">
-                <argument type="service" id="psysh.shell" />
+            <call method="setShellInitializer">
+                <argument type="service_closure" id="psysh.shell" />
             </call>
         </service>
 
diff --git a/src/PsyshFacade.php b/src/PsyshFacade.php
index c39f9a3..1a9c933 100644
--- a/src/PsyshFacade.php
+++ b/src/PsyshFacade.php
@@ -11,6 +11,7 @@
 
 namespace Fidry\PsyshBundle;
 
+use Closure;
 use Psy\Shell;
 use RuntimeException;
 use Symfony\Component\DependencyInjection\ContainerAwareInterface;
@@ -23,10 +24,15 @@ use function array_merge;
 final class PsyshFacade
 {
     private static Shell $shell;
+    private static Closure $shellInitializer;
 
     public static function init(): void
     {
-        // noop ... keeping the method as is for backward compatibility
+        if (isset(self::$shell)) {
+            return;
+        }
+
+        self::$shell = (self::$shellInitializer)();
     }
 
     public static function debug(array $variables = [], $bind = null): void
@@ -40,8 +46,8 @@ final class PsyshFacade
         self::$shell::debug($_variables, $bind);
     }
 
-    public function setShell(Shell $shell): void
+    public function setShellInitializer(Closure $shellInitializer): void
     {
-        self::$shell = $shell;
+        self::$shellInitializer = $shellInitializer;
     }
 }

@theofidry

Copy link
Copy Markdown
Owner

@lstrojny that would be a different scope however. But please check #81

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Symfony 7 support

3 participants