Skip to content
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

allow css/js asset directory to be relocated ('assetdirectory') #13063

Merged
merged 1 commit into from
Jan 7, 2015

Conversation

AdamWill
Copy link
Contributor

This allows the directory where CSS/JS asset collections are written to be changed, in case SERVERROOT is not writeable. Note it does not allow the expected URL to be changed: whatever directory is used, the server must be configured to serve it at WEBROOT/assets. It would be possible to add another config parameter to allow the admin to specify a custom asset URL, but I thought I'd keep the first implementation simple.

@karlitschek
Copy link
Contributor

@DeepDiver1975 what do you think?

@ghost
Copy link

ghost commented Dec 31, 2014

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@AdamWill
Copy link
Contributor Author

OK, so the problem I was hitting before was to do with the way I made $assetDir the /whatever/assets directory, rather than /whatever. I just changed the approach so that we say you can specify assetdirectory as the parent and the assets will always be put in a subdirectory of that directory, named assets. This is less of a change from the existing code and it seems to work in all cases for me. I tested without setting assetdirectory (so the assets wound up in /usr/share/owncloud/assets, as before), setting it as /var/lib/owncloud (so assets in /var/lib/owncloud/assets), setting it as /var/lib/owncloud/foo/bar (assets in /var/lib/owncloud/foo/bar/assets) and /srv (assets in /srv/assets). In each case it worked, and the generated CSS was the same, with the path to the images given as ../core/img/(filename).

So I'm pretty sure this will work if we're fine with the constraints - whatever directory you pick, you'll get an assets subdirectory, and you have to configure the server to serve that as $WEBROOT/assets.

I can look at making the asset path configurable later, but I'm a bit frazzled now...

@AdamWill AdamWill force-pushed the assets-relocate branch 2 times, most recently from 8c30ca7 to 1f883b3 Compare December 31, 2014 01:55
@DeepDiver1975
Copy link
Member

Thanks a lot @AdamWill - I'll review and test this asap.

@DeepDiver1975 DeepDiver1975 added this to the 8.0-current milestone Dec 31, 2014
@MorrisJobke
Copy link
Contributor

@owncloud-bot This is okay to test

@MorrisJobke
Copy link
Contributor

Works 👍 awesome

@ghost
Copy link

ghost commented Jan 5, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/5817/
👍 Test PASSed. 👍

* Where css and js assets are stored; this defaults to ``assets/`` in the
* ownCloud directory. The assets will be stored in a subdirectory of this
* directory named 'assets'. The server *must* be configured to serve that
* directory as $WEBROOT/assets.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlaschroder this has to be respected in the documentation as well - can you take care of this? Thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the current text kind of stinks (I wrote it for the first version of the code where you specified the asset directory directly rather than its parent, then just did a very small change when I changed the code approach, I think it's way too confusing ATM). I'll try and find a minute to fix that, but if I don't, don't just copy this for the docs, make it better...:)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeepDiver1975 My autogeneration tool of the documentation for config.sample.php will update the documentation just after the merge ;)

@carlaschroder
Copy link

@DeepDiver1975 , the manual page is auto-generated from config.sample.php, so as long it's correct there it will be spiffy in the manual. Thanks @AdamWill !

This allows the directory where CSS/JS asset collections are
written to be changed, in case SERVERROOT is not writeable. Note
it does *not* allow the expected URL to be changed: whatever
directory is used, the server must be configured to serve it
at WEBROOT/assets. It may be possible to add another config
parameter to allow the admin to specify a custom asset URL,
but I thought I'd keep the first implementation simple.
@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@DeepDiver1975
Copy link
Member

ok - let's :shipit: 👍 THX @AdamWill

DeepDiver1975 added a commit that referenced this pull request Jan 7, 2015
allow css/js asset directory to be relocated ('assetdirectory')
@DeepDiver1975 DeepDiver1975 merged commit 4628e98 into owncloud:master Jan 7, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants