feature #39484 [FrameworkBundle] Allow env variables in json_manifest_path (jderusse)

fabpot · fabpot · commit c766e59924ab · 2020-12-15T13:21:53.000+01:00
This PR was merged into the 5.3-dev branch. Discussion ---------- [FrameworkBundle] Allow env variables in `json_manifest_path` | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | Fix #39264 | License | MIT | Doc PR | - the parameter `framework.assets.json_manifest_path` does not allow env variable when value is remote. This PR adds a new `DynamicJsonManifestVersionStrategy` to fix that. Commits ------- 4e4a81c Allow env variables in `json_manifest_path`
diff --git a/UPGRADE-5.3.md b/UPGRADE-5.3.md
@@ -1,6 +1,11 @@
 UPGRADE FROM 5.2 to 5.3
 =======================
 
+Asset
+-----
+
+ * Deprecated `RemoteJsonManifestVersionStrategy`, use `JsonManifestVersionStrategy` instead.
+
 Form
 ----
 
diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md
@@ -1,6 +1,11 @@
 UPGRADE FROM 5.x to 6.0
 =======================
 
+Asset
+-----
+
+ * Removed `RemoteJsonManifestVersionStrategy`, use `JsonManifestVersionStrategy` instead.
+
 Config
 ------
 
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -1129,12 +1129,7 @@ private function createVersion(ContainerBuilder $container, ?string $version, ?s
         }
 
         if (null !== $jsonManifestPath) {
-            $definitionName = 'assets.json_manifest_version_strategy';
-            if (0 === strpos(parse_url($jsonManifestPath, \PHP_URL_SCHEME), 'http')) {
-                $definitionName = 'assets.remote_json_manifest_version_strategy';
-            }
-
-            $def = new ChildDefinition($definitionName);
+            $def = new ChildDefinition('assets.json_manifest_version_strategy');
             $def->replaceArgument(0, $jsonManifestPath);
             $container->setDefinition('assets._version_'.$name, $def);
 
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/assets.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/assets.php
@@ -77,10 +77,12 @@
             ->abstract()
             ->args([
                 abstract_arg('manifest path'),
+                service('http_client'),
             ])
 
         ->set('assets.remote_json_manifest_version_strategy', RemoteJsonManifestVersionStrategy::class)
             ->abstract()
+            ->deprecate('symfony/framework-bundle', '5.3', 'The "%service_id%" service is deprecated, use "assets.json_manifest_version_strategy" instead.')
             ->args([
                 abstract_arg('manifest url'),
                 service('http_client'),
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/assets.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/assets.php
@@ -30,6 +30,15 @@
             'remote_manifest' => [
                 'json_manifest_path' => 'https://cdn.example.com/manifest.json',
             ],
+            'var_manifest' => [
+                'json_manifest_path' => '%var_json_manifest_path%',
+            ],
+            'env_manifest' => [
+                'json_manifest_path' => '%env(env_manifest)%',
+            ],
         ],
     ],
 ]);
+
+$container->setParameter('var_json_manifest_path', 'https://cdn.example.com/manifest.json');
+$container->setParameter('env(env_manifest)', 'https://cdn.example.com/manifest.json');
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/assets.xml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/assets.xml
@@ -23,6 +23,13 @@
             </framework:package>
             <framework:package name="json_manifest_strategy" json-manifest-path="/path/to/manifest.json" />
             <framework:package name="remote_manifest" json-manifest-path="https://cdn.example.com/manifest.json" />
+            <framework:package name="var_manifest" json-manifest-path="%var_json_manifest_path%" />
+            <framework:package name="env_manifest" json-manifest-path="%env(env_manifest)%" />
         </framework:assets>
     </framework:config>
