[D8] user_redirect

Hi,
Can u please explain difference between login_destination.

Not blocker but good to consider:
No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

Nice work! looks good to me.

Hi benellefimostfa,
Pareview.sh is automated review, Please do some real testing for code break, security and blocker errors.

Thanks

Automated Review

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

No automated test cases were found, did you consider writing Simpletests or 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. You can also use the online version to check your project. You have to get a review bonus to get a review from me.
Source: http://pareview.sh/ - PAReview.sh online service

Manual Review

1. * Enabled module and User Register configuration set to user page, but after registration, i was seeing WSOD.
2. The configuration url should be with hypens admin/config/user-redirect/default
3. Instead of having access administration pages permission, if we have a custom permission, that would be great.
4. We should have a form id something like UserRedirect which would be matching our module, rather than default_form, which should be a unique one.

And their are similar modules Login redirect, the module maintainers are looking for co-maintainers, if possible you can work with 8.x for login_redirect module.

* Enabled module and User Register configuration set to user page, but after registration, i was seeing WSOD.

Since it is a major issue. moving back to Needs work

I am getting WSOD.

* Enabled module and User Register configuration set to user page, but after registration, i was seeing WSOD.

Also this changes are better practice.

• Instead of having access administration pages permission, if we have a custom permission, that would be great.
• We should have a form id something like UserRedirect which would be matching our module, rather than default_form, which should be a unique one.

Actually i was able to reproduce, lets wait for few other to review this :)

Both login & register work like a charm. Looks RTBC for me. Nice work everyone!

@tarekdj, in your reviews, it would help a lot to write up some notes of what you reviewed in the code. Project applications aren't so much a review of the functionality of the module, but rather a review of the coding ability of the person applying for git access.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

* No: Causes module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

* No: Does not follow the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (+) $url_object = \Drupal::service('path.validator')->getUrlIfValid($destination);, if the destination isn't valid, it will return FALSE and $url_object->setAbsolute(); will WSOD.
2. (+) http://cgit.drupalcode.org/sandbox-benellefimostfa-2676602/tree/composer... should list d8 as a requirement. drupal/core: 8.*
3. Small nit:Please enter un internal path without. Spelling error, un = an.
4. I'm not seeing you inject any services. That would make things a lot cleaner, like in your form. Then you wouldn't need to call off to \Drupal::, which is a code smell. Take a look at ConfigSync for an example that does this. In your case, since you are extending ConfigFormBase, you'll want to continue injecting the ConfigFactory, and also add the path validator service.
5. Suggestion: this modules requires that the path be an internal path (path.validator) since the validation only operates on internal URLs. Make it so external URLs can also be redirected.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I've marked this as Need Work for several reasons. First, due to project duplication. This is identical services to both https://www.drupal.org/project/login_destination & https://www.drupal.org/project/login_redirect. The second reason for marking Needs Work is that it doesn't include sufficient code to demonstrate knowledge of the Drupal APIs. I understand this is frustrating to you as an applicant, but this is somewhat a judgement call on the part of the git admin (myself).

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

There's also a similar module https://www.drupal.org/project/simple_redirect