Comments

bappa.sarkar’s picture

Can anyone review this module?

bappa.sarkar’s picture

Status:Active» Needs review
PA robot’s picture

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.

Ayesh’s picture

Hi Bappa,
Thank you very much for your effort in the application.

I have used Drupal Tweaks module that can increase the PHP memory limit and maximum execution time via the UI.

Manual review

phpconfig.info

- I think it would be better if we change the package to somewhere we usually use. It's unlikely that there will be another module to adjust PHP configuration.
"Development" or such category would be better I think.

phpconfig.module

- In the hook_menu implementation for the admin/build/phpconfig/add, it's not necessary to use a separate function just to return the form.
The following should work same but we no longer need phpconfig_add function.

$items['admin/build/phpconfig/add'] = array(
  'title' => 'Add new PHP configuration',
  'page callback' => 'drupal_get_form',
  'page arguments' => array('phpconfig_newform'),
  'access arguments' => array('use phpconfig'),
  'file' => 'phpconfig.inc',
  'type' => MENU_CALLBACK,
);

- You can also simplify the module by using woldcard loaders right in your hook_schema implementation.
For example you now have a function named phpconfig_edit that actually loads the given config item and returns the phpconfig_newform.
With wildcard loaders, you will define a phpconfig_config_item_load<code>-like function and then you can use <code>%phpconfig_config_item as the hook_menu item's wildcard. Drupal will load the item for you, and will even throw 404 errors if the config item is now found.
It's more of the extreme side for now though.

phpconfig.inc

- You should now call drupal_goto() in a submit function. drupal_goto() will stop the rest of the page from being executed (which may seem to work at the moment, but if some other module altered the form and added its own submit handlers, they will not be executed).
Remove the drupal_goto() and you can get the same functionality as following.

- drupal_goto('admin/build/phpconfig');
+ $form['#redirect'] = 'admin/build/phpconfig';

Otherwise everything looks really great. Drupal Tweaks module can do the basic PHP configuration changes (memory limit, max_execution_time) and your module fills the gap and allows everything to be changed.
Consider adding some links to php.net docs about settings that can be changed in the runtime.
http://www.php.net/manual/en/configuration.changes.modes.php
http://www.php.net/manual/en/ini.list.php

I wish you all the best!

Ayesh’s picture

Status:Needs review» Needs work

I'm marking this as "needs work" because the drupal_goto usage is more of a problem than an improvement. Feel free to let us know when you have the changes done.
(I'm just a user trying to keep the application queue. Probably Klousi or someone will have better thoughts)

bappa.sarkar’s picture

Status:Needs work» Needs review

Thanks Ayesh for your comments! Really appreciating!
I have replaced drupal_goto with $form['#redirect']. Also removed phpconfig_add function and added drupal_get_form in the menu hook. I have also fixed the edit function.

abghosh82’s picture

Status:Needs work» Needs review

I have installed the module and had a look through the code and used it. I would like to mention that this module is a very good utility for a novice user.

I have reviewed the code and have a couple of suggestions below:

  1. In the hook_boot if you can check that with ini_get the current value and if that matches what is to be set, then you avoid setting it again to the same value.
  2. I tried to set a wrong value for memory_limit like "xyz", I got a white page, you can avoid this by validating the expected format against the item and preventing the ini_set. As for a novice user this would be confusing and also better to handle errors gracefully :)
abghosh82’s picture

Status:Needs review» Needs work

As per the above comments I am setting it to needs work.

Ayesh’s picture

Status:Needs review» Needs work

I too like this module because it's really straight forward and less confusing.
It would be great if we could put some fields that makes sense and easy to use. For example, for single on/off flags (register_globals, for example), we can use a checkbox or radios, while we can display a size-related field for memory_limit, post_max_size, etc. Ideally a field to enter a numeric value and a select list to choose MB, GB, etc.

SoumyaDas’s picture

It's a very good utility module. Keep up the good work.

bappa.sarkar’s picture

Status:Needs work» Needs review

A new confirm form has been added to give user a warning about WSOD.

bappa.sarkar’s picture

Thanks ssdas!

fuzzy76’s picture

Assigned:Unassigned» fuzzy76
fuzzy76’s picture

  1. When adding a new config option, the confirmation page causes notice: Undefined index: status in phpconfig_newform_submit() (line 142 of /Users/fuzzy76/public_html/test/drupal-6.x-dev/sites/all/modules/phpconfig/phpconfig.inc). You should probably enable all warnings when developing to catch these.
  2. I am able to add the same option multiple times. Not necessarily a bad thing, just thought I'd mention it.
  3. phpconfig_newform() has several strings not wrapped in t().
  4. You also have one warning left on pareview.sh: http://pareview.sh/pareview/httpgitdrupalorgsandboxbappasarkar2010602git
  5. The options are never filtered in any way. But I guess there aren't any meaningful way to filter them since the options can contain any kind of string data. And if someone has permissions to add PHP options, they can pretty much do what they want with your site.
fuzzy76’s picture

Status:Needs review» Needs work
greggles’s picture

If the module is risky (as this is) then there should be a warning on the project page (there is) the README.txt (there isn't) and the permission should have a scary name like "administer phpconfig - can have stability and security implications" (it doesn't). In Drupal 7 you can use the description and "restrict access" permission array elements to give a more complete explanation.

bappa.sarkar’s picture

Status:Needs work» Needs review

Following changes done.

  1. PHP notice removed.
  2. All messages wrapped inside t().
  3. Sniffer issues fixed.
  4. Permission renamed to 'administer phpconfig'
  5. No duplicate configurations.
  6. Checking for WSOD for new configuration as a part of validation using AJAX. So NO WSOD!
greggles’s picture

The new AJAX test for configurations seems cool. It should ideally be wrapped with a drupal_get_token/drupal_valid_token csrf protection to avoid a scenario where someone uses CSRF to send a php setting that sets memory and execution time to very high values and then sends a bunch of values to try to create a DOS scenario.

geberele’s picture

StatusFileSize
new162.03 KB

Hi bappa.sarkar,
really cool module and really powerfull, it will be used a lot if you add more information. For example you can create a new page inside your module with a list of 'Configuration options'.

I've created a patch for you with a new configuration of the menu in tabs and a page with the list of 'Configuration options'.

Ayesh’s picture

Nice additions, Gabriele!
This may be an extreme end request, but can we improve the table to show all possible values ?

For example, ini_get_all() returns all configuration options, and IMO, ideal for this list.
We can also list the current configured value as well as a link to change that specific configuration option with the config name prefilled.

I will try to show you an example with a patch but I'm stuck in a small workshop these day.

geberele’s picture

StatusFileSize
new162 KB

Thanks Ayesh!
I've just noticed a wrong row in the previous patch, the new one attached has the fix.

ini_get_all() is a good one, good idea!!

Ideally it will be more cool if it has 'select' fields instead of normal textfield or textfield with autocomplete at least for the field 'Item'.

bappa.sarkar’s picture

You can't add all settings using ini_set. only PHP_INI_USER and PHP_INI_ALL can be added. Please see Changeable column in http://www.php.net/manual/en/ini.list.php

Ref: http://www.php.net/manual/en/configuration.changes.modes.php

@Geberele : It would be really nice to add a select box for item. But a new php extension comes with new ini setting options. So it is not always constant.

Cheers,
Bappa

bappa.sarkar’s picture

Hi greggles,

I have added access check in boot hook to prevent testing phpconfig without access. I was not able to use drupal_valid_token as the function is not present at boot level. I used more robust access check by querying permission table based on user role.

Thanks,
Bappa

greggles’s picture

Status:Needs review» Needs work

@bappa.sarkar - prevent csrf is different from preventing access bypass. It's possible to have CSRF without access bypass (that is usually the case, in fact). There are some articles on csrf at http://drupalscout.com/tags/csrf that might be helpful. If you need to bootstrap further to get more functions it seems fine to me to use drupal_bootstrap inside of your AJAX code since it runs infrequently.

Ayesh’s picture

Status:Needs work» Needs review

Hi Bappa,
Sorry for mentioning these too late. But I do have some comments about the current state of your module.

- Use of $_GET['q'], 'value' and 'item': value and item are common phrases, so if some other module accidentally sent the user to a page, for example, http://example.com/add-to-wishlist?item=beer&value=99 , it will attempt to ini_set('beer', 99);

Furthermore, I have never seen an ini_set call gives a fatal error. (try the following. Setting a )

<?php
ini_set
('beer', 99); // arbitrary setting. 
ini_set('extension_dir', '/etc/php/ext'); // PHP_INI_SYSTEM configuration; ignored.
ini_set('extension_dir', array()); // only a warning.
echo 'Cool!';
?>

No errors. Cool!

I think for this validation part, an ini_get() and checking if the config name exists in the returned value would be a better validation.

<?php
dvm
(ini_get('extension_dir')); // returns a string
dvm(ini_get('register_globals')); // string 0.
dvm(ini_get('register_globals')); // bool false - This value do not need to be set.
?>

I really think this module would be a really useful one in the future without allowing non-techie users to update PHP configuration. Let's polish it up!

bappa.sarkar’s picture

Hi Ayesh,

Thanks once again for pointing this out. Yes item and value are the common phrases but ini_set will only execute if $_GET['q'] is "admin/settings/phpconfig/test" and the user has permission administer phpconfig.

Thanks,
Bappa

greggles’s picture

Status:Needs review» Needs work

Fixing crosspost from #24.

Ayesh’s picture

