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

Dev: Make command to create new OAuth #35

Merged
merged 11 commits into from
Apr 18, 2023

Conversation

sammyskills
Copy link
Contributor

@sammyskills sammyskills commented Apr 3, 2023

This PR helps to quickly create and update the necessary file(s) to build a new OAuth.
Fixes #3

  • Create the base template for the OAuth.
  • Update config file.
  • Create language file.
  • Add docs.

@sammyskills
Copy link
Contributor Author

Why do we have this as callbake_url instead of callback_url?

$this->callbake_url = base_url('oauth/' . $this->config->call_back_route);

$this->callbake_url = base_url('oauth/' . $this->config->call_back_route);

Is it intentional?

@datamweb
Copy link
Owner

datamweb commented Apr 3, 2023

Is it intentional?

No just typo🫣.

@sammyskills
Copy link
Contributor Author

No just typo🫣.

Ok, fine.
Should I create a new PR to fix that or should it be included as a commit in this PR?

@datamweb
Copy link
Owner

datamweb commented Apr 3, 2023

Should I create a new PR to fix that or should it be included as a commit in this PR?

If you can, a new PR is the best choice.
please🥀

Copy link
Owner

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

Please run composer cs-fix.

src/Commands/Generators/NewShieldOauthGenerator.php Outdated Show resolved Hide resolved
src/Commands/Generators/NewShieldOauthGenerator.php Outdated Show resolved Hide resolved
src/Commands/Generators/NewShieldOauthGenerator.php Outdated Show resolved Hide resolved
@sammyskills
Copy link
Contributor Author

If you can, a new PR is the best choice. please🥀

Sent a PR #36

@sammyskills sammyskills requested a review from datamweb April 3, 2023 16:10
Copy link
Owner

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

So far so good, please continue!

@datamweb datamweb added the new feature PRs for new features label Apr 3, 2023
@datamweb datamweb marked this pull request as ready for review April 4, 2023 06:13
@datamweb datamweb marked this pull request as draft April 4, 2023 06:14
src/Commands/Generators/Views/newoauth.tpl.php Outdated Show resolved Hide resolved
src/Commands/Generators/Views/newoauth.tpl.php Outdated Show resolved Hide resolved
src/Commands/Generators/Views/newoauth.tpl.php Outdated Show resolved Hide resolved
src/Commands/Generators/Views/newoauth.tpl.php Outdated Show resolved Hide resolved
src/Commands/Generators/Views/newoauth.tpl.php Outdated Show resolved Hide resolved
src/Commands/Generators/Views/newoauth.tpl.php Outdated Show resolved Hide resolved
@sammyskills sammyskills marked this pull request as ready for review April 4, 2023 10:48
@sammyskills
Copy link
Contributor Author

About the languages files, will that still be necessary?

I ask because the number of files will definitely increase as more developers start using the library, and it may become very difficult to manage.

What do you think?

@datamweb
Copy link
Owner

datamweb commented Apr 4, 2023

What do you think?

Maybe it would be good to display the message and explain it.

@sammyskills
Copy link
Contributor Author

Maybe it would be good to display the message and explain it.

How do you suggest?

@datamweb
Copy link
Owner

datamweb commented Apr 4, 2023

How do you suggest?

Don't forget to translate the language strings. See https://github.com/datamweb/shield-oauth/blob/develop/docs/add_other_oauth.md#add-lang-file for details.

I'm not sure about this, the final decision is yours.

@sammyskills sammyskills requested a review from datamweb April 4, 2023 21:15
@sammyskills
Copy link
Contributor Author

I'm not sure about this, the final decision is yours.

Personally, I don't think it is ideal to automatically generate the language files for all languages. I feel it's best done manually, per PR, just like shield.

@datamweb
Copy link
Owner

datamweb commented Apr 4, 2023

Well, I run the command for the first time 🤩.

Delete file app/Config/ShieldOAuthConfig.php.
Run command php spark make:oauth yahoo.
Check out the path to app\Libraries\ShieldOAuth.

I think if file ShieldOAuthConfig.php doesn't exist, app\Libraries\ShieldOAuth\YahooOAuth.php shouldn't be created.

@sammyskills
Copy link
Contributor Author

Yes, you are correct!

@datamweb
Copy link
Owner

datamweb commented Apr 4, 2023

And we forgot the documents 😂.
Please update the documents.

@sammyskills
Copy link
Contributor Author

I think I finally saw the need to automatically create the default language file, and I've included it in this commit.

Please review and let me know what you think.

@sammyskills
Copy link
Contributor Author

Hi @datamweb,

Care to check the updates?

@datamweb
Copy link
Owner

@sammyskills Hi, sorry for the delay, I'm a bit busy, I'll try to check by the weekend.

Thank you!

@sammyskills sammyskills requested a review from datamweb April 18, 2023 13:42
@datamweb datamweb merged commit 8b82c5b into datamweb:develop Apr 18, 2023
@datamweb
Copy link
Owner

@sammyskills Thank you for your efforts.
I'm really sorry for the delay. Sometimes life is unpredictable.
I merged without checking, hope there is no problem.

@sammyskills
Copy link
Contributor Author

@sammyskills Thank you for your efforts. I'm really sorry for the delay. Sometimes life is unpredictable.

Hi @datamweb, I totally understand.

I merged without checking, hope there is no problem.

I tested before pushing the commit. But please check when you are less busy.
Thank you.

@sammyskills sammyskills deleted the make-oauth branch April 19, 2023 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature PRs for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev: CLI make:oauth To quickly build other OAuth
2 participants