Description
This module allows authors to create curated content that can be referenced later when creating content that includes the prepopulatedfields_field. An example would be to create a couple different nodes using different voices/tones, and then allowing content authors to pull in that data dynamically, with the ability to modify before saving the new content.
Current sandbox project
https://www.drupal.org/sandbox/dsvoboda/2616794
Git clone command
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/dsvoboda/2616794.git prepopulatedfields
cd prepopulatedfields
This is our first project submission, but we are excited to finally submit a project of our own!
Comments
Comment #2
PA robot CreditAttribution: PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxdsvoboda2616794git
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.
Comment #3
dsvoboda CreditAttribution: dsvoboda commentedThese issues should be resolved in the latest commit.
Comment #4
mmowers CreditAttribution: mmowers commentedHi @dsvoboda! Here's an initial manual review, based on the Project Application Review Template.
Manual Review
Recommendations are in bold italics below.
Comment #5
mmowers CreditAttribution: mmowers commentedComment #6
klausi@mmowers: those issues sound like good improvements, but are surely not application blockers. Anything else that you found or should this be RTBC instead?
Comment #7
mmowers CreditAttribution: mmowers commentedThanks @klausi, I'm still getting a hang of what bugs/issues warrant * and + designations. I'll edit the comment and remove the stars. Those are the only issues I saw, and I just took the module for a spin on my local env, and all looks good to me on that front too. I'm probably too new to this to make the call to move to RTBC, so I'll let you or someone else decide if my review looks sufficient. Thanks again.
Comment #8
dsvoboda CreditAttribution: dsvoboda commentedHello,
Thank you for reviewing our module. Just wanted to let you know that I've reworked the javascript to implement Drupal.behaviors, as well as made edits to the .info file per your recommendations.
This module is different from prepopulate in that prepopulatedfields allows you to define preset defaults for an individual field you can choose from in a dropdown below the input (textarea) - Prepopulate works with $_REQUEST variables where this one pulls data direct from a predefined content type. I've went ahead and added this to the project page as well.
Comment #9
legolasboAn additional review based on the one in #4
Automated Review
Automated review resulted in 3 issues. see http://pareview.sh/pareview/httpgitdrupalorgsandboxdsvoboda2616794git
Manual Review
In my opinion the differences raised in #8 are valid.
$help .= '<li>' . t('Create content of type <b>') . $content_type . t('</b> - <a href="/node/add/') . $clean_type . t('" target="_blank">click here</a>.') . '</li>';
Instead of breaking up the sentences you should use placeholders like @content_type or %content_type.
Does not seem to be used...
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.
While the issues on their own don't warrant going back to needs work I feel the project is not release candidate material just yet. Therefor I am marking the issue needs review.
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.
Comment #10
dsvoboda CreditAttribution: dsvoboda commentedlegolasbo, thank you for your input. Below is a list of my changes and responses!
One last note I didn't seem to notice anything marked with (+) or (*) - Wasn't sure if that got missed as well.
Thanks!
Comment #11
legolasbodsvoboda,
Any markup, no matter how small should be output using Drupal's theme layer to allow for changes by other modules and themes. By using a template you even increase the readability of the HTML you are outputting and thereby improve it's maintainability.
That's correct. In itself none of the changes mentioned warranted a + or *, but all together they made the module need work.
Comment #12
dsvoboda CreditAttribution: dsvoboda commentedlegolasbo,
I've worked theming functions into the module to handle the markup output in our latest commit.
Thanks!
Comment #13
dsvoboda CreditAttribution: dsvoboda commentedThought I'd check on the status of the module approval - Thanks.
Comment #14
th_tushar CreditAttribution: th_tushar commentedHi @dsvoboda,
I manually reviewed your code. Below are the findings.
There is a security vulnerability in your module code.
In above function, make sure the
$item['value']
variable get passed through appropriate sanitization function.Findings in
theme_prepopuluatedfields()
.l()
function should be used to generate link for$additional
variable.$additional
variable warning will occur.Changing the status to "Needs work". Change the status back to "Needs review" after fixing the above issues.
Comment #15
dsvoboda CreditAttribution: dsvoboda commented@th_tushar,
First I'd like to thank you for taking time to review our module.
In response to your concerns I've made a commit that should take care of the issues you brought up. You mention a warning if the
$available
variable isn't defined, however in the else it's set to an empty string if the user doesn't have adequate permission. The variable is defined as an empty string. If there's a different way you think we should go about handling that let us know, but we haven't run into an undefined variable warning in our tests.Let us know if there is anything else you'd like us to address. Additionally We'll begin working on a D8 version once the D7 version gets off the ground.
Thanks again!
Comment #16
klausimanual review:
<p>No templates have been created yet. ' .
: all user facing text must run through t() for translation. Please check all your strings.Comment #17
dsvoboda CreditAttribution: dsvoboda commented@klausi
I've made some changes to the code per your suggestions.
I've replaced the use of the constant with recalling the variable per your suggestion.
I've updated the code to use a passed object rather than creating it from within the theme.
I've replaced this with a translated string as well as in a few other spots that I had missed previously.
I've added an additional call to filter_xss_admin() in the option markup, but await what you find as a security vulnerability.
Thank you for your time!
Comment #18
th_tushar CreditAttribution: th_tushar commentedHi @dsvoboda,
I have manually reviewed your code, found the below issues.
There is a security vulnerability in the below lines of code in .module file,
$markup .= '<option value="' . $code . '">' . $value->title . '</option>';
The
$value->title
should be passed throughcheck_plain()
function. Also the$code
variable may not contain HTML and can not be too long as the value is being sourced from textarea. It should be a string/machine name of any entity to identify the selected value.In
prepopulatedfields_field_formatter_view()
function,$item['value']
should be passed throughfilter_xss()
as the field is also displayed for anonymous users.The link text of the
l()
function should be passed through translationt()
function.l('Create some','/node/add/' . $node_add, array('attributes' => array('target' => '_blank')));
it should be
l(t('Create some'),'/node/add/' . $node_add, array('attributes' => array('target' => '_blank')));
The value of '#default_value' should be string.
'#default_value' => isset($settings['prepopulatedfields_category']) ? $settings['prepopulatedfields_category'] : array(),
it should be
'#default_value' => isset($settings['prepopulatedfields_category']) ? $settings['prepopulatedfields_category'] : '',
Since the
$additional
variable is initialized on if user has permission, if user don't have permission, initialize it to empty string as below.user_access($permission) ? $additional = l('Create some','/node/add/' . $node_add, array('attributes' => array('target' => '_blank'))) . '.' : $additional = '';
On un-installation of the module, all the module related variables should be deleted from Drupal.
Changing the status to "Needs work". Please change the status back to "Needs review" once the above issues are fixed.
Comment #19
dsvoboda CreditAttribution: dsvoboda commented@th_tushar
Thanks again for your review, I've implemented your suggestions. To respond to a few of them:
The use of
filter_xss_admin()
instead offilter_xss()
should be more than adequate as it just increases the elements allowed, but still passes them through thefilter_xss()
: (from https://api.drupal.org/api/drupal/includes%21common.inc/function/filter_xss/7)And you mention a concern with the length of the
$code
variable possibly being too long as it's from a textarea, I'm unable to find a limit to the value attribute in the HTML spec - There are even people that have added 10 million character strings to values and the page renders fine (that's a lot of data!) Information can be read at this link (and links to spec docs on the page): http://stackoverflow.com/questions/1496096/is-there-a-limit-to-the-length-of-html-attributes.Let us know if there are any additional concerns we can address as we move forward with getting this module completed.
Thanks!
Comment #21
th_tushar CreditAttribution: th_tushar commentedComment #22
dsvoboda CreditAttribution: dsvoboda commented@th_tushar
I've made a commit addressing your concerns, I also posted comments detailing the changes I made, but they aren't showing up here. Not sure where they went. (Note the #19 and #20 are missing from the list of comments) - I think I double posted as I didn't see the first one show up. I assume they are still in the queue somewhere.
Specifically my concerns:
It's being passed through
filter_xss_admin()
which does the same thing thatfilter_xss()
does, but allows more generic HTML elements through, yet none of the elements it allows through offer any additional threat to the server, they are merely different elements that get cleaned through the same process. See https://api.drupal.org/api/drupal/includes%21common.inc/function/filter_xss_admin/7The other concern you brought up that I can't find any issue with is the worry that the content of
$code
that's stored in the select element's option value attribute may be too long, yet there's no indication of a limit in the HTML spec. So long as I'm sanitizing the code that's in the value there should be no concern. Now if I missed the limit, please let me know: http://w3c.github.io/html/, http://stackoverflow.com/questions/1496096/is-there-a-limit-to-the-length-of-html-attributes#answer-1496165Thanks!
Comment #23
dddave CreditAttribution: dddave commentedSorry dsvoboda, but Mollom unpublished your comments. @All: It would be really helpful if you guys could click on the confirm button under usernames of clearly legitimate users that try to jump through this hoop. Otherwise overzelaous Mollom is very likely to swallow comments containing code or links.
Comment #24
khurrami CreditAttribution: khurrami commentedHi,
Please correct followings in your prepopulatedfields.module file
Expected one space after the comma,
else {
$query = db_select('node','n');
Thanks
Comment #25
klausi@khurrami: looks like you forgot to change the status after your review. The minor whitespace issue is surely not an application blocker. Anything else that you found in your review or should this be set to RTBC?
Comment #26
dsvoboda CreditAttribution: dsvoboda commented@khurrami & @klausi,
I've updated the .module file to correct the space that was missing. Let me know if there's anything else I can do to move this forward.
Thanks!
Comment #27
dsvoboda CreditAttribution: dsvoboda commentedThought I'd check to see how the approval process is going? Has been waiting a while. Anything I can do to help?
Comment #28
Nandu.Kumar CreditAttribution: Nandu.Kumar commentedHello,
I reviewed the module and found errors.
kindly see the automated review for your project
https://pareview.sh/node/2979
FILE: /root/repos/pareviewsh/pareview_temp/prepopulatedfields.install
FILE: /root/repos/pareviewsh/pareview_temp/prepopulatedfields.module
Comment #29
PA robot CreditAttribution: PA robot commentedClosing 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.
Comment #30
dsvoboda CreditAttribution: dsvoboda commentedI've made changes found in comment #28. Changing status to "Needs Review"
Comment #31
nitesh624Automated Review
[Best practice issues identified by pareview.sh FILE: /root/repos/pareviewsh/pareview_temp/prepopulatedfields.module
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
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.
Comment #32
nitesh624Automated Review
[Best practice issues identified by pareview.sh FILE: /root/repos/pareviewsh/pareview_temp/prepopulatedfields.module
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
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.
Comment #33
nitesh624Sql queries can be more optimised...
Comment #34
nitesh624Automated Review
[Best practice issues identified by pareview.sh FILE: /root/repos/pareviewsh/pareview_temp/prepopulatedfields.module
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
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.
Comment #35
dsvoboda CreditAttribution: dsvoboda commented@nitesh624 I've removed the unnecessary default sort order on my select queries. Any other ideas on how you would optimize the queries? They seem pretty straightforward.
Thanks!
Comment #36
Satyam Upadhyay CreditAttribution: Satyam Upadhyay commentedHi @dsvoboda,
Firstly you should fix https://pareview.sh/node/2979 and i gone through your module there i found some issue like this:
Regards
Satyam
Comment #37
Satyam Upadhyay CreditAttribution: Satyam Upadhyay commentedHi @dsvoboda,
Sorry in my previous comment https://www.drupal.org/project/projectapplications/issues/2617672#commen... i could not update the status
Firstly you should fix https://pareview.sh/node/2979 and i gone through your module there i found some issue like this:
Regards
Satyam
Comment #38
dsvoboda CreditAttribution: dsvoboda commented@Satyam
I've removed the extra $ on line 165:
$element[$delta] = array('#markup' => $filtered_value);
The character length in the hook_help implementation shouldn't be an issue as there are many published modules that exceed 80 characters per line in their help section. This should not hold up approval of this module.
Thanks!
Comment #39
bkelly CreditAttribution: bkelly as a volunteer commentedHi dsvoboda -
Looks good to me. I'm marking it RTBC.
Thanks for the contribution.
- Bill Kelly
Comment #40
apadernoThank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:
You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
I thank all the dedicated reviewers as well.
Comment #41
apadernoComment #43
apadernoI am giving credits to the users who participated in this issue.