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

Contrib Group Application: shashankthigale11 + oauth_client #780

Open
2 tasks done
shashankthigale11 opened this issue Apr 25, 2024 · 6 comments
Open
2 tasks done

Comments

@shashankthigale11
Copy link

shashankthigale11 commented Apr 25, 2024

Hello and welcome to the contrib application process! We're happy to have you :)

Please indicate how you intend to help the Backdrop community by joining this group

  • Option 1: I would like to contribute a project
  • Option 2: I would like to maintain a project, but have nothing to contribute at this time
  • Option 3: I would like to update documentation and/or triage issue queues

Based on your selection above, please provide the following information:

Option 1: I would like to contribute a project.

Name of the module: OAuth Client

This module is not ported from Drupal 7 and its a completely new module.

(option 1) Please note these 3 requirements for new contrib projects:

Post a link to your new Backdrop project under your own GitHub account (option 1)
https://github.com/shashankthigale11/oauth_client

If you have chosen option 2 or 1 above, do you agree to the Backdrop Contributed Project Agreement
YES

@quicksketch
Copy link
Member

Hi @shashankthigale11, I'm sorry for the delay. I'll try to get some eyes on this application.

@klonos
Copy link
Member

klonos commented May 3, 2024

Hey @shashankthigale11 👋🏼 ...apologies for the delay here, but the community has been busy with the upcoming minor core release. I'm having a look at your application now 👀

@klonos
Copy link
Member

klonos commented May 3, 2024

This module is not ported from Drupal 7 and its a completely new module.

Confirming that the namespace https://www.drupal.org/project/oauth_client does not exist 👍🏼 (only https://www.drupal.org/project/oauth2_client and https://www.drupal.org/project/openid_connect which has "OAuth client" in the project human-readable name, but no namespace clash). No clash in Backdrop contrib either 👍🏼

@shashankthigale11 can you please update this line in the README?:

Ported to Backdrop CMS by Shashank Thigale.

Since the module has not been ported over from Drupal, that line should be better as "Created for Backdrop by ...".

Both a README.md and an appropriate LICENSE.txt are included in the project, so we're all good there 👍🏼

Before we can make a final review and approve your application, I was wondering if you can please take the time to address the following things:

  • Go through the documentation available here, and fix the formatting in your code accordingly: https://docs.backdropcms.org/documentation/coding-and-documentation-standards ...I specifically (only quickly) checked oauth_client_config.inc and oauth_client.module and I see quite a few issues that should be addressed. That would make the initial review here easier, but also allow easier contributions by others in the community in the future.

  • I noticed that there is no configuration currently in oauth_client.settings.json. Is that intentional?

  • There is a test_mo_config() function and a test_mo_config() function in the .module file. Are these needed there? If so, then they should be prefixed with oauth_client_ I believe.

  • Then in oauth_client_uninstall() I see that you are doing a $config->clear(). I believe that that shouldn't be required if you implement hook_config_info(). Can you please try that instead?

  • I also tried installing the module in a demo instance via https://backdropcms.org/demo, but got the following error when I tried to enable the module:

    Failed opening required '/var/lib/tugboat/backdrop/modules/oauth_client/includes/Handler.php' (include_path='.:/usr/local/lib/php')

    I believe that that is because of the way that the required libraries are being loaded in the .module file:

    require_once BACKDROP_ROOT . '/modules/oauth_client/includes/Handler.php';
    require_once BACKDROP_ROOT . '/modules/oauth_client/includes/Utilities.php';

    You can see other examples in the rest of the contrib modules for Backdrop for clues on how to do that. I quickly found this for you (from the popular devel module): https://github.com/backdrop-contrib/devel/blob/1.x-1.x/devel.module#L426C3-L426C107

    @include_once BACKDROP_ROOT . '/' . backdrop_get_path('module', 'devel') . '/lib/krumo/class.krumo.php';

    ...but I also found a suggestion in https://stackoverflow.com/questions/31946670/how-to-include-a-php-class-in-drupal-7 that says that something like this should also work (note: untested!):

    module_load_include('php', 'mymodule', '../myclass.class');

    Since I wasn't able to install the module, I couldn't review anything further.

Apologies for the lengthy list of items I am requesting above - I hope that it doesn't deter you from contributing. Let me know when you have addressed the above issues, and I will have another look then ...or if you have any questions or need help with any of the above items in the meantime, then also let us know and we can assist you.

Thank you for contributing to Backdrop! 🙏🏼

@klonos
Copy link
Member

klonos commented May 3, 2024

@shashankthigale11 I also wanted to say that you can consider joining us in our official Zulip chat to get answers to any questions faster (you can use your GitHub account to join). You are free to join any existing thread, or start a new one, but this is where we are handling notifications about contrib applications: https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/Contrib.20Application

If you have any question, or you think we are taking too long to reply here, then please drop a message there, and our friendly community members will be happy to assist you :)

@herbdool
Copy link

herbdool commented May 3, 2024

I recommend using a namespace that makes it more explicit that this is for only Azure and related. Currently oauth_client makes it seem like it's much more generic.

@jenlampton
Copy link
Member

jenlampton commented May 4, 2024

@shashankthigale11 It looks like you've gotten a lot of feedback, but all of that is intended to be helpful - none of it is a requirement to join Backdrop Contrib. Since it appears that you have met the only 2 actual requirements...

An invitation to join the Backdrop Contrib group is on the way! 🎉 Please check your inbox and accept the invitation :)

Once you have accepted the invitation, feel free to transfer the repository into the backdrop-contrib group at any time (ask here if you have questions).

@jenlampton jenlampton changed the title Contrib Group Application: Contrib Group Application: shashankthigale11 + oauth_client May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants