[D7] Restricted Links

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

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.

Automated Review

Your module have some error that reported by the automated system. Those need to fix. http://pareview.sh/pareview/httpgitdrupalorgsandboxadam-id2372881git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause 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

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) After enable the module (without date module) getting this error. Ref rl-1.jpg
2. (*) Without the date module when viewing a saved node or editing a node getting these errors. Ref rl-2.jpg
3. (*) In content type default value settings section I have set Administrator role is default role and save. But again edit the field then the default value become reverted. Ref rl-3.jpg and rl-4.jpg
4. (*) I have 2 link field (Multi value field settings). I have enter links to only 1st field and save. On node view getting more error. Ref rl-5.jpg and rl-6.jpg
5. (*) Restricted links system should be optional. If I chose form content type settings then it will come otherwise not.
6. (*) According to your readme.txt file, "If you have the Date module enabled, you must enter an activation date". It is confusing because I can save a node without the date. This line should be relevant according to functionality.
7. (*) Active and Expire date do not follow the site timezone settings. So, for it you need to set a message about the timezone and functionality change to support the site timezone.
8. (+) There should be a option to exclude a role from the role list and if one role set default form field settings then the role selection should not come.
9. (+) Why this line features[features_api][] = api:1 present in module info file.
10. Module package should be relevant to the module.
11. Date field should be popup date field. It helps users to select a date quickly. It is for better usability.
12. Time field should be time picker or datetime's time field. It helps users to select a any time like 01:22 am or 10:20 PM etc. It is for better usability.

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.

For #2, #4

1) I have used a fresh drupal installation. Without the date module. (Ref rl-7.jpg)
2) Create a content type named Links.
3) Add a link field named link.
4) Save the content type.
5) Click add a new content and enter a link and save it. Then getting this error. (Ref rl-8.jpg)
6) Now click edit the content and getting this errors. (Ref rl-9.jpg)
7) Nothing change only click the save button and getting same error on front end. (Ref rl-10.jpg)
8) Now going to field settings and change numbers of value to 2 form 1. (Ref rl-11.jpg)
9) Again edit the same node and getting this type of view. (Ref rl-12.jpg)
10) No change only save the content and getting this errors. (Ref rl-13.jpg)

For #3
1) Edit link field settings form content type. Goto default value section I have select "Authenticate User" as default role for the links. (Ref rl-14.jpg)
2) Save the field settings.
3) Again click the edit link of the field. Gogo default value section but got the previous default value reverted to module default one. (Ref rl-15.jpg)
4) So, according to point 1 if i select only one role as default then the roles selection option should not come to frontend to select.

For #7
You use time function to calculate the current date/time but this time function return the datetime of the servers timezone. so, if you set any time according to your site then it will not work. User will set time according to the site timezone not the server and many of user never know what is the servers timezone. so, use drupal time functions to calculate date time.

#8
Discard this point. Same as #3. I have said same thing in different way.

#11
No need to use date popup. The drupal code have jquery ui datepicker and you can simply use it.

#12
I mainly said if i need 1:20 Am time then what can I do. So give input field rather than the dropdown.

Hello ,

Below are my findings of the code review of this module:

1. Wherever possible make use of t() function which will help in multilingual
2. Try making use of #attached css in place of drupal add css
3. Rather than printing html make a tpl so that others can modify it

Hello,
the module is interesting, but I encountered following issues. I used pure Drupal instalation with Link and Date module.

1. link does not appear, only label (3.png) - function theme() on line 215 (restricted_links.module) returns only following string " "
2. it would be helpfull to have some default time in Resticted links settings (midnight?)
3. restricted field settings fieldsets go beyond the edge of admin layer (1.png)
4. time restrictions should be optional
5. with multivalue fields administrator is forced to fill Active date even for empty fields (2.png)
6. tabs instead of 2 spaces in code (eg. function restricted_links_field_widget_link_field_form_alter())

Please, take a look at: http://pareview.sh

EDIT: removed long pareview.sh dump.

Manual Review

Add Implements hook_help() in your module file

Automated Review

Link to pareview resultsPareview is still showing some minor formatting problems, mostly indentation issues.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation. I can't find anything that's even similar, and I can see that this could be a very useful thing to have.

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. The README is well written and contains good instructions for getting started with the module.

Code long/complex enough for review

Yes: Follows 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. * When the date module is enabled, there is a warning - Warning: Illegal string offset 'checked' in restricted_links_field_widget_link_field_form_alter() line 160 of restricted_links.module This is preventing the module from functioning correctly, when the line is commented out, everything works as expected. To be honest, given that there is a default value, it doesn’t seem strictly necessary to make that a required field, imo.

In general, this looks very good. Multivalued fields work as expected with distinct settings, there are no remaining issues with the admin display, all tested permutations work as they ought to. The warning needs to be fixed, but otherwise, this is looking great! Very cool little module.

This review uses the Project Application Review Template.

Assigning to myself for next review.

Automated Review :
http://pareview.sh/pareview/httpgitdrupalorgsandboxadam-id2372881git

Please specify on the project page how the module functionality differ from the field_permissions module.Drupal community holds a strong "collaboration rather than competition".

