Module name: Multilingual Login Redirect
Project page: https://www.drupal.org/project/multilingual_login_redirect
Details:
Multilingual Login Redirect module allows you to redirect a user to a specific url or node number when he logs in depending on the actual language of the website and the user role.

Git repository: git clone --branch 8.x-1.x https://git.drupal.org/project/multilingual_login_redirect.git

Could you please review my new module for Drupal 8?

Thanks a lot

CommentFileSizeAuthor
#7 Testing.png29.56 KBmnsh1416

Comments

AlessandroSC created an issue. See original summary.

PA robot’s picture

PA robot commented
Issue summary: View changes
Status: Active » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgprojectmultilingual_login_r...

Fixed the git clone URL in the issue summary for non-maintainer users.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

cchanana’s picture

cchanana commented

Hello AlessandroSC,

I have reviewed your module, I found following code issues

1. In "multilingual_login_redirect.module" you have used \Drupal\multilingual_login_redirect\Entity\MultilingualRedirect number of times instead you can create an object of this class in construct and then use this object at required places to access the function.
2. In "multilingual_login_redirect.module" you have used null in lowercase for example "$current_redirect === null" , change null to NULL.

alessandrosc’s picture

alessandrosc commented

Hi cchanana,

Thank you very much for checking and for your feedback.

This should now be fixed in my last commit.

alessandrosc’s picture

alessandrosc commented
Status: Needs work » Needs review
hiramanpatil’s picture

hiramanpatil
Location Pune
commented

Pareview.sh showing some errors in code. Please resolve them.

https://pareview.sh/node/1855

Thanks,

mnsh1416’s picture

mnsh1416 commented
StatusFileSize
new Testing.png29.56 KB

Hey @AlessandroSC,

I have faced a functionality issue when I have configured this module on D (v8.3.2, see the screenshot) and when I try to login with a user who has a role of administrator, it redirects me to "http://localhost/drupal-8.3.2/user/node/3" instead of "http://localhost/drupal-8.3.2/node/3".

let me know if i am missing something?

alessandrosc’s picture

alessandrosc commented
Status: Needs review » Needs work

Hi all,

@hiramanpatil thanks for checking, I have fixed most of the issues tonight, I will finish the javascript issues in the next few days.

One error I have found difficult to resolve is this:
61 | WARNING | Role::loadMultiple calls should be avoided in classes,
| | use dependency injection instead

Do you know how and if I can pass the Role class as a dependency injection in the same way I have done with the LanguageManager class?

@mnsh1416 as I said in the documentation you should fill the fields with one of the following:
1- relative paths, for example /it/blog
2- absolute paths, for example http://mysite.com/it/blog
3- node number following this format: node:[node_id], for example node:20

In your case you are using a relative path without the slash in the front which means the redirect will not work as expected.

Please let me know if you find the doc page unclear, I will try to make it as clear as possible.

alessandrosc’s picture

alessandrosc commented
Status: Needs work » Needs review

Hello,

I have now fixed the pareview.sh issues, please check https://pareview.sh/node/1768

I have also fixed some minor typos and improved a bit the code.

Feel free to review this module and let me know if any other actions is required from me in order to be ok in the security application.

alessandrosc’s picture

alessandrosc commented
alessandrosc’s picture

alessandrosc commented

Hello,

can someone please check this?

I would like to have this module covered by the security policy at some point.

Thank you

dhayanandan_k’s picture

dhayanandan_k commented

Hi @alessandroSC,

I have checked the code in https://pareview.sh/node/1768. Still it has below issues. Please check.

FILE: ...t/repos/pareviewsh/pareview_temp/src/Entity/MultilingualRedirect.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
162 | WARNING | Unused variable $role_weight.
--------------------------------------------------------------------------

FILE: ...ot/repos/pareviewsh/pareview_temp/multilingual_login_redirect.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
37 | WARNING | Unused variable $weight.
--------------------------------------------------------------------------

Time: 49ms; Memory: 4Mb

Thanks,

Dhaya

dhayanandan_k’s picture

dhayanandan_k commented
Status: Needs review » Needs work
alessandrosc’s picture

alessandrosc commented
Status: Needs work » Needs review

Hi dhayanandan_k,

Thanks for spotting them, they are all fixed and pareview doesn't return errors anoymore.

sleitner’s picture

sleitner commented
Status: Needs review » Needs work

There are some issues on pareview, see details: https://pareview.sh/pareview/https-git.drupal.org-project-multilingual_l...

Review of the 8.x-1.x branch (commit 8ef1663):

  • Your README.txt does not follow best practices (headings need to be uppercase). .
  • The multilingual_login_redirect.module does not implement hook_help().
  • No automated test cases were found, did you consider writing PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Closed (won't fix)

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

I am closing this application due to lack of activity.