Closed sminnee closed 6 years ago
... if we state that the path of our core modules is not a public API and tell people to use the resource lookup APIs to find assets.
We've done this already: https://docs.silverstripe.org/en/4/changelogs/4.0.0#upgrade-module-paths-in-file-references. I've also checked for _DIR
and _PATH
in the core docs, and couldn't find any (meaningful) references to it any more.
Overall, I think it would be a lot cleaner if a default install of 4.0 moved core modules into the right folder already. It'll remove the uglyness of inconsistent folder naming (campaign-admin
vs. silverstripe-admin
), and emphasise the fact that you shouldn't rely on these constants.
@tractorcow How much work would the implementation of silverstripe-vendormodule
be? It sounds like it'll be a few LOC on https://github.com/composer/installers/blob/master/src/Composer/Installers/SilverStripeInstaller.php#L32, right?
These modules will be picked up by any silverstripe-* scan of packages
@micmania1 brought up a good point the other day: How much performance impact does module auto-discovery via ManifestFileFinder
have when it needs to scan vendor/*
as well as the first two directory levels of the project? I guess we can limit depth there as well, so it'll effectively be ~50 file stat calls for an average installation (~25 composer deps).
How much performance impact does module auto-discovery via ManifestFileFinder have when it needs to scan vendor/* as well as the first two directory levels of the project?
This code could probably use a refactor as part of the implementation of this ticket. The manifest code should probably first look for eligible modules and then scan only those modules.
Eligible modules can be found by:
vendor/composer/installed.json
and looking for the applicable "type" values. From there, infer the module path. This may necessitate a bit of duplication of the composer/installers logic but it shouldn't be too onerous (silverstripe-vendormodule vs silverstripe-module, and installer-name). Duplicating the logic would be a lot better than yanking composer into every project IMO.silverstripe-module
packages but also modules checked straight into source.Note that scanning installed.json and inferring paths means we can avoid opening every composer.json in the project.
Note that FRAMEWORK_PATH and FRAMEWORK_DIR will probably get set to vendor/silverstripe/framework
if we are using this feature (will need a test). That being the case, #7075 isn't a requirement.
That's important as it means that 3rd party code relying on FRAMEWORK_PATH will still work, even if it's deprecated it shouldn't be broken.
@tractorcow How much work would the implementation of silverstripe-vendormodule be? It sounds like it'll be a few LOC on https://github.com/composer/installers/blob/master/src/Composer/Installers/SilverStripeInstaller.php#L32, right?
Super easy.
How are you looking to address public assets served from vendor folder? That's the harder issue. We would need to remove htaccess protection from that folder, which may not be automatically-safe. :)
I'd actually love to do some kind of url-rewriting for these assets as a part of a larger change, implementing the ResourceURLGenerator for these modules. E.g. _client/silverstripe/framework/file.js
served from vendor/silverstripe/framework/client/file.js
, meaning libraries with no client
folder aren't disclosable. It could be implemented via nginx / htaccess so no PHP overhead.
The SilverStripe\Core\Manifest\ResourceURLGenerator
service was introduced to help with this problem. ResourceURLGenerator::urlForResource()
could copy the resources into a separate webroot path, or there could be a CLI tool for pre-syncing these. However, that's a bit more work.
I would prefer symlink / url rewriting instead of copying.
Symlink would probably be better than URL rewriting in the default case, as URL rewriting requires a lot of interaction with the web-server. On Apache this is doable but excludes anyone not using Apache for static assets.
Worth a chat with ops to see what is the best baseline approach. :)
Be careful of operating systems that don't support symlinks.
Laravel has a means for modules to publish public assets (https://laravel.com/docs/5.5/packages#public-assets) - and this looks nice because it would theoretically allow the publishing of assets off the webserver and to a CDN or some other location without having to work some custom server-side magic.
The RespurceURLGenerator interface would let us build an adapter to this part of Laravel, and that might be a useful module to have. However I'd probably say that we should bundle with a simpler implementation without too many extra dependencies.
Certainly symlink ops should fail over to recursive copy on systems that don't support them.
As a 2nd user story we probably want to have a system that preemptively copies assets during dev/build rather than just in time. A couple of filemtime ops per resource isn't massive in the scheme of things, but for prod environments it's useful to optimise what you can.
So the options are:
Each solution would have their own considerations. We'd need to make it exceptionally clear how to configure your site on a per-server basis.
My preference is symlinks implemented through composer plugin (we could build it into the recipe plugin), but we would need a fallback (could be done via dev/build). Only very old versions of windows don't support symlinks, as far as I know, so how often would it be necessary?
The other time that symlinks would be inappropriate is if you're running a webserver that doesn't follow symlinks out of the webroot. On a prod environment this might not be such a bad idea. Also, if you're bundling up your site as a .tar.gz after "composer install" is run, symlinks might not survive the process so well.
I see, so that's a good argument for shifting the symlink generation / attempt to dev/build. We could fall back to the copy if that failed, but we might run into the same issue (no write access out of web root).
It's also a good argument for using copy + composer install. ;-)
I don't think One True Approach is necessary or particularly likely. I'd say that symlinks + dev/build is more lightweight and so probably makes more sense as the default.
Where you're already bundling code into .tar.gzs prior to deploy, I could see the copy-based approach as making sense. In future, you could imagine warming caches such as compiled templates into this too.
Do we need to add an extensible build pipeline for dev/build? Its become something more than build the DB now.
Just a note: Symlink won't work with my development Vagrant box on a Windows host.
Thanks we'll check that out for our testing. :) Definitely can't rely on symlink directly.
Do we need to add an extensible build pipeline for dev/build
Couldn't we rely on composer hooks instead of our own dev/build mechanism? I'd prefer if dev/build did remain fairly restricted so we don't start coupling lots of different tasks to it.
Couldn't we rely on composer hooks instead of our own dev/build mechanism?
It was raised that some devs don't use composer to deploy, but rather upload / ftp a zip file. Thus dev/build is necessary to "repair" any lost symlinks that didn't survive the journey.
It was raised that some devs don't use composer to deploy, but rather upload / ftp a zip file. Thus dev/build is necessary to "repair" any lost symlinks that didn't survive the journey.
That sounds like a reason to use copy over symlinks IMO rather than justification for adding more reliance on dev/build.
If dev/build had some granular control so you could just publish assets or just rebuild config and so on, then maybe that'd be appropriate.. but some sites might not want to build the entire database just to republish an asset
That sounds like a reason to use copy over symlinks IMO rather than justification for adding more reliance on dev/build.
But what if your archive / deployment system doesn't support it? What if you are just a new dev and aren't aware how to safely archive and deploy symlinked folders? It's another major point of fragility.
but some sites might not want to build the entire database just to republish an asset
Who do you know who deploys a new site without a dev/build after deploying new files? We would be symlinking the entire folder, not individual files. If you really wanted to, you wouldn't need a dev/build unless you'd added/removed an entire module, in which case you'd need a dev/build anyway.
But what if your archive / deployment system doesn't support it? What if you are just a new dev and aren't aware how to safely archive and deploy symlinked folders? It's another major point of fragility.
I think we agree here, which is why I suggest not using symlinks at all.
As for Dev/build, that's only true because we don't have database migrations yet. I'd rather not build a new system that tightly couples non-db related tasks to our db building trigger just to take it apart later
The semantics of "dev/build" being "stuff to do after a deploy" seem okay to me. If we ever added something relating to database schemas that wasn't appropriate to do after every deploy, we'd probably need to put it somewhere other than dev/build, or better yet design it to be after-every-deploy safe.
Fun fact, it used to be "db/build" in v2.2 and was renamed with a view that we might introduce this, but we never did. ;-)
requireDefaultRecords()
is sometimes co-opted for post-deploy maintenance too.
I agree that dev/build
is an appropriate default place for this, but would like to see it available as a separate command as well for more granular control. Build pipelines in dev/build
would be nice for this, but it should also be achievable via a simple composable command that can be wrapped, right?
Where do we copy those files into? A public/
folder would be the natural choice, but that gets confusing because devs will assume it's the "webroot" when it isn't (.htaccess
directing traffic is one level up). A public/
folder as the webroot is the eventual goal, but I didn't get the impression that's the end goal for this particular Github issue. Weirdly, I can't find that work described elsewhere - do we need a new issue?
I think copy is a safe default, but we still need symlinks. You have to remember that this will impact every frontend asset you work with as a module developer, and those tend to change those a lot during development. Running a copy task every time you change a JS or CSS file simply isn't viable, regardless if it's coupled in dev/build
or not. So lack of symlink support (e.g. on Windows+Vagrant) would be painful, unless you implement your own file watcher build process that copies the files you're interested in (e.g. through Webpack). My assumption is that it won't affect developers on custom project frontend assets, since those continue to live in mysite/
and themes/
. With the eventual goal of public/
folder as webroot, mysite/
assets could move over to public/
(no copy/symlink needed). But assuming themes/
would live outside of public/
, they'd still need a copy/symlink - and increase the impact to developers.
Also note that the choice of symlinks vs. copy isn't per-project, but per-environment. You might support symlinks in prod, but not on a dev's local Windows/Vagrant combo. So if we implement symlinks, we need that option configurable as an environment constant (outside of the repo).
We need that option configurable as an environment constant
I'd keep it at the level of services that be configured. You can build yml that switches to a different service configuration based on the presence of an env var.
Proposed acceptance Critera:
I can install a module with type silverstripe-vendormodule
If this module has themes I can use those themes in my project
Modules of this type will be installed in the vendor
path
I can generate URLS to resources within this vendor module
I can generate paths to resources within this vendor module
If I deploy a website using at least one vendormodule I can reasonably expect that it will work normally
My vendor folder is secured from public web access
Excluded:
Nice to have:
base_path doesn't have to be the webroot
I have fine-degree control over web-visibility of module assets
Additions:
vendor
are not directly exposed through the webserver (without going through copy or symlink)I've created a working proof of concept for discussion at https://github.com/silverstripe/silverstripe-framework/pull/7395
Added two ACs after talking to TSP:
As discussed with @chillu we've covered the following:
Added two ACs:
More discussion; It seems that we may be better suited to move the copying logic OUT of framework and putting it into a new silverstripe/recipe-plugin
, as a composer plugin required by silverstripe-vendormodule
.
I think for development we would need a custom composer command composer vendorassets
to update all assets. You would need to run this after, for example, changing the branch in a vendor module to one with updated assets (unless of course you're using symlinks :)).
You would need to run this after, for example, changing the branch in a vendor module to one with updated assets
It's true that you'd need to run a command after changing your dependencies, but we could recommend people do that in post-update
hooks on their projects - I don't think that necessitates triggering it directly via a recipe plugin. We need the separate task anyway for hosting environments that run composer install --no-scripts
, and can't allow execution of arbitrary PHP in that context.
Yes I agree... next week I'll look at building the composer tool into something that can be installed globally.
I've created a follow up issue for actually converting the core modules to this: https://github.com/silverstripe/silverstripe-framework/issues/7405
Perhaps we can start by migrating just, say, siteconfig and get some feedback on issues of 4.x-dev users? On Wed, 27 Sep 2017 at 8:02 AM, Ingo Schommer notifications@github.com wrote:
I've created a follow up issue for actually converting the core modules to this: #7405 https://github.com/silverstripe/silverstripe-framework/issues/7405
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/silverstripe/silverstripe-framework/issues/7360#issuecomment-332349312, or mute the thread https://github.com/notifications/unsubscribe-auth/AADqQMMFLoRjVit63al-yeNWxwdTd1uvks5smXRfgaJpZM4PSgyP .
--
Sam Minnée
CEO
SilverStripe Limited
www.silverstripe.com
Phone: +64 4 978 7334
Twitter: @sminnee http://www.twitter.com/sminnee
Book a meeting with me https://freebusy.io/sam@silverstripe.com
https://www.facebook.com/silverstripe https://twitter.com/silverstripe https://www.linkedin.com/company/431285
Ever since we split SS4 into minipackages we've been chomping at the bit to shift modules to the vendor path. The current root path of a default install — let alone a CWP recipe — is very cluttered.
Rather than move all modules in one fell-swoop that would be invasive enough to count as an API breakage, I think we let individual modules choose when they're ready to be placed in the vendor path. Although we should recommend that people don't reference hard-coded paths, it's more likely that a module will do this about its own code than external code to do this. So making it a module decision rather than a project decision will be lower impact.
Specifically I think we support 2 new composer package types:
silverstripe-vendormodule
and maybesilverstripe-vendortheme
. These modules will be picked up by anysilverstripe-*
scan of packages, and we can patchcomposer/installers
to handle them properly.In addition to that patch, we'll want to ensure that the module manifest can find such modules.
Adding this capability is definitely a minor change. Making use of this in existing modules will be a minor change if and only if we state that the path of our core modules is not a public API and tell people to use the resource lookup APIs to find assets.
Acceptance Criteria:
vendor
are not directly exposed through the webserver (without going through copy or symlink)Excluded:
Nice to have:
Tasks
PRs
other things to review
I've put the plugin / cli tool on a separate repo to review before moving to
silverstripe
organisation