[Asset] Allows to download asset manifest over HTTP #35762

GromNaN · 2020-02-17T23:38:02Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #35761 Fix #33001
License	MIT
Doc PR	symfony/symfony-docs#13255

framework:
    assets:
        json_manifest_path: 'https://cdn.example.com/manifest.json'

nicolas-grekas

Please mind the CI.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

nicolas-grekas

LGTM, here are some nitpicking.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/assets.xml

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

src/Symfony/Bundle/FrameworkBundle/composer.json

src/Symfony/Component/Asset/Tests/VersionStrategy/RemoteJsonManifestVersionStrategyTest.php

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php

src/Symfony/Component/Asset/composer.json

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php

src/Symfony/Component/Asset/VersionStrategy/JsonManifestVersionStrategy.php

…omponent tests (GromNaN) This PR was merged into the 3.4 branch. Discussion ---------- Use strict assertSame instead of assertEquals in Asset component tests | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | #35762 (comment) | License | MIT | Doc PR | N/A Using `assertSame` instead of `assertEquals` is recommended when possible (see sebastianbergmann/phpunit-documentation-english#3). It is stricter and must be more efficient ([`===`](https://github.com/sebastianbergmann/phpunit/blob/8.5.2/src/Framework/Constraint/IsIdentical.php#L63) vs a [comparator class](https://github.com/sebastianbergmann/phpunit/blob/8.5.2/src/Framework/Constraint/IsEqual.php#L79)). ~~Also, removing useless string cast.~~ Commits ------- e8f3e84 Use strict assertion in asset tests

nicolas-grekas · 2020-02-21T18:12:55Z

Let's improve the HttpClient component then!

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php

…es (GromNaN) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [HttpClient][DX] Add URL context to JsonException messages | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | #35762 (comment) | License | MIT | Doc PR | N/A In order to help when debugging incorrect JSON responses, this PR adds the requested URL to the error message. Before: `Syntax Error` After: `JSON error: Syntax error, from "https://example.com/file.json".` See the 2nd commit for full diff in new unit tests Commits ------- 0653917 [HttpClient][DX] Add URL context to JsonException messages

GromNaN · 2020-02-26T10:12:03Z

Absolutely, it needs to be cached. And the cache needs to be served in case of error. IMO this is the responsability of the HttpClient to handle this behavior.
It's a good job that the CachingHttpClient already handles stale-while-revalidate and stale-if-error.

I'm trying to create a demo app to show the feature, but I don't find the correct config for http_client.

nicolas-grekas

IMO this is the responsibility of the HttpClient to handle this behavior.

I'm not sure about that: there are many benefits in using CacheInterface that won't be achieved by a CachingHttpClient (sharing of the cache, lock while fetching, probabilistic expiration, etc).

But this could be handled in a separate PR that adds a generic CachedManifestVersionStrategy, so that both ways are possible in the end.

Which means I'm good here, whith one last comment.

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php

GromNaN

I simplified the code to the simplest form. The CachedManifestVersionStrategy is a clever idea that would solve performance and high availability issues. I can work on it once this PR gets merged.

Commits has been squashed & rebased.

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php

Handle URL in json_manifest_path Download the manifest using the HttpClient

nicolas-grekas · 2020-03-31T19:39:15Z

Anyone else @symfony/mergers?

weaverryan · 2020-04-04T19:19:40Z

I like this. Practical question: how could/should someone add caching? Decorate the service? Something else? There just needs to be a reasonable way and it's ok if it's simply covered in the docs.

nicolas-grekas · 2020-04-05T09:58:47Z

@weaverryan check #35762 (review)

nicolas-grekas · 2020-04-05T10:00:25Z

Thank you @GromNaN.

nicolas-grekas · 2020-04-05T10:00:50Z

@GromNaN would you like to give CachedManifestVersionStrategy a try?

GromNaN · 2020-04-06T07:57:23Z

I'll open a PR today.

This PR was squashed before being merged into the master branch. Discussion ---------- [Asset] Document remote JSON manifest Documentation for symfony/symfony#35762 - New feature for Asset component - Integration in FrameworkBundle configuration Commits ------- be4760c [Asset] Document remote JSON manifest

carsonbot added Status: Needs Review Asset Feature labels Feb 17, 2020

GromNaN force-pushed the remote-manifest branch 2 times, most recently from dfdc390 to 17cf357 Compare February 17, 2020 23:43

nicolas-grekas added this to the next milestone Feb 18, 2020

nicolas-grekas reviewed Feb 18, 2020

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

GromNaN force-pushed the remote-manifest branch 4 times, most recently from 8e3c79b to b027a5c Compare February 18, 2020 15:35

This comment has been minimized.

Sign in to view

GromNaN requested a review from nicolas-grekas February 18, 2020 21:07

nicolas-grekas reviewed Feb 18, 2020

