Entity List Field

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

There should be a semicolon on line 54 in your ajax-pager.js file.
The git clone on your description above should be this git clone http://git.drupal.org/sandbox/arnoldbird/1938852.git entity_list_field

Added future plans.

Hey arnoldbird,

1. Pareview had brought up some errors, but I decided to check it before I submitted this and noticed you cleaned them all up, great job!

2. Your Project Page could use some work. You've got some of the good basics (dependencies, an example case) however some additional sections like Features, Similar Projects, and Documentation links would go a long way. A great read on this is at: http://drupal.org/node/997024

3. A Documentation Page would help a lot. This helps new users know how to use the module and can include any other useful information you might have for someone trying Entity List Field. Your README.txt already has most of this information. See #5 on this link: http://drupal.org/node/7765

You do a great job of meeting the Coding Standards. Overall your code is well written, clean, and safe.

Module duplication does not seem to be a problem either, since this is the only module I could find that does this type of functionality without using Views.

Edit: saw the your above post. In response: entlist_field could definitely work. I don't really have much of a better suggestion... I understand your thought on clashing with other names or just being awkward, however it does, often, seem like being verbose in drupal is not a bad thing.

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

Hi arnoldbird,

Thanks a lot for sharing this module and work on fixing previously reported issues above.
I have been following the discussion initially at A field for displaying a list of entities (on GDO) which lead me to this application.

Just tested the module at 15bf4ad and got an error probably coming from an incomplete find/replace for a file path:
See in entity_list_field.module, line 408:
<?php drupal_add_js(drupal_get_path('module', 'entity_list_field') . '/settings-form.js'); ?>
should probably be:
<?php drupal_add_js(drupal_get_path('module', 'entity_list_field') . '/widget-form.js'); ?>

That's all I could find so far from a testing standpoint.

Initially, I was curious to find out what module would really add that Views and related fields modules (EVA, View reference, Views Field View, Viewfield) couldn't do already and it seems indeed that it adds a very flexible (perhaps too flexible in some cases) user interface for admins to build custom lists of items. It would be kind of like a very simplified user interface for views attached to a field.
I can better understand the use case now, believe this could serve to some other developpers out there and wouldn't see any interest in opposing the Similar modules argument I initially had in mind.

However, I would still be curious to know if something like this could potentially be built as an additional layer to existing Views Field modules, mentioned above (for example, provide the parameters to a views field/views to load the corresponding list).

One last validation related advice would be for you to run the coding standards validation (PAReview) again, after you have committed your changes, to make sure you didn't introduce any new coding standards errors when you made the changes. After that, feel free to change the status back to needs review, that will save you one not so useful review from another user who would have simply told you about the coding errors mistakes (more back and forth = more time). This is something very common that happens to all of us (and just for one whitespace at the end of the line... the status is changed back to needs work again).

On top of all that, if you really wanted to do things the best way, I would recommend you copy paste some of the different items I listed/detailed above and make them tickets in your sandbox project tracker. Then, when you commit you will be able to include a ticket number in your log message, as it's being done with most standard contributed modules (Views, Panels, Search API, etc...).
This is not needed for the validation of the module, but think about its future and potential community of developpers that would greatly appreciate going through your documented work if they came to use this module. When the module is approved, the commits will be visible to users along with the issues and all that, which would really get your module to look like a real/serious one.

Obviously there is still more work needed on this module, so I allowed myself to set the status to needs work.

I would be glad to hear what you think about trying to work out a potential extension on top of an existing Views Field module.

Well done and I sincerely hope these comments help you improving this very nice module.
Cheers!

manual review:

1. Only instance variables should use camel case, function parameters or local variables should use underscores. So no false positive from Coder Sniffer. See http://drupal.org/coding-standards#naming
2. You have to use "/**" doxygen style function comments instead of "/*", so there is also no false positive from coder sniffer. See http://drupal.org/node/1354#functions
3. This module looks like it has great overlap with Entity Views Attachments, please try to justify the differences on your project page. http://drupal.org/project/eva
4. entity_list_field_field_schema(): do not duplicate hook documentation in the doc block.
5. entity_list_field_field_schema(): descriptions for the columns would be very useful.
6. entity_list_field_field_schema(): why is numitems a float, sounds like an integer?
7. entity_list_field_field_widget_form(): do not use drupal_add_js() if you have a form at hand anyway, use the #attached property of the elements.
8. "$msg = t($str, $args);": do not pass dynamic variables to t() as that cannot be found by translation extraction tools. Always pass string literals to t() where possible.

to be continued ...

entity_list_field_options_callback(): this is vulnerable to XSS exploits. I can prepare this form:

<html> 
<form action=http://sff15f74b3575343.s3.simplytest.me/entity_list_field/options method=post> 
<input name='el_key' value='b"><script>alert("XSS");</script>' type='hidden'>
<input type=submit>
</form> 
</html>

Then I'll place that on my site and let it auto-submit with Javascript - bang, you have arbitrary javascript executed from your site. You need to sanitize user provided text before printing, please read http://drupal.org/node/28984 again. This is a security blocker.

entity_list_field_pageturner_callback(): why do you write to $_GET? Please add a comment.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

remove outdated content

manual review:

1. entity_list_field_pageturner_callback(): this is vulnerable to unexpected code execution if you just take the class name out of a $_POST parameter. Sure, there must be a class in your Drupal installation that does something unexpected and has a getList() method. You need to make sure that the 'list class' is in a range of values that you know about and that are safe. This is a security blocker. This reminds me a bit of the unserialize exploit: http://heine.familiedeelstra.com/security/unserialize
2. entity_list_field_field_widget_form(): do not use drupal_add_js() if you have a form at hand anyway, use the #attached property of the elements. Or does that not work.

But otherwise this looks pretty ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution, arnoldbird!

I updated your account to let you 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 get 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.