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

opnsense/core: automatically remove trailing slash #3361

Merged
merged 5 commits into from
Mar 28, 2019

Conversation

fabianfrz
Copy link
Member

removes also spaces for more safety.

closes #2647

@fichtner fichtner self-assigned this Mar 23, 2019
@fichtner
Copy link
Member

Wouldn't it be easier to add this to the validation?

@fabianfrz
Copy link
Member Author

@fichtner would be an option too - in this case it would autocorrect which is probably also a benefit for the user as he does not have to change it and save it again.

@fichtner
Copy link
Member

It's obscure. I'd rather want to go for a hard fail with a clear validation message. the damage is already done so to speak. We can make a not in the release notes...

@fabianfrz
Copy link
Member Author

@fichtner, ok then I will change it.

@fabianfrz
Copy link
Member Author

@fichtner this should throw an error in case the user does input something stupid

Co-Authored-By: fabianfrz <fabianfrz@users.noreply.github.com>
Co-Authored-By: fabianfrz <fabianfrz@users.noreply.github.com>
@fichtner fichtner merged commit cec7691 into opnsense:master Mar 28, 2019
@fichtner
Copy link
Member

Merged, thanks!

@fabianfrz fabianfrz deleted the nc_remove_trailing_slash branch March 28, 2019 11:15
@@ -9,6 +9,8 @@
</enabled>
<url type="TextField">
<Required>N</Required>
<mask>/^https?:\/\/.*[^\/]$/</mask>
Copy link
Member

Choose a reason for hiding this comment

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

Incidentally, I use cloudnow. It is an Austrian company. and I post here only my comment because it would be another alternative and it is synonymous of me for months running smoothly. maybe this would be another alternative?

grafik

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@opnsenseuser What do you want to tell me with your screenshot? It is very unlikely that the validation fails with your URL.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I have mispronounced myself. I just wanted to say that cloudnow settings work here as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure that other cloud services would also be configurable via the input mask. So would not it be advantageous to replace the term nextcloud with a generic term?

Copy link
Member

Choose a reason for hiding this comment

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

@fabianfrz I just wanted to make an improvement proposal. I imagine a drop down box, where you can select different cloud services. eg nextcloud, owncloud, cloudnow, etc. the data are then eg already pre-filled but still modifiable. I imagine that as in the dyndns settings Plugin from opnsense. what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@opnsenseuser nextcloud and owncloud are usually on premise solutions, so you have to explicitly configure the URL or the hostname anyway. In my case I have my own nextcloud on my own domain which I used for testing this module against. The reason why it is called nextcloud is that I do not test it against an owncloud instance (even if it very likely works because the same WebDAV library is working under the hood).

SAAS services are different. If it works, that's fine but I don't want to test against all of them (never ending story) and I currently cannot add anything else then the input fields (for example no JS to create something like a template directly in my file but that should be pluggable). Please note that this must follow the paths of nextcloud to work. Other WebDAV variants will not work as the nextcloud paths and responses are hardcoded in the file.

fichtner pushed a commit that referenced this pull request Mar 29, 2019
EugenMayer pushed a commit to KontextWork/opnsense_core that referenced this pull request Jul 22, 2019
EugenMayer pushed a commit to KontextWork/opnsense_core that referenced this pull request Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

System > Configuration > Backups > Nextcloud URL
3 participants