[D7] Entity list display

Correct git-line:
git clone http://git.drupal.org/sandbox/mikkeX/1335106.git entity_list_display

Also, run Coder module, it found few coding standards issues.

Also, correct your docblocks. This is the correct form:

/**
 * Summary here; one sentence on one line (should not, but can exceed 80 chars).
 *
 * A more detailed description goes here.
 *
 * A blank line forms a paragraph. There should be no trailing white-space
 * anywhere.
 *
 * @param $first
 *   "@param" is a Doxygen directive to describe a function parameter. Like some
 *   other directives, it takes a term/summary on the same line and a
 *   description (this text) indented by 2 spaces on the next line. All
 *   descriptive text should wrap at 80 chars, without going over.
 *   Newlines are NOT supported within directives; if a newline would be before
 *   this text, it would be appended to the general description above.
 * @param $second
 *   There should be no newline between multiple directives (of the same type).
 * @param $third
 *   (optional) TRUE if Third should be done. Defaults to FALSE.
 *   Only optional parameters are explicitly stated as such. The description
 *   should clarify the default value if omitted.
 *
 * @return
 *   "@return" is a different Doxygen directive to describe the return value of
 *   a function, if there is any.
 */
function mymodule_foo($first, $second, $third = FALSE) {
}

Document ALL functions like this. You now have some totally undocumented functions and the rest are incomplete.
Please read this page:
http://drupal.org/node/1354

If i do git clone, i do not receive your changes. Apparently you are working in a wrong branch.
Basically, the master branch is not used in drupal projects and should be empty and you should name your working branch as 7.x-1.x
Read this:
http://drupal.org/node/1015226

Also, there should be some documentation in the README.txt. I think if you copy the text from your project description, it's fine at this stage.
You can improve it later.

corrected git with branch

EDIT:
Post contained personal information that needed to be removed. The issue itself was apparently fixed.

Also, remove

files[] = entity_list_display.module

from your .info file. It is only used for files that declare classes or interfaces.

The branch where you work daily can be dev or anything you want, but the actual dev branch has to be 7.x-1.x and that's the branch we would like to review here.

Ok the "dev" branch looks better(except for the branch name), but you have added some stuff there which need to be removed:

LICENSE.txt (file)
version = "7.x-1.x-dev" (line in the info file)

Those are added automatically by drupal.org in the packaging process.

Ok, you added the correct branch, good. I'll continue the review tomorrow, it's 1:40 already here, got to get some sleep ;).

I'll change the status, if someone wants to take over.

edit: Most likely this is already RTBC, but i'll have to actually install the module first to see.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 7.x-1.x-dev branch:

• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  plugins/content_types/entity_list_display_pane.inc:75:  //available_view_modes is the array to contain the avaible view modes, for now this is not content type aware, but should be
  plugins/content_types/entity_list_display_pane.inc:77:  //entity_get_info feteches the info of avaible view modes
  plugins/content_types/entity_list_display_pane.inc:82:  //get the machine names of the view modes
  plugins/content_types/entity_list_display_pane.inc:91:  //output the form
  plugins/content_types/entity_list_display_pane.inc:95:    //states will propably be used later on when this form is more dynamic
  

• ./plugins/content_types/entity_list_display_pane.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    //available_view_modes is the array to contain the avaible view modes, for now this is not content type aware, but should be
    // combine the machine view mode name with the label, this could sure be done much better?
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./entity_list_display.info ./plugins/content_types/entity_list_display_pane.inc ./README.txt ./entity_list_display.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:

• There are still files in your master branch. Also your major branch naming is not yet correct. Please refer to http://drupal.org/node/1127732 and related docs to empty the master and correctly name the major branch.

HTH, dave

Hi,

remember to switch the status back to "needs review" once you think it's time.

-dave

Hi, some few more automated review results:

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ..._temp/test_candidate/plugins/content_types/entity_list_display_pane.inc
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   106 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 0
   139 | ERROR | Calling class constructors must always include parentheses
  --------------------------------------------------------------------------------
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./entity_list_display.info ./plugins/content_types/entity_list_display_pane.inc ./README.txt ./entity_list_display.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:
There are some branches you should remove: "taxonomies" and "7.x-1.x-dev".

Still some little formatting issues:
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ..._temp/test_candidate/plugins/content_types/entity_list_display_pane.inc
  --------------------------------------------------------------------------------
  FOUND 11 ERROR(S) AFFECTING 7 LINE(S)
  --------------------------------------------------------------------------------
   11 | ERROR | Inline comments must start with a capital letter
   11 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
   75 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
   77 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
   82 | ERROR | Inline comments must start with a capital letter
   82 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
   84 | ERROR | Inline comments must start with a capital letter
   84 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
   89 | ERROR | Inline comments must start with a capital letter
   92 | ERROR | Inline comments must start with a capital letter
   92 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

I tested it and it worked for me.

