Features direct save

First impression:

- readme should be README.txt
- documentation for all functions please
- deadwood code (just a the watchdog)
- there are two .info files?

I like the idea :)

I like the idea too, but this needs a code review from someone who understands security and the implications of letting a module write module code...

Well it requires you to give access to sites/all/modules and that's wrong. Can't you use the new Drupal 7 feature with authenticate.php?

@meba, We're very much aware that it's wrong to write to the modules directory on production. However, this module is intended to facilitate development with Drupal, not to be used on production servers.
Try it out - you'll see it does good things to development with features :)

edit: we're currently using this module with Drupal 6, so using D7 is not an option for now.

Point taken.

I'd like to see some loud warnings from this module that it's not meant for production servers.

Alternatively, look at how the update module in D7 handles things.

Quick eyeball review:

> ; $Id:
> // $Id$

Obsolete.

- Readme and info file texts: please get a native English speaker to go over these :)

> name = "Features download"

Name in code disagrees with 'Features direct save'.

> function features_download_form_alter

Use the hook_form_FORM_ID_alter hook for clearer code?

> '#description' => t('the root folder will be sites/all/modules/'),

Needs capital and full stop.

> // Assemble the combined component list

All comments should be a sentence with a full stop.

> $dest = $form_state['values']['features_download_dest'];

Use full words for variable names: $destination.

  if (arg(3) == 'create') {
    $directory .=  $module_name;
  }

I think the destination directory form element is confusing -- if I read the above correctly, when creating rather than updating a feature, the form element you input is the root; when updating it's obviously deduced from where the code is already. Though in that case -- why ask the user for the directory anyway? What would happen if the user changed it -- would the former location be deleted? Doesn't look like it in the code.

Also, what if the updated feature has fewer files than the old version -- eg what if I remove all the Views -- will the code update remove dead files?

I think this module should depend on http://drupal.org/project/bad_judgement Bad Judgement module. I am afraid that people might not recognize how the module is supposed to be used. Bad Judgement will make it clear.

Subscribe.

BTW, http://drupal.org/project/apps also asks users to open write permissions to their sites directory, and to do so on production (!)
See https://skitch.com/z.stolar/r7x2n/ideation-details-openpublic

subscribe

Maybe what we could do is address the (rightful) concerns by UX.
If we put a big red warning in the interface screens explaining that this is only for development use.

If we put a big red warning in the interface screens explaining that this is only for development use.

In my opinion you should add the warning to the project page and README and maybe the admin/reports/status, but every page is overkill.

As Lior points out, this is something where the project doesn't have a consistent policy. I started a discussion at http://groups.drupal.org/node/105004 but it didn't get much traction yet. Maybe you could comment there?

Hi Greg,

In the meantime, can we release the module? :)
I mean - if an app at the importance of OpenPublic is allowed to write files in production (!!) then certainly there's no need to limit devs from doing so in development environments...

I say - set it free! This has the potential of becoming a module that can actually help people developing for Drupal :)

BTW, I also think that the module page should make it clear that it is meant for development only, but that should be it as far as a requirement can go. We'll also add a note in the status page, but each page is not only an overkill, is also a nuisance for development, a.k.a. counter-productiveness.

I see now that the warnings have been added (or had been added beforehand).

Given that the code review issues raised by joachim have been fixed and that it has warnings marking fixed and granting the git vetted user to perelman.yuval.

Thanks for the reviews, everyone, and work to add useful modules, perelman.yuval.