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

[RFR] Documentation for the Ldap component #6402

Merged
merged 1 commit into from
May 29, 2016
Merged

[RFR] Documentation for the Ldap component #6402

merged 1 commit into from
May 29, 2016

Conversation

csarrazi
Copy link
Contributor

Q A
Doc fix? no
New docs? yes
Applies to 2.8, 3.0
Fixed tickets #6201 #5756

@javiereguiluz
Copy link
Member

@csarrazi thanks for this!! Bootstrapping documentation for a new component is very hard ... but at the same time very much appreciated!

I have a global question about the proposed docs. Sometimes you write LDAP server but most of the times you write Ldap server. Wikipedia always uses LDAP as an acronym. I propose to do the same ... except when referring to the name of the component, which apparently is Ldap. Thanks!

javiereguiluz
javiereguiluz reviewed Mar 29, 2016
View reviewed changes
components/ldap.rst
The Ldap Component
==================

The Ldap component provides a means to connect to an Ldap server (OpenLdap or Active Directory).
Copy link
Member

Choose a reason for hiding this comment

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

This description should be very concise, so you did it great. However, if this component adds LDAP support to the Symfony Security component, maybe we should mention it. Right now it looks like this component is stand-alone and not integrated with Symfony security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The component is indeed standalone. The security component actually is the one which integrates LDAP functionality in Symfony.

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
Contributor Author

Choose a reason for hiding this comment

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

Oh, and I'm actually currently writing the doc for the Security component, too, which is why the PR is still WIP.

@csarrazi
Copy link
Contributor Author

@javiereguiluz Yes, that's what is intended. However, as I wrote this quite late in the evening, I wasn't very consistent. Same thing with a / an before LDAP.

@csarrazi
Copy link
Contributor Author

Still needs to pass on all files to normalize Ldap <-> LDAP, and a -> an.

Also, cookbook/security/ldap.rst still needs some more work (the examples still need to be adapted, I only added stubs for now).

@csarrazi csarrazi mentioned this pull request Apr 1, 2016
Merged
@linaori
Copy link
Contributor

linaori commented Apr 13, 2016

Is there anything left to do on this PR? We're in the progress of updating our applications to Symfony 3 and the ldap packages used do not support 3.0 thus we will have to implement our own. Sadly the documentation is still missing :(

@csarrazi
Copy link
Contributor Author

The only remaining part is the cookbook, which is not finalized. Indeed, the configuration sections at the end is not ready yet: Only a few yaml examples are ready, and the configuration for other formats (php, xml) is definitely not ready, as I generally do not use these.

@csarrazi csarrazi changed the title [WIP] Documentation for the Ldap component [RFR] Documentation for the Ldap component Apr 16, 2016
@csarrazi
Copy link
Contributor Author

Done. @javiereguiluz could you take a look at it?

@csarrazi
Copy link
Contributor Author

By the way, this documentation is for version 2.8 and 3.0 of the Ldap component. Changes brought in 3.1 will be addressed in another PR.

javiereguiluz
javiereguiluz reviewed Apr 16, 2016
View reviewed changes
cookbook/security/ldap.rst

* Checking a user's password and fetching user information against an
LDAP server. This can be done using both the LDAP user provider and
either the LDAP form login or LDAP http basic authentication providers.
Copy link
Member

Choose a reason for hiding this comment

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

http -> HTTP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@javiereguiluz
Copy link
Member

I left some unimportant comments but overall I like this documentation a lot: you went straight to the point, everything is clearly explained and there are a ton of useful examples. You did a truly amazing job here. Thanks!

HeahDude
HeahDude reviewed Apr 16, 2016
View reviewed changes
components/ldap.rst
The Ldap Component
==================

The Ldap component provides a means to connect to an LDAP server (OpenLDAP or Active Directory).
Copy link
Contributor

@HeahDude HeahDude Apr 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It sounds weird to use an "a" with a plural, shouldn't it be "provides (the/some)? means"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is how the word means works. In its singular form, it still takes an s. :)

Also, as the Ldap component is not the only means to connect to an LDAP server, the article a is correct, here. :)

@csarrazi
Copy link
Contributor Author

Comments corrected.

By the way, the PR for 3.1 will be quite small in comparison, as the only difference will be the description of the new way to define the ldap service. However, it first needs this PR to be merged in all branches (2.8, 3.0, master/3.1).

@csarrazi
Copy link
Contributor Author

Are there still missing things here? I'd like to be able to work on adding the doc for version 3.1 of the Ldap component before the 3.1 release.

Ping @javiereguiluz

xabbuh
xabbuh reviewed May 21, 2016
View reviewed changes
components/ldap.rst
The :class:`Symfony\\Component\\Ldap\\LdapClient` class can be configured
using the following options:

* host: IP or hostname of the LDAP server
Copy link
Member

Choose a reason for hiding this comment

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

I would enclose the option names with literals (double backticks).

@csarrazi
Copy link
Contributor Author

Anything else to do @xabbuh, @javiereguiluz?

@javiereguiluz
Copy link
Member

@csarrazi nothing else for me ... except saying thank you for your great work!

I'm sure that doc maintainers will merge this soon. We want to document lots of things before the 3.1 release in 5 days.

@csarrazi
Copy link
Contributor Author

Yeah. And this needs to be merged so I can update the docs for 3.1.

@xabbuh
Copy link
Member

xabbuh commented May 29, 2016

@javiereguiluz I counted your last comment as a +1 vote. So I am going to merge here.

@xabbuh
Copy link
Member

xabbuh commented May 29, 2016 •
edited
Loading

@csarrazi Thanks for your work on this new feature!

@xabbuh xabbuh merged commit 305abb8 into symfony:2.8 May 29, 2016
xabbuh added a commit that referenced this pull request May 29, 2016
@xabbuh
feature #6402 [RFR] Documentation for the Ldap component (csarrazi)
d96ed74 
This PR was merged into the 2.8 branch.

Discussion
----------

[RFR] Documentation for the Ldap component

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | 2.8, 3.0
| Fixed tickets | #6201 #5756

Commits
-------

305abb8 Added documentation for the Ldap component
@csarrazi
Copy link
Contributor Author

You're welcome. I'll work on the 3.1 doc asap!

@xabbuh
Copy link
Member

xabbuh commented May 29, 2016

@csarrazi I merged your changes up into the 3.1 branch.

@csarrazi csarrazi deleted the doc/ldap branch September 19, 2016 17:59
@xabbuh xabbuh added the Ldap label Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Ldap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@csarrazi @javiereguiluz @linaori @xabbuh @HeahDude