The Lister module provides a new field type for Drupal 7, called "Listed content". It aims to provide an extendable method of allowing end users to create lists of content as they would any other field.

The module can be extended and other modules could be created to provide any number of methods for generating lists of content. However, the repository also contains four submodules that cover the implementations that we required during initial development: EntityFieldQuery based lists, Views based lists, RSS feed based lists and menu children lists. Without at least one of these 4 modules the core module will be featureless. I imagine the lister_efq and lister_views modules would be the most widely applicable to other uses. Lister RSS requires a Symfony component and provides a composer.json file for installation (see the Readme for more details).

Sandbox: https://www.drupal.org/sandbox/justanothermark/2491203
Git command: git clone --branch 7.x-1.x https://git.drupal.org/sandbox/justanothermark/2491203.git lister
Readme link: http://cgit.drupalcode.org/sandbox-justanothermark-2491203/tree/README.md

PAReview link: http://pareview.sh/pareview/httpsgitdrupalorgsandboxjustanothermark24912...
There are currently 3 warnings as part of the PAReview report. These are all about concatenating translated strings in the format '#empty_option' => ' - ' . t('None') . ' - ', which I believe are valid exceptions as the -s should not be part of the translated string. I am happy to change this if opinions differ.

Comments

justanothermark created an issue. See original summary.

PA robot’s picture

PA robot commented

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.

a.hover’s picture

a.hover commented

Hi Mark,

The module on the whole looks good and is almost there.

A really tiny niggle - there is a redundant variable in getListDisplayOptions() (/modules/lister_efq/ListerEfqPlugin.inc, line 356)

  protected function getListDisplayOptions() {
    $view_modes = array();

    $view_modes = array(
      'default' => t('Default'),
      'grid' => t('Grid'),
    );

More major:

  • Adding a lister field inside a Paragraphs field. When I choose the listing type (e.g. Configurable content type), I don't get the subsequent fields appearing for it. Bit of a minority use case, so not sure if it needs fixing.
  • On a normal field (i.e. not inside a Paragraphs field) I get an error when I try to add a Preset Content View:
        Notice: Undefined index: data in ListerViewPlugin->formValidate() (line 81 of /var/www/www/sites/all/modules/contrib/lister/modules/lister_view/ListerViewPlugin.inc).
    Warning: Invalid argument supplied for foreach() in ListerViewPlugin->formValidate() (line 81 of /var/www/www/sites/all/modules/contrib/lister/modules/lister_view/ListerViewPlugin.inc).

    Then when I select a view I get the following error:

        Notice: Undefined index: options in ListerViewPlugin->formValidate() (line 81 of /var/www/www/sites/all/modules/contrib/lister/modules/lister_view/ListerViewPlugin.inc).
    Warning: Invalid argument supplied for foreach() in ListerViewPlugin->formValidate() (line 81 of /var/www/www/sites/all/modules/contrib/lister/modules/lister_view/ListerViewPlugin.inc).
    Notice: Undefined index: arguments in ListerViewPlugin->form() (line 58 of /var/www/www/sites/all/modules/contrib/lister/modules/lister_view/ListerViewPlugin.inc).
    Warning: Invalid argument supplied for foreach() in ListerViewPlugin->form() (line 60 of /var/www/www/sites/all/modules/contrib/lister/modules/lister_view/ListerViewPlugin.inc).

Will keep on reviewing so watch this space.

a.hover’s picture

a.hover commented
Status: Needs review » Needs work
justanothermark’s picture

justanothermark commented
Status: Needs work » Needs review

I have fixed the undefined index and invalid argument warnings.

Re: the Paragraphs bug. I suspect this might be related to https://www.drupal.org/node/2262929 and/or https://www.drupal.org/node/2564327 issues on Paragraphs. I have created an issue to look into this further but I'm not sure an incompatibility with a relatively small contrib module should block the review?

davidsonjames’s picture

davidsonjames commented

Mostly looks great. A couple of issues I came across:

The composer.json file in the lister_rss submodule requires: symfony/css-selector": "~2.8.4" based on line 75 of modules/lister_rss/ListerRssPlugin.inc.

Also the "lister_rss_theme" theme needs to specify "'template' => 'templates/lister-rss-item'," rather than "'template' => 'lister-rss-item'," otherwise it never finds the template file.

The final issue was around the views submodule. Firstly none of my views appeared for me to select until I changed

"if (isset($instance['widget']['settings']['lister_view']['views'])) {"

to

"if (!empty($instance['widget']['settings']['lister_view']['views'])) {"

Once I'd made that change I could select a view, but now when I save the node and try and view it I get:

Fatal error: Unsupported operand types in /var/www/drupal7/modules/node/node.module on line 1415 but I'm not sure why.

Edit:

The issue of the unsupported operand type only appears to happen when you call a view in which the content in which you're viewing is a result of that view.

msmithcti’s picture

msmithcti commented

Overall, the code looks good. Couple of minor points:

It might be worth checking if the plugin class exists before instantiating it (earlier the better probably) and handling that instance more gracefully:

<?php
// Create a new Plugin object from the selected type.
$plugin = isset($plugins[$current_type]['class']) ? new $plugins[$current_type]['class']() : array();
?>

I would suggest providing an interface (or making ListerPlugin.inc abstract) and checking that the plugin class implements the expected interface.

Other than that, it looks great!

msmithcti’s picture

msmithcti commented
Status: Needs review » Needs work
justanothermark’s picture

justanothermark commented

@davidsonjames, I've added CSS Selector to the composer.json file and fixed the hook_theme() implementation.

As you added, trying to render a node as part of rendering the same node causes problems. This is a wider issue with node_view() doing:

// We don't need duplicate rendering info in node->content.
unset($node->content);

This also happens with Display Suite and other modules that can result in nested content. See, this core bug. I've created an issue and added a warning to the project page but I believe this is more likely to happen when testing than for practical uses of the module (as the view being listed probably shouldn't include the node itself in most cases).

Thanks for the review :)

justanothermark’s picture

justanothermark commented
Status: Needs work » Needs review

@splatio, the base ListerPlugin class is not abstract as it is used to provide a plugin for the 'None' option. However, I have extracted the plugin instantiation into a function that checks whether the class exists and that it is an instance of ListerPlugin as suggested.

kapil.ropalekar’s picture

kapil.ropalekar commented

Hi justanothermark,

My findings for your module are as follows:

lister_view.module

severity: criticalLine 56: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
'#title' => str_replace('_', ' ', ucfirst($argument_display['table'])) . ' : ' . $argument_display['field'],

Thanks !

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs review » Needs work

Hi Mark,

I found that module is very nice. but when i am adding any content with "listed content" field type then i have got a warnings while saving the content.
can you please look into the issue & fixed this.

Warning: array_filter() expects parameter 1 to be array, null given in ListerViewPlugin->render() (line 117 of /Users/yogeshpawar/Sites/drupal-seven-dev/sites/all/modules/lister/modules/lister_view/ListerViewPlugin.inc).
Warning: array_values() expects parameter 1 to be array, null given in ListerViewPlugin->render() (line 119 of /Users/yogeshpawar/Sites/drupal-seven-dev/sites/all/modules/lister/modules/lister_view/ListerViewPlugin.inc).

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

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.

justanothermark’s picture

justanothermark commented
Status: Closed (won't fix) » Needs review

Apologies for the delays.

Re #11: I have added a check_plain() as the #title should be plain text.

Re #12: I couldn't recreate circumstances where $data['options'] would be NULL to give the warnings mentioned. However, I have added an additional line that sets $data['options'] to an empty array if it is NULL or not an array. This should fix the warnings.

justanothermark’s picture

justanothermark commented

Actually, I think I've found the actual cause of the warnings from #12 (or some additional warnings). They happened when changing the type of the lister so that $item['data'] already existed in the plugin form() method. I've modified the code that sets default values in ListerViewPlugin and ListerEfqPlugin to prevent these warnings and ensure that defaults are always set, even when changing existing Lister values.

pol’s picture

pol
Location Brussels
commented
Status: Needs review » Needs work

Hello,

I'm absolutely not against new modules and new developers in the Drupal community, but I have a question regarding your module application.

Beside the fact that one is a field type and the other if a field formatter, could you explain the differences, in terms of features, between your module and Views Field Formatter ?

As far as I understand its purpose, I think it's basically doing the same with a few restrictions. VFF was written especially to fulfill that specific feature, to be able to have any kind of list in a field, using Views and all its flexibility.

Thanks for letting us know.

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

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.