[D6] XML display

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• README.txt is missing, see the guidelines for in-project documentation.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• @file doc block is missing in the install file, see http://drupal.org/node/1354#files .
• ./xml_display.install: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  
  function xml_display_install() {
  
  

• ./xml_display.module: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  65-**/
  
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./xml_display.info:                                       ASCII text, with CRLF line terminators
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  xml_display.module:84:    } elseif ( is_null($xml_fetch['xml']) OR (time() - $xml_fetch['elapsed_time']) > $xml_fetch['last_update'] ) {
  
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  xml_display.admin.inc:2:// $Id$
  
  xml_display.info:1:; $Id$
  
  xml_display.install:2:// $Id$
  
  xml_display.module:2:// $Id$
  
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

@karljohann has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256#abandonedtwoweekscontact

Hi karljohann,

Manual Review:
1) Remove unnecessary comments from xml_display_uninstall() function.

Please take a moment to make your project page follow tips for a great project page.

please get a review bonus first. Then try to fix issues raised by automated review tools and set this back to "needs review".

Automated Review:
No issues found here.

Manual Review:

• Why have you used the function trigger_error on line 99? You can better set a status message or something else to inform the user and create a watchdog log entry.
• In your admin form, you don't have to add the submit and validation functions if they have the same name as the original form with _submit and _validate added.

Automated Review:
Your "xmlt_perm" function should be call xml_display_perm.

Manual Review:
Also on your "xmlt_perm" function, the comment on that function is a bit of a mess (you have extra *s).

In your "xml_display_cache_xml" function:
if (is_null($block_content[$i]['xml']) OR (time() - $block_content[$i]['elapsed_time']) > $block_content[$i]['last_update']) {
$block_content is never set.
$i is never set.

Hi karljohann,

I think instead of using xml_display_edit_form_validate() for checking the xsl field you can use the #required attribute on that field.

You can read about how to document your code here: http://drupal.org/node/1354, particularly for the form-related functions in xml_display.admin.inc.

I don't think xml_display_admin_delete_submit() actually implements hook_delete, so the function description should be changed.

Can you suggest one or more urls that contain xml we can use to test the module?

Thanks

Hi karljohann,

In xml_display_schema(), the key 'primary key' and its value need to be lower case.

When I try to view the block, I get the error "Class 'XsltProcessor' not found in /sites/all/modules/contrib/xml_display/xml_display.module". You should indicate what needs to be done to make this class available, like download some library or install a php function.

Reviewing

Installed this on simplytest.me.
When to /admin/build/block to add the block and got the following error:

Invalid argument supplied for foreach() in /home/s31d63081dbbb370/www/sites/default/modules/1307164/xml_display.module on line 76.

You have no default definition of $block_content, so if no database records are found it fails.

The block does not appear in list to add.

Sorry, karljohann, you can't mark your own application as reviewed. Hopefully someone will be able to do another review soon, and you can help reduce the wait by reviewing other applications in the queue.

Looks pretty good, RTBC from me.

One minor point, instead of $block['subject'] = t("xml_display $delta is Empty"); the proper way to use t() is: $block['subject'] = t('xml_display %delta is Empty', array('%delta' => $delta);.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Thanks klausi and anwar_max for reviews, and _wdm_, robinvdvleuten, xenophyle, and jcaustin98 for reviews remarkable for their directly applicable recommendations (and good work karljohann for directly applying them!)

karljohann, correct your t() code per kscheirer's suggestion (why does the block subject have the module machine name in it incidentally?) and i'll make you a full project maintainer straight away.

Looks like karljohann committed that fix here: http://drupalcode.org/sandbox/karljohann/1307164.git/commitdiff/26edbb58....
I see you have the XSL requirement on the project page, could you implement a hook_requirements() as well?

I agree with mlncn, this module is ready. Thanks for being patient and working with us karljohann.

Thanks for your contribution, karljohann!

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.

----
Top Shelf Modules - Crafted, Curated, Contributed.