View reviewed changes

GromNaN force-pushed the remote-manifest branch from a82bbd7 to aa997d7 Compare February 18, 2020 22:52

GromNaN requested a review from nicolas-grekas February 18, 2020 23:19

nicolas-grekas reviewed Feb 19, 2020

View reviewed changes

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Feb 19, 2020

View reviewed changes

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php Outdated Show resolved Hide resolved

src/Symfony/Component/Asset/VersionStrategy/JsonManifestVersionStrategy.php Outdated Show resolved Hide resolved

GromNaN mentioned this pull request Feb 19, 2020

Use strict assertSame instead of assertEquals in Asset component tests #35793

Merged

GromNaN force-pushed the remote-manifest branch 2 times, most recently from bfd0141 to 10dd62a Compare February 19, 2020 22:44

GromNaN force-pushed the remote-manifest branch 2 times, most recently from c929d43 to d517ee3 Compare February 21, 2020 17:02

GromNaN requested a review from nicolas-grekas February 21, 2020 17:13

GromNaN mentioned this pull request Feb 21, 2020

[HttpClient][DX] Add URL context to JsonException messages #35822

Merged

GromNaN force-pushed the remote-manifest branch from d517ee3 to 1366b3b Compare February 22, 2020 22:25

nicolas-grekas reviewed Feb 23, 2020

View reviewed changes

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php Outdated Show resolved Hide resolved

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Feb 26, 2020

View reviewed changes

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php Outdated Show resolved Hide resolved

GromNaN force-pushed the remote-manifest branch 2 times, most recently from b6b5d59 to 7e45e6e Compare February 26, 2020 12:01

GromNaN commented Feb 26, 2020

View reviewed changes

src/Symfony/Component/Asset/VersionStrategy/RemoteJsonManifestVersionStrategy.php Outdated Show resolved Hide resolved

GromNaN force-pushed the remote-manifest branch from 7e45e6e to cc7f039 Compare February 26, 2020 12:10

[Asset] Allows to download json manifest from a remote url

4ba12a8

Handle URL in json_manifest_path Download the manifest using the HttpClient

GromNaN force-pushed the remote-manifest branch from cc7f039 to 4ba12a8 Compare February 26, 2020 14:39

GromNaN mentioned this pull request Feb 26, 2020

[Asset] Document remote JSON manifest symfony/symfony-docs#13255

Merged

GromNaN requested a review from nicolas-grekas February 27, 2020 10:54

nicolas-grekas approved these changes Feb 27, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 27, 2020

nicolas-grekas added the Ready label Mar 31, 2020

javiereguiluz approved these changes Apr 3, 2020

View reviewed changes

nicolas-grekas merged commit 0647508 into symfony:master Apr 5, 2020

GromNaN deleted the remote-manifest branch April 6, 2020 07:55

GromNaN mentioned this pull request Apr 6, 2020

[Asset] Add CachedVersionStrategy to decorate any Asset version strategy #36371

Closed

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

fabpot mentioned this pull request May 5, 2020

Release v5.1.0-BETA1 #36713

Merged

GromNaN mentioned this pull request May 18, 2020

Encore manifest.json, assets package base_path #36844

Closed

GromNaN referenced this pull request Oct 12, 2020

Create ManifestPackages to replace ManifestVersionStrategy

c3720c1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Asset] Allows to download asset manifest over HTTP #35762

[Asset] Allows to download asset manifest over HTTP #35762

GromNaN commented Feb 17, 2020 •

edited

Loading

nicolas-grekas left a comment

This comment has been minimized.

nicolas-grekas left a comment

nicolas-grekas commented Feb 21, 2020 via email

GromNaN commented Feb 26, 2020 •

edited

Loading

nicolas-grekas left a comment

GromNaN left a comment •

edited

Loading

nicolas-grekas commented Mar 31, 2020

weaverryan commented Apr 4, 2020

nicolas-grekas commented Apr 5, 2020

nicolas-grekas commented Apr 5, 2020

nicolas-grekas commented Apr 5, 2020

GromNaN commented Apr 6, 2020

[Asset] Allows to download asset manifest over HTTP #35762

[Asset] Allows to download asset manifest over HTTP #35762

Conversation

GromNaN commented Feb 17, 2020 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

This comment has been minimized.

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Feb 21, 2020 via email

GromNaN commented Feb 26, 2020 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

GromNaN left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Mar 31, 2020

weaverryan commented Apr 4, 2020

nicolas-grekas commented Apr 5, 2020

nicolas-grekas commented Apr 5, 2020

nicolas-grekas commented Apr 5, 2020

GromNaN commented Apr 6, 2020

GromNaN commented Feb 17, 2020 •

edited

Loading

GromNaN commented Feb 26, 2020 •

edited

Loading

GromNaN left a comment •

edited

Loading