Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

DOCSP-42794: Laravel Passport #3113

Merged
merged 11 commits into from
Aug 29, 2024
Merged

DOCSP-42794: Laravel Passport #3113

merged 11 commits into from
Aug 29, 2024

Conversation

norareidy
Copy link
Contributor

@norareidy norareidy commented Aug 23, 2024

JIRA - https://jira.mongodb.org/browse/DOCSP-42794
Staging - https://preview-mongodbnorareidy.gatsbyjs.io/laravel/DOCSP-42794-passport/user-authentication/#laravel-passport

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

Sorry, something went wrong.

@github-actions github-actions bot added the docs label Aug 23, 2024
norareidy and others added 6 commits August 27, 2024 11:38
…aravel-mongodb into DOCSP-42794-laravel-passport
@norareidy norareidy marked this pull request as ready for review August 27, 2024 17:51
@norareidy norareidy requested a review from a team as a code owner August 27, 2024 17:51
@norareidy norareidy requested a review from GromNaN August 27, 2024 17:52
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

A single remark.

@norareidy norareidy requested a review from GromNaN August 28, 2024 20:15
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

a few things!


Laravel Passport is an OAuth 2.0 server implementation that offers
API authentication for Laravel applications. Use Laravel Passport if
your application requires OAuth2 support.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: maybe you can link to a resource describing Oauth and/or passport?

Comment on lines +207 to +210
'web' => [
'driver' => 'session',
'provider' => 'users',
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this other entry 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.

Yes the web entry is added automatically

'driver' => 'session',
'provider' => 'users',
],

Copy link
Contributor

Choose a reason for hiding this comment

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

S: remove space (will need to adjust emphasized lines)

Comment on lines 232 to 234
For example, the following code overrides the default ``Laravel\Passport\AuthCode``
model class by defining a ``MongoDB\Laravel\Passport\AuthCode`` class and including
the ``DocumentModel`` trait:
Copy link
Contributor

Choose a reason for hiding this comment

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

S: possibly clearer explanation:
Something like

Suggested change
For example, the following code overrides the default ``Laravel\Passport\AuthCode``
model class by defining a ``MongoDB\Laravel\Passport\AuthCode`` class and including
the ``DocumentModel`` trait:
The following example code extends the default ``Laravel\Passport\AuthCode``
model class when defining a ``MongoDB\Laravel\Passport\AuthCode`` class and includes
the ``DocumentModel`` trait:

protected $primaryKey = '_id';
protected $keyType = 'string';
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: does the user need to create models for each of the listed classes? If so, that should be more clear in a note or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah each one needs a custom model. I changed the wording of the paragraph below (line 246) to clarify that, lmk if it's clear enough

Comment on lines 255 to 257
You can now use Laravel Passport and MongoDB in your application. For more information, see
`Laravel Passport <https://laravel.com/docs/{+laravel-docs-version+}/passport>`__ in the
Laravel documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: recommend moving the link up to the intro

Suggested change
You can now use Laravel Passport and MongoDB in your application. For more information, see
`Laravel Passport <https://laravel.com/docs/{+laravel-docs-version+}/passport>`__ in the
Laravel documentation.
The, you can use Laravel Passport with MongoDB in your application.

@norareidy norareidy requested a review from rustagir August 29, 2024 17:49
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

lgtm!

@norareidy norareidy merged commit b84d583 into mongodb:4.6 Aug 29, 2024
26 checks passed
@norareidy norareidy deleted the DOCSP-42794-laravel-passport branch August 29, 2024 20:14
This was referenced Aug 29, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants