contrib icon indicating copy to clipboard operation
contrib copied to clipboard

Contrib Group Application: shashankthigale11 + oauth_client

Open shashankthigale11 opened this issue 1 year ago • 6 comments

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:

  • [x] Include a README.md file containing license and maintainer information. You can use this example: https://raw.githubusercontent.com/backdrop-ops/contrib/master/examples/README.md
  • [x] Include a LICENSE.txt file. You can use this example: https://raw.githubusercontent.com/backdrop-ops/contrib/master/examples/LICENSE.txt.
  • N/A: ~~If porting a Drupal 7 project, Maintain the Git history from Drupal~~.

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

shashankthigale11 avatar Apr 25 '24 10:04 shashankthigale11

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

quicksketch avatar May 03 '24 05:05 quicksketch

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 avatar May 03 '24 08:05 klonos

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 avatar May 03 '24 09:05 klonos

@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 :)

klonos avatar May 03 '24 09:05 klonos

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.

herbdool avatar May 03 '24 12:05 herbdool

@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...

  • [x] Include a README.md file containing license and maintainer information. You can use this example: https://raw.githubusercontent.com/backdrop-ops/contrib/master/examples/README.md
  • [x] Include a LICENSE.txt file. You can use this example: https://raw.githubusercontent.com/backdrop-ops/contrib/master/examples/LICENSE.txt.
  • N/A: ~~If porting a Drupal 7 project, Maintain the Git history from Drupal~~.

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 avatar May 04 '24 00:05 jenlampton

@shashankthigale11 we are going to close this issue as complete, but if you have any questions or need any assistance feel free to open a new issue.

jenlampton avatar Sep 05 '24 19:09 jenlampton