Please add this to the project page and the issue summary above so that it will be clear to next reviwers.Thanks! for specifying.
Assigning to myself for next review.

Automated Review

No Best practice issues identified by pareview.sh / drupalcs / coder. Please take a look at http://pareview.sh/pareview/httpgitdrupalorgsandboxadam-id2372881git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause 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

Yes: Follows 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. (*) The restricted links formatter view display the warnings after changing the link field widget display.I edited the same node again and saved it and the error disappears.Steps to reproduce.Add a link field from UI in any content type.Set the field display to restricted links from the display settings and create the node.Again come to the display settings and change the display settings and update.Again change the field widget to 'Restricted link' you will see the warnings on the node page.
2. function template_preprocess_restricted_widget : Its preferrable to use the #attaced over drupal_add_css.

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.

Please take a review bonus to come up in hight priority list so that you will get the reviews from git admins.

This review uses the Project Application Review Template.

Attached the screenshot.Sorry for coming late was bit busy with some other stuff.

2) Based on that, is this minor notice really one that should hold up the release of a module?

Please add this notice on the issue summary page so that others reviewer already aware of this.Just discussed with mpdonadio over IRC.We can set it to Needs Review

I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :)

Hi admurray,

I found your module interesting, especially for its date-restriction capability, that is not available in field_permission module, as you pointed out.
So here is my review.

Automated Review

There is a warning reported by http://pareview.sh/pareview/httpgitdrupalorgsandboxadam-id2372881git:
FILE: /var/www/drupal-7-pareview/pareview_temp/restricted_links.module

---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
43 | WARNING | Redeclaration of function parameter $user as global variable.

Having looked at the code, I'm surprised: sure it does not prevent the code from running, but what's the point?
The function _restricted_link_check($item, $user = NULL) is only invoked once, as _restricted_links_check($item);, so you always get $user = NULL, and finally the $user argument is useless. It should be suppressed, as well as the if (!isset($user)), IMO.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause 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.
But you should review the confusing information about enabling date restriction (line 33), as already suggested by @saniyat (#3, 6).

Code long/complex enough for review

Yes: Follows 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. This first point is rather addressed to the community: despite what was pointed out by @saniyat (#3, 10), I think that "Package" name in _restricted_links.info should better be set to "Fields", so (consistency) featuring in the same group as the Link module; also because, as a Drupal user, I generally prefer not to have a huge list of groups, often with only one module each!
2. Regarding the module implementation strategy: currently if we want a link field to become restricted we get it formatted as "Title, as link", and none of the other link options are available.
   So for a more fiendly result, why not to merely:
   1. NOT add a supplemental formatter: so all of the link display options remain available
   2. have the "Restricted links settings" group always added into the field group when editing content: since restricted_links defaults to no restriction it will not change anything to the link behaviour till user changes it
3. A very minor inconstency: with "Link expires" checked and no date entered, click "Save" button works.
4. (*) A major inconstency, even if it doesn't break functionality: with "Link expires" checked, uncheck "Use date restrictions" doesn't hide the "Expire date" group (should change '#states' => ['invisible'... condition (module, line 181 and next ones) into 'visible' with both checkboxes TRUE).
5. A questionable feature: (module, lines 54-57) when the link is not active or already expired, its label (if not hidden by definition) keeps exposed in resulting page, with no content. What"s the point to have a label for something which is not simply empty but really "does not exist" (at least for now, or for the current user)
6. Having no default value for the expiration date, it is really painful to enter each of its components!
   Even if obviously there is no realistic way to presume which default date would reach the user need, any arbitrary date could be helpful: why not to simply use the activation date (notably because we probably often keep hours/minutes unchanged)?
   Furtherly, custom setting of a delay (number of hours, days, ...) based on activation date would help too.
7. (*) Currently the date format is hard fixed, so it may be embarrassing for some non-English users (like me :-): as a minimum level of internationalization, it should at least be set to variable_get('date_format_short', 'm/d/Y - H:i') as date module suggests.
   Furtherly, custom settings capabilities would be very welcome, including making hours/minutes optional as already suggested by @jardac (#18, 4).
8. As suggested by @mudi.. (#23), an implementation of hook_help would be useful (when online, README.txt is no longer available).
9. Maybe I miss some issue with that, but something puzzles me: why to have made this module dependent on Date module, instead of using Drupal available date- and time-pickers as other reviewers suggested above? This way, it seems to me that it were be more simple.

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.

Please don't feel attacked by this long review! Above are only 2 points that should be (helpfully easily) corrected. Other ones are only suggestions and don't hold up release, IMO.
BTW, I looked at the notice that was mentioned by @naveenvalecha (#37, 1): you probably already observed that it comes from drupal_attributes() interpretation of some data passed thru l(), so clearly coming from lines 52 and 59 in theme.inc.
I'm not familiar enough with this part of Drupal code, so I can't exactly explain what happens, but I guess it regards line 43, where $link_options is populated by the $vars['elements']: when previously due to pure "Link" module's, they probably contain some not-array attributes-list (either null, empty or simple string). HTH.

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.