[D6] phpconfig

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.

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!

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)

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 :)

As per the above comments I am setting it to 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.

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

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.
   

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.

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.

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'.

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.

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

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 )

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.

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!

Fixing crosspost from #24.

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!

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

• 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.

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.

removed automated review.

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.

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

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.

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.

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

Congratulations Bappa!! ;)

Congratulations, dear vetted member!