But there already is a module called entitylist (http://drupal.org/project/entitylist)
You can't use exactly the same module name for yours.
Rather then *kind of* duplicating you could also think about helping to make the entitylist module easier to setup (and then come back to the queue with more basic module), did you already tried to get in contact with the maintainers?

Your module is more complicated than most others in the queue, so please be patient.
As soon as all formatting issues are fixed, I think your module will be reviewed manually as soon as somebody has time to do it.

btw, I had similar problem here. my first module in the queue stuck for three months after I decided to create a simpler one that could make it (http://drupal.org/project/faqfield)

Oh, I'm sorry somehow I didn't recognize the _list part. .. it was late yesterday ;-)

You know what? Code standarts got updated, your formatting throws errors again: http://ventral.org/pareview/httpgitdrupalorgsandboxmikkex1335106git

BUT these changes came by drupal 8 so I don't know if you must follow them yet.

Sorry it seems like you got two application issues opened (http://drupal.org/node/1363104)
Unfortunatelly you can only hold one of these open, please close one of them!

(I'd suggest you to close the more recent one)

it is best practice to review modules with the oldest 'last change' status first, if you always post into this issue it will maybe never get reviewed ;-)

sorry for the latency in the queue, the holidays take lots of time!

It looks to me like you have many branches and there should only be 7.x.1.x with all the code and then the master branch with nothing but a readme.

So you would have to delete the 7.x-1.x-dev branch, and I'm also seeing these branches:

2 weeks ago taxonomies
6 weeks ago postvar
6 weeks ago hard_coded_field_name
6 weeks ago hard_coded_field_name_with_context

I'm not sure what they are but probably they need deleting, or merge them first into 7.x.1.x

need to flip it to needs work for you to clean up the git tree as I say above.

sorry you are right actually: http://drupal.org/node/1015226

You are free to name your branches whatever you like for your day-to-day work.

awesome-new-feature, experimental-redesign, performance-bugfix are all acceptable branch names.

Git's default master branch should not be used and no downloadable releases can be tied to that branch. Use 6.x-1.x, 7.x-1.x or 8.x-1.x as your main development branch (see below).

First a minor point: in your readme you have a typo: "To insert a list of nodes on a panechoose Node entity list."

Now the part where I'm missing something perhaps, or maybe it's a reason to sharpen the readme so others don't loose time trying to figure this out, as I have.

When I create content for a panel I see your new option "entity list" which is very cool. But when I go and choose "node entity list" I get a box with just white / nothing in it (see attached screenshot). Now maybe I didn't configure the content type? Anyhow

1.) instead of just the white blank screen maybe there should be a drupal_set_message sort of alert telling the user to configure xy & z
2.) It's actually not clear to me if this is a bug or if I didn't configure something from the readme. Where do I choose what content types are to be used for this? Is that in fact why this isn't working for me? I could dig deeper, but I think highly of good documentation and want to kick this back as needs work for you to this this up. I promise to continue another review soon after :)

I'm waiting in the que as well with my Droogle project and know how you feel waiting for another review. I hope to be of some help here. Klausi has encouraged us to go for a review bonus to get our projects reviewed more quickly and I will promote his post here for you: http://drupal.org/node/1410826

The version of ctools is version = "7.x-1.0-rc1"

I do believe I followed your directions correctly but I get nothing but that white box that shows in the screenshot.

Here is the error message i'm getting in watchdog:

location: http://drupal7:8888/panels/ajax/editor/add-pane/panel_context%3Apage-panel1%3Apage_panel1_panel_context/left/entity_list_display_pane/entity_list_display_pane

referrer: http://drupal7:8888/admin/structure/pages/nojs/operation/page-panel1/handlers/page_panel1_panel_context/content?render=overlay

message: Warning: Parameter 1 to entity_list_display_pane_content_type_edit_form() expected to be a reference, value given in drupal_retrieve_form() (line 785 of /Users/barnettech/Sites/drupal7/drupal7/includes/form.inc).

dunno if it's just a warning and unrelated to the problem.

I looked at the code a bit. I notice on lines 14,15, and 16 of entity_list_display_pane.inc you have this:

'category' => array(t('Entity list display'), -3),
  // Add to the category Entity list.
'category' => array(t('Entity list'), -3),

you define category twice incorrectly? I don't see that this would stop it from working?, but just wanted to note it. Are you using the same exact code in your browser that you've posted to drupal.org ? I know I have to be careful that my module is in a different git repository and then I copy over files to drupal.org's git repository and I have to make sure all the little things I fix for drupal.org doesn't cause something to break inadvertently.

I run: PHP 5.3.6 with Suhosin-Patch (cli) (built: Sep 8 2011 19:34:00)

turning off overlay did not fix it.

ctools examples work fine. I just test out a few: like this one: http://drupal7:8888/ctools_ajax_sample/nojs/animal/sheep and I have:

