-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[AssetMapper] Allowing for files to be written to some non-local location #51847
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
looks pretty good. I am right now in Berlin for SymfonyLive. I will check next week and build something with flysystem :). But arent the most things breaking changes? 🤔 |
Indeed, but the component has been released as experimental to allow for such changes if needed. |
That's also why I'd like to get this in NOW for 6.4 - the component will be stable in 6.4, so making a change like this will be harder. We're already dipping a little beyond the 6.4 feature-freeze date :) |
ac2f5cf
to
45e93ee
Compare
src/Symfony/Component/AssetMapper/Command/AssetMapperCompileCommand.php
Outdated
Show resolved
Hide resolved
85d6468
to
fef0903
Compare
public function getOutputDir(): string | ||
{ | ||
return $this->outputDir; | ||
} |
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 created this event - and just included $outputDir
because I had it available. Now that it's not readily available, I don't think it's a big deal to not include it. My use-case for this event is to run some code that pre-compiles some internal files (not into the output directory) so that when the true compilation process happens, those files are ready and available.
fef0903
to
67249ca
Compare
@shyim While I think this PR is valid and should be merged, since you created the original issue, if you were able to validate that this gives you what you need, that would help :) |
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 exchanged the interface with flysystem and it works fine for me. Thanks for your awesome work and sorry for my late reply.
src/Symfony/Component/AssetMapper/CompiledAssetMapperConfigReader.php
Outdated
Show resolved
Hide resolved
b3157ee
to
d01d19c
Compare
HtmlErrorRenderer Rebased and those tests are fixed. |
65c2b75
to
bad3998
Compare
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.
LGTM, just one minor question :)
src/Symfony/Component/AssetMapper/Path/LocalPublicAssetsFilesystem.php
Outdated
Show resolved
Hide resolved
bad3998
to
2937d07
Compare
@weaverryan Can you rebase this one? |
2937d07
to
11050dc
Compare
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.
Rebased and re-tested on a project locally. Should be good to go!
$entries = $this->importMapConfigReader->getEntries(); | ||
|
||
return $entries->has($moduleName) ? $entries->get($moduleName) : null; | ||
} |
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.
From an earlier PR - method was no longer used
d5f695c
to
48a2d68
Compare
Thank you @weaverryan. |
Hi!
An attempt at making AssetMapper flexible enough to support non-local filesystems. The "hook point" would be that you could replace the
asset_mapper.local_public_assets_filesystem
service with your own that implementsPublicAssetsFilesystemInterface
. I'm not worried about making the hook point super user-friendly: I just want the system to support this now, as trying to add this later (when we need to protect BC) will be harder.Cheers!