Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[AssetMapper] Bug fix for paths that start with the same string#50355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[AssetMapper] Bug fix for paths that start with the same string#50355
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -76,7 +76,7 @@ public function findLogicalPath(string $filesystemPath): ?string | |||
| } | |||
| foreach ($this->getDirectories() as $path => $namespace) { | |||
| if (!str_starts_with($filesystemPath, $path)) { | |||
| if (!str_starts_with($filesystemPath, $path.'/')) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Be careful. Import map paths might already have a trailing/:https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script/type/importmap#mapping_path_prefixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we're ok. These are all comparing filesystem paths - not the actual paths that are exposed to the user / importmap.
9f35611 to9158f05CompareI've just simplified this - it's now just a simple bug fix. The "absolute path" thing might be cool. But in practice, during a UX bundle install, we'll use |
Thank you@weaverryan. |
…yan)This PR was squashed before being merged into the 6.3 branch.Discussion----------Bug fix for paths that start with the same string| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | None| License | MIT| Doc PR | Still TODOHi!This should be minor:1) Fixed a bug where if 1 asset path directory is a substring of another (e.g. `assets/dir` and `assets/dir2`), when you called `AssetMapperRepository::findLogicalPath()` passing a filesystem path pointed at `assets/dir2`, it would incorrectly match something in `assets/dir`.~~2) Allowed `importmap.php` paths to be absolute. The use-case is for UX libraries. UX libraries WILL automatically add a "namespaced path" to the asset mapper. For example, the StimulusBundle I'm working on will likely expose a `symfony/stimulus-bundle` asset mapper namespace that points to the `assets/` directory of that bundle. But, for users, I don't want this to be "another thing you need to know". So, the result of this PR is that, in the user's `importmap.php`:~~```diff '`@symfony`/stimulus-bundle' => [- // "symfony/stimulus-bundle" is an asset path that points into that bundle- 'path' => 'symfony/stimulus-bundle/loader.js',+ 'path' => __DIR__.'/../../assets/loader.js', ],```~~The user points to the absolute file, WE figure out which "asset path" that's in, and everything works nicely.~~UPDATE: I've removed this.Cheers!Commits-------51b9246 Bug fix for paths that start with the same string
97a03c2 to51b9246Compare
Uh oh!
There was an error while loading.Please reload this page.
Hi!
This should be minor:
assets/dirandassets/dir2), when you calledAssetMapperRepository::findLogicalPath()passing a filesystem path pointed atassets/dir2, it would incorrectly match something inassets/dir.2) Allowedimportmap.phppaths to be absolute. The use-case is for UX libraries. UX libraries WILL automatically add a "namespaced path" to the asset mapper. For example, the StimulusBundle I'm working on will likely expose asymfony/stimulus-bundleasset mapper namespace that points to theassets/directory of that bundle. But, for users, I don't want this to be "another thing you need to know". So, the result of this PR is that, in the user'simportmap.php:The user points to the absolute file, WE figure out which "asset path" that's in, and everything works nicely.UPDATE: I've removed this.
Cheers!