[D7] Form Reference Field

Hi oliverpolden,
The Form Reference Field module looks really promising,
i rapidly just gone through these module and got this small mistake please correct it.
in file form_reference.module on line 185 "*" is missing.

Thanks,
Rahul.

1. See here and follow module documentation guidelines https://www.drupal.org/node/161085
2. The README-file should repeat the synopsis on the project page. See this template https://www.drupal.org/node/2181737
3. Please add the project name to the git clone instruction
   git clone --branch master http://git.drupal.org/sandbox/xalen/2390753.git form_reference

Hi oliverpolden.

Interesting module, thanks for posting. Is it related in any way to the inline_entity_form module?

Just had a quick look through and the following seemed worthy of comment.

- There's a README.md file that is nearly empty, and a README.txt file that looks to be formatted in a MD-compatible way. I think it would be better to have just one of these!

- Please html format the project page -- use h2 & h3 as appropriate, mainly.

- The module would be more user-friendly if there was a hook_help implementation.

An extension opportunity would be to implement hooks for Features integration (and/or others: Views, Rules, et al)

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

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.

> An extension opportunity would be to implement hooks for Features integration (and/or others: Views, Rules, et al)

As this module provides a field type, that should all work automatically surely.

> package = Other

Package should probably be 'Fields', assuming I'm correct in thinking that already exists.

 * @return array
 *   Information about core forms.

This should be documented with actual details.

> foreach (module_implements('form_reference') as $module) {

If you invent a hook, then you must document it in an api.php file.

> $forms = array_merge($forms, $module_forms);

Use module_invoke_all() here, it takes care of the merging.

> function form_reference_core_forms() {

Rather than do this, which then requires special handling in form_reference_all_forms(), you should use the pattern of implementing your hook on behalf of those core modules (user, contact, search).

      $path = dirname(__FILE__) . '/README.md';
      if (file_exists($path)) {

It's your file, you should know that it exists!

> '#description' => t('Manually enter forms that can be referenced. One per line.'),

This should say how you specify a form. Is it with the function name of the form builder, for instance?

        if (strpos($item['form_reference'], 'node:') !== FALSE) {
          module_load_include('inc', 'node', 'node.pages');
          $content_type = str_replace('node:', '', $item['form_reference']);
          $element[$delta] = node_add($content_type);

I would move all this to the hook. Other modules will want to specify an include file to load for their form (a lot of forms live in a MODULE.pages.inc). Special casing nodes here doesn't seem right. Maybe allow your hook to specify a wrapper callback?

Hi.

Works ok but some issues still remain.

1/ Please move your code from master repo to version specific branch. Check this for more - http://drupal.org/node/1127732
2/ When you don't select Content Type or Core modules for a field the option groups still appear without any options. Would be better if they weren't there.
3/ If you select a Content Type form, when it renders it overrides the title of the node
4/ Since you're adding core forms into there why not add taxonomy and some others as well?

Hi Oliver, thanks for the update. A few comments:

- the guidelines relating to git use state that there must not be a master branch; there is still one. The instructions for deleting master branches are here. http://drupal.org/node/1127732

[the development / head branch for a drupal.org project will be called something like 7.x-1.x or 6.x-2.x or similar; using terms like 'master' and 'head' are too vague]

- The project page is looking much better, but please don't SHOUT the titles; use "Title Case In Titles" or "Sentence case in titles".

- On the project page, (and in places such as the readme) it would be helpful to the reader to spell out the full name of the COOL module at least the first time, given that the shortname isn't very explanatory :-)

- This statement on the project page does concern me somewhat: "This module assumes that only appropriate users have permission to configure a form reference field and they should be vigilant about the forms they allow users to select e.g. search vs permissions form."

I say this because it sounds as if the module is relying on users doing the right thing, and that bad things will happen if they don't. If that is the case, then I think the module should be changed before it's released. If it is not the case, then the quoted text isn't conveying the intended meaning properly.

So: I would suggest that you either change the module so that this assumption does not need to be made (i.e. the module always does the right thing) or explain in more detail here exactly what this meant and what the consequences of not being vigilant are.

I've had another look at the code itself and it looks well set out. Neither pareview nor I spotted any issues of significance (pareview wanted a line indented differently). It's good to see you've included the .api.php file. Is it intentional that the @addtogroup is not matched with closing "@}" ?

One concern: I don't think you should leave the .gitignore file in the repo. While I don't think it will do any harm, it could cause other devs pain if they download the module to another git project.

Hi Oliver,

Please also change the git clone branch in the instructions to git clone --branch 7.x-1.x http://git.drupal.org/sandbox/xalen/2390753.git form_reference_field as it appeares here - https://www.drupal.org/project/2390753/git-instructions. Once you fix issues rivimey listed, i will resume my review.

Hi Oliver,

The project page and git files are looking much better, though I still want the 'Notes' section of the description to be expanded to include how to be 'vigilant' and what risk is presented if you are not. Apart from that, the rest seems ok to me: I'll let @rachit_gupta have a look too though.

This seems like a useful module, but it needs better documentation. It doesn't handle all kinds of content creation forms.

The git command in the project page is:
git clone --branch 7.x-1.x xalen@git.drupal.org:sandbox/xalen/2390753.git form_reference

This doesn't work. It should be:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/xalen/2390753.git form_reference

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.

The list of recommended modules should probably include Field Permissions. This could be used to restrict display of a content creation form to only the roles which could create that content.

The description could do a better job of telling a potential user what the module does. It should state that the form is shown when the entity (node?) is displayed. The form is not available when the content is edited.

Does the entity created from the form retain any connection to the entity containing form reference? If so, how are they linked?

Remember that modules are often grabbed by people who know little more about PHP than how to spell it. Using "PSR-4" in the description is interesting to people who care about innards of implementation, but can scare away others who might find the module's capability useful.

Similarly, the description/README file should include explicit details about how to prevent security holes, including use of Field Permissions.

The hook_form_reference() should be documented. Under TROUBLESHOOTING, the user is told to "See form_reference_form_reference()" but the code doesn't provide any information.

It would be useful to know what restrictions are associated with a form created by a custom module which may contain such a hook.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[No: List of security issues identified.]
The code itself appears to be secure, but there should be a warnings in the description and configuration details in the README file to help anyone using the module keep it that way. This text should be suitable for use by a beginner.

Coding style & Drupal API usage

The code looks good, except that hook_form_reference() isn't explained.

The form I tested with includes an Image field with a Media browser widget. Instead of showing the "Browse" button I would expect to see, the embedded form includes a link. When I clicked the link, nothing happened.

Limitations on field types are okay, especially in an initial release, but they should be described.

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.