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.

Sign up for GitHub

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

Jump to bottom

added parameter default values #3087

Closed
wants to merge 4 commits into from
Closed

added parameter default values #3087

wants to merge 4 commits into from

Conversation

j-ed
Copy link
Contributor

@j-ed j-ed commented Jan 16, 2017 •
edited
Loading

Fix #3069

Based on a source code analysis (looking for getSystemValue() function) I added the default values of the parameters.

  • Unfortunately for several parameters different default values are used in the source code and my question how this should be addressed has not been answered yet. Examples:
getSystemValue('dbtype', 'sqlite')
getSystemValue('dbtype', '')
getSystemValue('dbtype')
  • For other parameters contradictory values are used. Examples:
getSystemValue('preview_max_x', 2048)
getSystemValue('preview_max_x', 1024)
or
getSystemValue('preview_max_y', 2048)
getSystemValue('preview_max_y', 1024)
  • Additionally some of the parameters have still not been adjusted for Nextcloud. Examples:
getSystemValue('dbname', 'owncloud')
getSystemValue('dbtableprefix', 'oc_')

I followed the recommendation of @nickvergessen and created separate issue tickets for each parameter.

@j-ed
added parameter default values
d2927af 
based on a source code analysis (looking for getSystemValue() function) I added the default values of the prameters
@mention-bot
Copy link

@j-ed, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @georgehrke and @oparoz to be potential reviewers.

@nickvergessen nickvergessen added the 3. to review Waiting for reviews label Jan 16, 2017
@oparoz
Copy link
Member

oparoz commented Jan 16, 2017

👍

MorrisJobke
MorrisJobke reviewed Jan 16, 2017
View reviewed changes
config/config.sample.php
@@ -74,8 +74,10 @@


/**
* Where user files are stored; this defaults to ``data/`` in the Nextcloud
* directory. The SQLite database is also stored here, when you use SQLite.
* Where user files are stored.The SQLite database is also stored here, when
Copy link
Member

Choose a reason for hiding this comment

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

Space before The missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[x] fixed.

nickvergessen
nickvergessen reviewed Jan 16, 2017
View reviewed changes
config/config.sample.php
@@ -762,6 +762,11 @@
/**
* This section is for configuring the download links for Nextcloud clients, as
* seen in the first-run wizard and on Personal pages.
*
* Defaults to
* * Desktop client: ```https://nextcloud.com/install/#install-clients```
Copy link
Member

Choose a reason for hiding this comment

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

should be two ticks only on each side, not 3

@MorrisJobke
Copy link
Member

@j-ed Sorry for not bringing this up earlier: we require a sign-off for your commits. You can read the details here: https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work

Thanks for the contribution and it now looks good. Once the sign-off is added to the commits we can merge it :)

@MorrisJobke
Copy link
Member

Thanks for the contribution and it now looks good. Once the sign-off is added to the commits we can merge it :)

Then also CI gets green ;)

nickvergessen
nickvergessen requested changes Jan 17, 2017
View reviewed changes
config/config.sample.php
@@ -1182,12 +1322,16 @@
* When changing this, note that older unsupported versions of the Nextcloud desktop
* client may not function as expected, and could lead to permanent data loss for
* clients or other unexpected results.
*
* Defaults to ``2.2.0``
*/
'minimum.supported.desktop.version' => '2.0.0',
Copy link
Member

Choose a reason for hiding this comment

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

I'd also adjust the sample value here to match the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed typo after double-checking which default is used in the source code.

config/config.sample.php
*/
'share_folder' => '/',

/**
* If you are applying a theme to Nextcloud, enter the name of the theme here.
* The default location for themes is ``nextcloud/themes/``.
*
* Default to ``themes/`` in Nextcloud directory
Copy link
Member

Choose a reason for hiding this comment

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

No, this is not the theme directory, but the actual theme. This has no default value:
Defaults to the theming app which is shipped since Nextcloud 9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[x] done.

config/config.sample.php
@@ -1140,6 +1272,8 @@
* use.
*
* The Web server user must have write access to this directory.
*
* Defaults to ``''`` (empty string)
Copy link
Member

Choose a reason for hiding this comment

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

defaults to null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

default information removed.

config/config.sample.php
@@ -1025,6 +1148,8 @@
* to be fetched in addition to any requested file.
*
* One way to test is applying for a trystack account at http://trystack.org/
*
* Defaults to ``''`` (empty string)
Copy link
Member

Choose a reason for hiding this comment

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

It's an array and has no default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[x] done.

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 17, 2017
@ChristophWurst
Copy link
Member

nickvergessen requested changes

I've adjusted the label accordingly

@j-ed
applied various fixes
8693942 
- removed default value for 'objectstore' because it has not default value.
- removed default value for 'tempdirectory' because it is unset.
- changed default value for 'theme' to "Defaults to the theming app which is shipped since Nextcloud 9"
- fixed typo in default value of 'minimum.supported.desktop.version', after double-checking the version
  number in /apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php:71.

Signed-off-by: Juergen Edner <juergen@eisfair.org>
@j-ed
Copy link
Contributor Author

j-ed commented Jan 17, 2017

@MorrisJobke I've added a 'Signed-off-by:" line to my last commit.

@MorrisJobke
Copy link
Member

@MorrisJobke I've added a 'Signed-off-by:" line to my last commit.

It needs to be done for all commits. I will squash them into one and merge this here then.

@MorrisJobke MorrisJobke mentioned this pull request Jan 18, 2017
Merged
@MorrisJobke
Copy link
Member

It needs to be done for all commits. I will squash them into one and merge this here then.

Done in #3133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke left review comments

@nickvergessen nickvergessen nickvergessen requested changes

Assignees
No one assigned
Labels
2. developing Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add default value of parameters to parameter description in config.sample.php and the documentation
6 participants
@j-ed @mention-bot @oparoz @MorrisJobke @ChristophWurst @nickvergessen