[AssetMapper] Fix in-file imports to resolve via filesystem #52349
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weaverryan
commented
Oct 28, 2023
| @@ -38,23 +37,29 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac | |||
| { | |||
| return preg_replace_callback(self::ASSET_URL_PATTERN, function ($matches) use ($asset, $assetMapper) { | |||
| try { | |||
| $resolvedPath = $this->resolvePath(\dirname($asset->logicalPath), $matches[1]); | |||
| $resolvedSourcePath = Path::join(\dirname($asset->sourcePath), $matches[1]); | |||
There was a problem hiding this comment.
A bit unrelated: let's let Path do the work
weaverryan
commented
Oct 28, 2023
| // avoid warning about vendor imports - these are often comments | ||
| if ($asset->isVendor) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This just represents a reorg so we can return earlier
weaverryan
commented
Oct 28, 2023
| @@ -38,23 +37,29 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac | |||
| { | |||
| return preg_replace_callback(self::ASSET_URL_PATTERN, function ($matches) use ($asset, $assetMapper) { | |||
| try { | |||
There was a problem hiding this comment.
It looks like we can ignore this. In practice, our preg_replace_callback() will return a string. But if there is some change we want to make to satisfy psalm, we can.
fabpot
approved these changes
Oct 28, 2023
|
Thank you @weaverryan. |
f7f3574 to
659c635
Compare
This was referenced Oct 29, 2023
xabbuh
added a commit
that referenced
this pull request
Oct 29, 2023
This PR was merged into the 6.4 branch. Discussion ---------- [AssetMapper] Fixing merge | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | None | License | MIT Fixing the merge of #52323, #52331 and #52349 Also tested on a real project locally to verify the moving pieces :). Thanks! Commits ------- 99d5cbb [AssetMapper] Fixing merge from multiple PR's
This was referenced Oct 29, 2023
Merged
Merged
nicolas-grekas
added a commit
that referenced
this pull request
Oct 31, 2023
…e importmap entry path (weaverryan) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [AssetMapper] Fixing bug where JSCompiler used non-absolute importmap entry path | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | None | License | MIT Introduced over the weekend in #52349. The `path` key inside `importmap.php` can be a full filesystem path or it can start with `./` to indicate a relative path to the root directory. That has been the case for weeks. In #52349, when start using the `path` of an importmap entry to find the `MappedAsset`. When doing that, I didn't consider the case where `path` is a relative path. This PR fixes that and moves the code handling this into `ImportMapConfigReader` so that we're not repeating it in several places. Cheers! Commits ------- 31d04ef [AssetMapper] Fixing bug where JSCompiler used non-absolute importmap entry path
weaverryan
added a commit
to symfony/ux
that referenced
this pull request
Nov 2, 2023
This PR was squashed before being merged into the 2.x branch. Discussion ---------- Adding support for AssetMapper 6.4 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Issues | None | License | MIT Change for AssetMapper 6.4 support. This is 2 main things: A) Changes needed after symfony/symfony#52349 with removal of trait + absolute import paths B) Outputting `autoimport` via `{{ importmap() }}` function / deprecate `ux_controller_link_tags()`. TODO: * I still need to the React/Svelte/Vue support to double-check it's all good for both 6.3 and 6.4. * Finish lazy autoimport * Make sure `controllers.json` is properly a config cache file resource for appropriate files Cheers! Commits ------- 05bcb98 Adding support for AssetMapper 6.4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi!
Another fix from further testing in the complex world!
tl;drRelative imports in CSS & JS files are now resolved via the filesystem path, not the AssetMapper "logical path".Full Explanation
In AssetMapper, we configure different paths/directories, which exposes them to the public directory. If you need to fetch the info about an asset, you can use its "logical path".
However, when you're inside of a JavaScript or CSS file, these "asset mapper" paths or "logical asset paths" are correctly irrelevant. Instead, you should code as if all of your files are already public and won't be moved around:
This PR fixes case 2️⃣ which previously did not work. Before, we used
../vendor/foo/bar/assets/baz.jsand the "asset mapper logical path" of the importing file (app.js) and tried to construct a final, logical path from that. But because of the../vendor, that went above theapp.jsroot, and we panicked. Even if we didn't panic, logical paths should not be relevant here. Instead, it's simpler: the relative import inapp.jsuses the filesystem path. Of course, if the final file -/vendor/foo/bar/assets/baz.jsis not, itself, in some AssetMapper path, an clear exception is thrown (if the file is not exposed publicly, we can't resolve a proper path to it).Cheers!