You have a Wensleydale sheep named "marty".

sorry still no luck. I also tried downloading views, enabling everything I could find under views and chaos tools, I was thinking maybe you have a dependency you don't realize you have. Perhaps you call a function that is actually part of a contrib module? Try installing your module on a base drupal7 install and see if it works for you. I also tried it in firefox to see if it was a browser problem. It didn't work in chrome or firefox. See if there is a dependent module you didn't realize should be declared in .info as a dependency?

it is never calling this form function: entity_list_display_pane_content_type_edit_form

I put in a watchdog statement and verified it never gets called.

what does this mean in your readme:

Your content type must have the view
mode you choose for it activated, else it will just use the default.

Ok found an article on it: http://mearra.com/blogs/juha-niemi/drupal-7-custom-node-view-modes

another typo: you wrote: "Sends the variables to to function that render the ouput." and you see you wrote the word "to" twice. Trying to find the bug, and just ran into this minor thing to report.

I just read this tutorial and see some differences in how you write your module: http://www.batayneh.me/post/create-ctools-plugin-panels-drupal-7

you don't have a folder with your plugin name (in the tutorial they name their plugin "activities") and then their render callback and render form function names use the convention of

'render callback' => 'yourmodule_activities_render',
  'edit form' => 'yourmodule_activities_form'

yours is:

'edit form' => 'entity_list_display_pane_content_type_edit_form',
  'render callback' => 'entity_list_display_pane_content_type_render',

so should there be a folder that the .inc file goes in called "pane_content_type" ?

I checked your module list I have all of those installed.

I tried creating that folder, still no luck. neither entity_list_display_pane_content_type_edit_form or entity_list_display_pane_content_type_render are getting called.

I just ran through this tutorial and I come up with the same problem. http://www.nicklewis.org/drupal/tutorials/ctools-custom-content-types I'm flipping this to needs review until we see if its something about my installation.

Ok I got obsessed with making this work. I just installed it on an ubuntu box (previously I tested it on my mac) and in this drupal install, I come up with the same problem. I'll leave it as needs review so you can see if another reviewer has the same problem. Run drush up and update all your modules and core, do you have the problem? I checked, and all my stuff is up to date.

CTools example no context content type works in panels for me, just tried it.

I have verified you code does not work on my system by making my own plugin. The attached file which basically copies the ctools example works fine, so you'll need to identify the bug. If I have time I will help. My file which does work is attached. Also notice I changed the filename a bit to match the convention that ctools used in the example. I'm not sure if that is part of the problem.

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

  remotes/origin/7.x-1.x-dev

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review of the 7.x-1.x branch:

• the project page should list differences to other existing modules so that users can make better decisions which module to use.
• entity_list_display_entity_info_alter(): this is a hook implementation and should be documented as such, see http://drupal.org/node/1354#hookimpl Also the doc block contents on this function is just wrong.
• entity_list_display_ctools_plugin_directory(): this is also a hook implementation, no? Also the doc block is a bit weird ... a parameter does not set anything?
• "$block->title = t('List of') . ' ' . $conf['content_type'];": do not concatenate translatable strings like this, use placeholders in t() instead.
• entity_list_display_pane_content_type_edit_form(): "$entity_info = entity_get_info();" this line appears twice. Also if you are only interested in info about nodes you should pass the $entity_type parameter to it.
• Why is your module called entity_list_display if it does only work with nodes? Either make it work with all kind of entities or rename it to node_list_display.
• entity_list_display_entity_info_alter(): why do you even need that hook? What do the two view modes mean? You define them here but never do anything with them in your code?

Because "7.x-1.x-dev" can easily cause confusion as it is so similar to the correct name "7.x-1.x".
Regarding comment #11: I think bfr meant the name "dev" not "7.x-1.x-dev".

Just saw this project demonstrated. Cool thing – an innovative (and useful) way of extending CTools.

My two cents: I have had a quick look at the code, and learned good things from it. Haven't looked close enough to say if things are "perfect", but as far as I can tell they are definitely good enough.

After spending some more time with the module and its code, I'd like to add two things:

1) The function function entity_list_display_pane_content_type_edit_form() should not have ampersands ("&") with its parameters.
2) In _entity_list_display_output(), there are some lines (conditionally) adding a query against field_tags. These are not used by the module in its current state (but were used in the demonstration I saw today).

As said before, though, my humble opinion is that this code is well above the threshold of acceptable for a full project. MiSc demonstrates that he knows how to write good Drupal code – both in the style of writing and in the functionality he is implementing.

manual review:

• entity_list_display_pane_admin_title(): the if() ":" and "endif;" alternative syntax should only be used in template files. Same for other if()s and foreach()es in your code. It gets really confusing in _entity_list_display_output(), where you got some indentation errors in the nested ifs/foreaches.

Otherwise this looks RTBC to me. Removing review bonus tag, if there are still changes required you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

MiSc: Congrats!