+
+    <parameters>
+        <parameter key="var_json_manifest_path">https://cdn.example.com/manifest.json</parameter>
+        <parameter key="env(env_manifest)">https://cdn.example.com/manifest.json</parameter>
+    </parameters>
 </container>
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/assets.yml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/assets.yml
@@ -21,3 +21,11 @@ framework:
                 json_manifest_path: '/path/to/manifest.json'
             remote_manifest:
                 json_manifest_path: 'https://cdn.example.com/manifest.json'
+            var_manifest:
+                json_manifest_path: '%var_json_manifest_path%'
+            env_manifest:
+                json_manifest_path: '%env(env_manifest)%'
+
+parameters:
+    var_json_manifest_path: 'https://cdn.example.com/manifest.json'
+    env(env_manifest): https://cdn.example.com/manifest.json
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
@@ -597,7 +597,7 @@ public function testAssets()
 
         // packages
         $packages = $packages->getArgument(1);
-        $this->assertCount(7, $packages);
+        $this->assertCount(9, $packages);
 
         $package = $container->getDefinition((string) $packages['images_path']);
         $this->assertPathPackage($container, $package, '/foo', 'SomeVersionScheme', '%%s?version=%%s');
@@ -621,8 +621,18 @@ public function testAssets()
 
         $package = $container->getDefinition($packages['remote_manifest']);
         $versionStrategy = $container->getDefinition($package->getArgument(1));
-        $this->assertSame('assets.remote_json_manifest_version_strategy', $versionStrategy->getParent());
+        $this->assertSame('assets.json_manifest_version_strategy', $versionStrategy->getParent());
         $this->assertSame('https://cdn.example.com/manifest.json', $versionStrategy->getArgument(0));
+
+        $package = $container->getDefinition($packages['var_manifest']);
+        $versionStrategy = $container->getDefinition($package->getArgument(1));
+        $this->assertSame('assets.json_manifest_version_strategy', $versionStrategy->getParent());
+        $this->assertSame('https://cdn.example.com/manifest.json', $versionStrategy->getArgument(0));
+
+        $package = $container->getDefinition($packages['env_manifest']);
+        $versionStrategy = $container->getDefinition($package->getArgument(1));
+        $this->assertSame('assets.json_manifest_version_strategy', $versionStrategy->getParent());
+        $this->assertStringMatchesFormat('env_%s', $versionStrategy->getArgument(0));
     }
 
     public function testAssetsDefaultVersionStrategyAsService()
diff --git a/src/Symfony/Bundle/FrameworkBundle/composer.json b/src/Symfony/Bundle/FrameworkBundle/composer.json
@@ -34,7 +34,7 @@
     "require-dev": {
         "doctrine/annotations": "~1.7",
         "doctrine/cache": "~1.0",
-        "symfony/asset": "^5.1",
+        "symfony/asset": "^5.3",
         "symfony/browser-kit": "^4.4|^5.0",
         "symfony/console": "^5.2",
         "symfony/css-selector": "^4.4|^5.0",
@@ -71,7 +71,7 @@
         "phpdocumentor/reflection-docblock": "<3.0",
         "phpdocumentor/type-resolver": "<0.2.1",
         "phpunit/phpunit": "<5.4.3",
-        "symfony/asset": "<5.1",
+        "symfony/asset": "<5.3",
         "symfony/browser-kit": "<4.4",
         "symfony/console": "<5.2",
         "symfony/dotenv": "<5.1",
diff --git a/src/Symfony/Component/Asset/CHANGELOG.md b/src/Symfony/Component/Asset/CHANGELOG.md
@@ -1,6 +1,11 @@
 CHANGELOG
 =========
 
+5.3.0
+-----
+
+ * deprecated `RemoteJsonManifestVersionStrategy`, use `JsonManifestVersionStrategy` instead.
+
 5.1.0
 -----
 
diff --git a/src/Symfony/Component/Asset/Tests/VersionStrategy/JsonManifestVersionStrategyTest.php b/src/Symfony/Component/Asset/Tests/VersionStrategy/JsonManifestVersionStrategyTest.php
@@ -13,47 +13,83 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Asset\VersionStrategy\JsonManifestVersionStrategy;
+use Symfony\Component\HttpClient\MockHttpClient;
+use Symfony\Component\HttpClient\Response\MockResponse;
 
 class JsonManifestVersionStrategyTest extends TestCase
 {
-    public function testGetVersion()
+    /**
+     * @dataProvider ProvideValidStrategies
+     */
+    public function testGetVersion(JsonManifestVersionStrategy $strategy)
     {
-        $strategy = $this->createStrategy('manifest-valid.json');
-
         $this->assertSame('main.123abc.js', $strategy->getVersion('main.js'));
     }
 
-    public function testApplyVersion()
+    /**
+     * @dataProvider ProvideValidStrategies
+     */
+    public function testApplyVersion(JsonManifestVersionStrategy $strategy)
     {
-        $strategy = $this->createStrategy('manifest-valid.json');
-
         $this->assertSame('css/styles.555def.css', $strategy->applyVersion('css/styles.css'));
     }
 
-    public function testApplyVersionWhenKeyDoesNotExistInManifest()
+    /**
+     * @dataProvider ProvideValidStrategies
+     */
+    public function testApplyVersionWhenKeyDoesNotExistInManifest(JsonManifestVersionStrategy $strategy)
     {
-        $strategy = $this->createStrategy('manifest-valid.json');
-
         $this->assertSame('css/other.css', $strategy->applyVersion('css/other.css'));
     }
 
-    public function testMissingManifestFileThrowsException()
+    /**
+     * @dataProvider ProvideMissingStrategies
+     */
+    public function testMissingManifestFileThrowsException(JsonManifestVersionStrategy $strategy)
     {
         $this->expectException('RuntimeException');
-        $strategy = $this->createStrategy('non-existent-file.json');
         $strategy->getVersion('main.js');
     }
 
-    public function testManifestFileWithBadJSONThrowsException()
+    /**
+     * @dataProvider ProvideInvalidStrategies
+     */
+    public function testManifestFileWithBadJSONThrowsException(JsonManifestVersionStrategy $strategy)
     {
         $this->expectException('RuntimeException');
         $this->expectExceptionMessage('Error parsing JSON');
-        $strategy = $this->createStrategy('manifest-invalid.json');
         $strategy->getVersion('main.js');
     }
 
-    private function createStrategy($manifestFilename)
+    public function provideValidStrategies()
+    {
+        yield from $this->provideStrategies('manifest-valid.json');
+    }
+
+    public function provideInvalidStrategies()
+    {
+        yield from $this->provideStrategies('manifest-invalid.json');
+    }
+
+    public function provideMissingStrategies()
+    {
+        yield from $this->provideStrategies('non-existent-file.json');
+    }
+
+    public function provideStrategies(string $manifestPath)
     {
-        return new JsonManifestVersionStrategy(__DIR__.'/../fixtures/'.$manifestFilename);
+        $httpClient = new MockHttpClient(function ($method, $url, $options) {
+            $filename = __DIR__.'/../fixtures/'.basename($url);
+
+            if (file_exists($filename)) {
+                return new MockResponse(file_get_contents($filename), ['http_headers' => ['content-type' => 'application/json']]);
+            }
+
+            return new MockResponse('{}', ['http_code' => 404]);
+        });
+
+        yield [new JsonManifestVersionStrategy('https://cdn.example.com/'.$manifestPath, $httpClient)];
+
+        yield [new JsonManifestVersionStrategy(__DIR__.'/../fixtures/'.$manifestPath)];
     }
 }
diff --git a/src/Symfony/Component/Asset/Tests/VersionStrategy/RemoteJsonManifestVersionStrategyTest.php b/src/Symfony/Component/Asset/Tests/VersionStrategy/RemoteJsonManifestVersionStrategyTest.php
@@ -17,6 +17,9 @@
 use Symfony\Component\HttpClient\MockHttpClient;
 use Symfony\Component\HttpClient\Response\MockResponse;
 
+/**
+ * @group legacy
+ */
 class RemoteJsonManifestVersionStrategyTest extends TestCase
 {
     public function testGetVersion()
diff --git a/src/Symfony/Component/Asset/VersionStrategy/JsonManifestVersionStrategy.php b/src/Symfony/Component/Asset/VersionStrategy/JsonManifestVersionStrategy.php
@@ -11,6 +11,9 @@
 
 namespace Symfony\Component\Asset\VersionStrategy;
 
+use Symfony\Contracts\HttpClient\Exception\DecodingExceptionInterface;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
+
 /**
  * Reads the versioned path of an asset from a JSON manifest file.
  *
@@ -26,13 +29,15 @@ class JsonManifestVersionStrategy implements VersionStrategyInterface
 {
     private $manifestPath;
     private $manifestData;
+    private $httpClient;
 
     /**
      * @param string $manifestPath Absolute path to the manifest file
      */
-    public function __construct(string $manifestPath)
+    public function __construct(string $manifestPath, HttpClientInterface $httpClient = null)
     {
         $this->manifestPath = $manifestPath;
+        $this->httpClient = $httpClient;
     }
 
     /**
@@ -53,13 +58,23 @@ public function applyVersion(string $path)
     private function getManifestPath(string $path): ?string
     {
         if (null === $this->manifestData) {
-            if (!is_file($this->manifestPath)) {
-                throw new \RuntimeException(sprintf('Asset manifest file "%s" does not exist.', $this->manifestPath));
-            }
+            if (null !== $this->httpClient && 0 === strpos(parse_url($this->manifestPath, \PHP_URL_SCHEME), 'http')) {
+                try {
+                    $this->manifestData = $this->httpClient->request('GET', $this->manifestPath, [
+                        'headers' => ['accept' => 'application/json'],
+                    ])->toArray();
+                } catch (DecodingExceptionInterface $e) {
+                    throw new \RuntimeException(sprintf('Error parsing JSON from asset manifest URL "%s".', $this->manifestPath), 0, $e);
+                }
+            } else {
+                if (!is_file($this->manifestPath)) {
+                    throw new \RuntimeException(sprintf('Asset manifest file "%s" does not exist.', $this->manifestPath));
+                }
 
-            $this->manifestData = json_decode(file_get_contents($this->manifestPath), true);
-            if (0 < json_last_error()) {
-                throw new \RuntimeException(sprintf('Error parsing JSON from asset manifest file "%s": ', $this->manifestPath).json_last_error_msg());
+                $this->manifestData = json_decode(file_get_contents($this->manifestPath), true);
+                if (0 < json_last_error()) {
+                    throw new \RuntimeException(sprintf('Error parsing JSON from asset manifest file "%s": ', $this->manifestPath).json_last_error_msg());
+                }
             }
         }
 
diff --git a/src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php b/src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php
@@ -13,6 +13,8 @@
 
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 
+trigger_deprecation('symfony/asset', '5.3', 'The "%s" class is deprecated, use "%s" instead.', RemoteJsonManifestVersionStrategy::class, JsonManifestVersionStrategy::class);
+
 /**
  * Reads the versioned path of an asset from a remote JSON manifest file.
  *
@@ -23,6 +25,8 @@
  *     }
  *
  * You could then ask for the version of "main.js" or "css/styles.css".
+ *
+ * @deprecated since Symfony 5.3, use JsonManifestVersionStrategy instead.
  */
 class RemoteJsonManifestVersionStrategy implements VersionStrategyInterface
 {
diff --git a/src/Symfony/Component/Asset/composer.json b/src/Symfony/Component/Asset/composer.json
@@ -16,7 +16,8 @@
         }
     ],
     "require": {
-        "php": ">=7.2.5"
+        "php": ">=7.2.5",
+        "symfony/deprecation-contracts": "^2.1"
     },
     "suggest": {
         "symfony/http-foundation": ""