Sorry about the false positive. I just did some tests with the latest version (a75539a3b970d3537dbf1549a62483d81a010e73) and works fine at my end - it's same in functional code as the previous version, though.

Good luck!

bappa.sarkar’s picture

Assigned:fuzzy76» Unassigned
Status:Needs work» Needs review

Hi greggles,

Thanks for the link that you have provided about CSRF, the tumblr video is really awesome.

I have added token validation in my module to handle CSRF. Please give me your feedback.

Thanks for the knowledge!

Cheers,
Bappa

greggles’s picture

Status:Needs review» Needs work

The diffs look like not quite what you wanted to achieve - http://drupalcode.org/sandbox/bappa.sarkar/2010602.git/commitdiff/9ba851... ?

bappa.sarkar’s picture

Status:Needs work» Needs review

Hi greggles,

Actually i had added a wrong file during my commit. Check the previous commit http://drupalcode.org/sandbox/bappa.sarkar/2010602.git/commitdiff/d6c288...

Thanks,
Bappa

kscheirer’s picture

Status:Needs review» Reviewed & tested by the community
  • I don't think you want phpconfig_has_access(). Instead of looking up all the user's roles and then seeing if your permission is part of them, can you use user_access('administer phpconfig') ? That does the same thing. Then you also don't need to check for user 1, since user_access() always returns TRUE for uid 1.
  • Instead of the the access integers 2, 4, 6, can you use the constants? It's a little easier to read INI_PERDIR, INI_SYSTEM, [I'm not sure what 6 maps to].
  • What is phpconfig_test()? That's not a drupal hook, can you update the function doc?
  • Can you add a warning to the project page? The administer phpconfig permission is extremely powerful and should only be given to trusted users. I like that you check to make sure the site isn't broken, but it would still be too easy to accidentally/maliciously make the site perform poorly or in unexpected ways.
  • Security looks much improved, thanks!

I'd like to see the extra warnings go in, but I don't think that's a blocking issue.

----
Top Shelf Modules - Crafted, Curated, Contributed.

bappa.sarkar’s picture

Hi kscheirer,

I can't able to use user_access in boot hook as the function is not yet present in boot level.

All module file got included after boot hook. That's why I used custom access check.

Thanks,
Bappa

bappa.sarkar’s picture

I have Added the warning in the project page.

bappa.sarkar’s picture

Added review bonus tag.

bappa.sarkar’s picture

Issue summary:View changes

Adding Review links done for other's projects

klausi’s picture

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I also removed the purely automated review from the issue summary.

klausi’s picture

Issue summary:View changes

removed automated review.

bappa.sarkar’s picture

Adding review bonus tag.

bappa.sarkar’s picture

Issue summary:View changes

Adding reviews

bappa.sarkar’s picture

Issue summary:View changes

Adding review link

bappa.sarkar’s picture

Can anyone please tell me is there anything pending to get RTBC? Please help.

Thanks,
Bappa

kscheirer’s picture

No further action is required, but the best thing you can do is get a Review Bonus by reviewing other applications. That will get you to the top of the list of projects to get reviewed (and hopefully approved). Only manual reviews count, just using http://ventral.org is not enough.

urwen’s picture

Bappa, help me reviewing my module and I will give you the favour back: https://drupal.org/node/2083831

bappa.sarkar’s picture

Issue summary:View changes

Adding review bonus

bappa.sarkar’s picture

Issue summary:View changes

adding more manual reviews

urwen’s picture

in line 67, you are using include_once with the path to the module. Why not using module_load_include, which is standard and will detect your module path automatically?

module_load_include('inc', 'phpconfig', 'includes/phpconfig.inc');

other than that, the module looks good and the functionality is quite useful, nice approach to a common issue.

bappa.sarkar’s picture

Which function you are talking about? I only used include once in function phpconfig_boot(). And I can't use module_load_include() as in boot level we don't have this function.

BTW thanks for the review :)

klausi’s picture

Status:Reviewed & tested by the community» Fixed

manual review:

  1. phpconfig_has_access(): why do you need that function? Just include the relevant files and use user_access()?
  2. phpconfig_test(): doc block is wrong, this is not a hook? See https://drupal.org/node/1354#menu-callback
  3. admin/settings/phpconfig is pretty empty and could use a hook_help().

But otherwise looks good to me, so ...

Thanks for your contribution, bappa.sarkar!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.

urwen’s picture

you were right bappa, sorry for that... and congrats :-).

bappa.sarkar’s picture

I would like to give Thanks to all who gave their valuable time to review the module. Thanks once again :)

Cheers,
Bappa

geberele’s picture

Congratulations Bappa!! ;)

Ayesh’s picture

Congratulations, dear vetted member!

bappa.sarkar’s picture

Thanks Ayesh, gebrele, urwen. You guys are awesome :-)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Adding more review