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

DOC: rework readme #126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

DJCrashdummy
Copy link
Contributor

  • unified and corrected php-syntax
  • reworked IMAP description in the style of the XMPP description to prevent confusion with the order and numeration
  • added line breaks to XMPP description to make it easier readable
  • reworked SMB & HTTP heading
  • added jump-marks to headers for convenience
  • reordered sections to fit the order in the intro and at https://apps.nextcloud.com/apps/user_external

- unified and corrected php-syntax
- reworked IMAP description in the style of the XMPP description to prevent confusion with the order and numeration
- added line breaks to XMPP description to make it easier readable
- reworked SMB & HTTP heading
- added jump-marks to headers for convenience
- reordered sections to fit the order in the intro and at https://apps.nextcloud.com/apps/user_external
@DJCrashdummy
Copy link
Contributor Author

btw: in the brief description on top the additional backends and the "website" (appstore-url: https://apps.nextcloud.com/apps/user_external) would fit.

@violoncelloCH
Copy link
Member

hi, thanks very much!
would you mind to sign-off on your commit like requested by the Nextcloud contributing guidelines? https://github.com/nextcloud/server/blob/master/.github/CONTRIBUTING.md#sign-your-work
this can be done with git commit --amend -s for the last commit and then git push -f to push the changed commit back to github...

Copy link
Member

@violoncelloCH violoncelloCH left a comment

Choose a reason for hiding this comment

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

in general: nice improvements! thank you very much :)

README.md Outdated Show resolved Hide resolved
@DJCrashdummy
Copy link
Contributor Author

DJCrashdummy commented Feb 25, 2020

[...] sign-off on your commit like requested by the Nextcloud contributing guidelines?

sorry @violoncelloCH, i'm not a developer and thus i don't use the CLI in this case... is this somehow possible via the github-GUI?

@violoncelloCH
Copy link
Member

violoncelloCH commented Feb 26, 2020

[...] sign-off on your commit like requested by the Nextcloud contributing guidelines?

sorry @violoncelloCH, i'm not a developer and thus i don't use the CLI in this case... is this somehow possible via the github-GUI?

I see... basically you just need to have Signed-off-by: Your Name <your@mail.domain> as last line of the commit message... However I don't know if Github allows you to modify a commit message :/

@DJCrashdummy
Copy link
Contributor Author

sorry for asking already answered questions. 🤦‍♂️ i've read the link again and finally got it. 😉

However I don't know if Github allows you to modify a commit message

i've looked around quite a while, but i'm afraid it's not possible.

anyway... i'd prefer to not make my email-address public because of possible spam! 😒

README.md Outdated Show resolved Hide resolved
- parameter number `2` (the 3rd one): adapted options according to nextcloud#126 (comment) to fit nextcloud#122
- parameter number `3` (the 4th one): added the second possibility to allow all domains according to nextcloud#126 (comment)

Signed-off-by: DJCrashdummy <DJCrashdummy@users.noreply.github.com>
Signed-off-by: DJCrashdummy <DJCrashdummy@users.noreply.github.com>
@DJCrashdummy
Copy link
Contributor Author

are there for 2, 4 and 5 default values...? or is there an other reason to have this values in the example?
if not, i would rather use 4 => false (create uids with domains to be on the safe side) and 5 => true (groups can still easily be deleted afterwards) to prevent possible future problems for copycats.

@violoncelloCH
Copy link
Member

violoncelloCH commented Apr 2, 2020

as you can see here

public function __construct($mailbox, $port = null, $sslmode = null, $domain = null, $stripeDomain = true, $groupDomain = false) {
parent::__construct($mailbox);
$this->mailbox = $mailbox;
$this->port = $port === null ? 143 : $port;
$this->sslmode = $sslmode;
$this->domain = $domain === null ? '' : $domain;
$this->stripeDomain = $stripeDomain;
$this->groupDomain = $groupDomain;
}

there are default values for all of those... however those are more something which grew historically; meaning the default values were set so that nothing changes for existing setups which don't configure a value for the additional parameters...
so I guess it's safe to give a different example in the Readme if it's sensible and we can explain the choice :)

of the address.
`0`: IMAP server
`1`: IMAP server port (default `143`)
`2`: secure connection
Copy link
Member

Choose a reason for hiding this comment

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

okay, update for this parameter (also for the example above). We missed the case of starttls (as in #135). So code changes under way in #138. To conclude there is:

  • ssl for ssl/tls connections as normaly on port 993 which are encrypted from the beginning
  • tls for STARTTLS which is normally used on port 143 (so connection is initialized but then "upgraded" to a tls connection via a respective IMAP command with instructions)
  • notls (or technically any different value as of the implementations, but notls should be used) for plain connections (normally on port 143)

Copy link
Member

Choose a reason for hiding this comment

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

okay, this might change again, as there is an intention to default to STARTTLS (everything that's not explicit ssl or notls) in #140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants