adlibapi

Hello, jurcello. You need to create a sandbox project, and provide a link here.

Hi Jur,

Here are a few notes after a quick scan of this module:

• Aren't the library files that should go into sites/all/libraries missing in the git sandbox? I get "Failed opening required 'sites/all/libraries/adlib/AdlibConnector.php" and didn't find any folder in the git repo that should be going there...
• Linked with that, it might be a good idea to add extra checks for dependencies, so the whole Drupal installation isn't brought to a halt when the library is missing
• You should remove the obsolete // $id$ and ; $Id$ CVS tags from your .info & .module files
• package = Triquanta should be more descriptive, e.g. package = Adlib, or be removed at all (so modules can go into "Other")
• You should run the project through Coder module to check for coding standards
• In adlibapi_feeds folder, there is a test.info file which can be removed

When I find some more time I might review this module in greater depth. Looking forward to seeing it released!

Sven

Updating priority according to the new project application priority guidelines. The application's priority should be set back to normal once a reviewer responds to your application and the application review process has continued.

I am really interested in this module. Where can I donwload it for testing?

capono:

Using Git:

Click the sandbox link (http://drupal.org/sandbox/jurcello/1072526), and then click 'version control' for instructions.

Without Git:

Click the sandbox link (http://drupal.org/sandbox/jurcello/1072526), and on the bottom right click 'Repository Viewer'.

Then choose the version you want under the 'Heads' section, click tree, and click snapshot.

Hope this helps!

jthorson: thank you, I found it.

Seems to be a lot of work has been put in this module and it looks good. Unfortunately it is not the way we want to use it. We just started to test the new jQuery connector. So we will not import the data into Drupal.

Edit: typo

I see no commit of your #7 work. Checked out from git.drupal.org but only found the initial commit:

Date:   Mon Mar 7 13:19:04 2011 +0100

    Initial commit

Changing status, so that this doesn't stagnate ...

Jurcello,

Did a look through the 6.x version of your module:

1. Should adlib_image.info list a dependency on cck?
2. Line 126 of adlib_image.module: $dummy = 'dummy'; ... why?
3. Not a showstopper, but in-code documentation could use a general spell-checking/typo-hunt throughout.
4. All dynamic/user-supplied text must be run through a sanitization routine (such as check_plain() or filter_xss()) before being output to the screen
   Example (from adlib_references_functions.inc, line 31): Can you guarantee that $node->title is safe, and not something like "<script>dobadthings()</script>"?
   $message= "Last updated node in current step: ". $node->title ." (nid: ". $node->nid . ").";
   I suspect the above is susceptible to an XSS attack.

jurcello 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

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I performed a quick code review and found several indentation and other coding standard issues. Also noticed that there is an empty test.info file in one of the sub-modules. I strongly recommend you to use the tool below to identify and resolve all coding standard and other major issues before submitting your project for review again.

http://ventral.org/pareview

You need to set the status to "needs review" if you want to get a review. See http://drupal.org/node/532400

coding style issues have secondary priority, common code of any project will probably never follow every standard 100%. but we are HAPPY if your at least trying to do it perfectly! ;)

Your project page is not very detailed / looks a little unstructured, please have a look at the tips for a great project page, you may also use HTML-tags for better structure.

An automated review of your project has found many issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.
Automated review: http://ventral.org/pareview/httpgitdrupalorgsandboxjurcello1072526git

Please read about Coding standards (http://drupal.org/coding-standards) and doxygen standards (http://drupal.org/node/1354)

There are about 100 other applications waiting for review and only a hand full of active reviewers.
We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

This one isn't a deal-breaker, but:

     // Intentional ommit of t() functions in l.
     $adlib_settings_url = l('admin/structure/adlibapi', 'admin/structure/adlibapi');
     $form['database'] = array(
       '#type' => 'select',
       '#title' => t('Adlib database'),
       '#description' => t('Adlib database as configured in: !settings_url', array('!settings_url' => $adlib_settings_url)),
       '#options' => $db_options,
       '#default_value' => $this->config['database'],
     );

This should ideally look like this, since links in the UI should usually have descriptive text, particularly if it's an internal URL:

    $form['database'] = array(
      '#type' => 'select',
      '#title' => t('Adlib database'),
      '#description' => t('Adlib database as configured on <a href="@settings_url">the AdlibAPI settings page</a>', array('@settings_url' => '/admin/structure/adlibapi')),
      '#options' => $db_options,
      '#default_value' => $this->config['database'],
    )

Also, I manually went through the rest of the code and found no issues with regard to Drupal API usage or other coding standards. However I then went through the readme and on the "Save the settings" step I get these notices:

    Notice: Undefined property: AdlibSearchResponse::$_error in AdlibSearchResponse->getXMLArray() (line 85 of /var/git/working-copies/abielefe/d7_sandbox/htdocs/sites/all/modules/custom/adlibapi/library/AdlibSearchResponse.php).
    Notice: Undefined property: AdlibSearchResponse::$_error in AdlibSearchResponse->getXMLArray() (line 85 of /var/git/working-copies/abielefe/d7_sandbox/htdocs/sites/all/modules/custom/adlibapi/library/AdlibSearchResponse.php).

Your code should not be giving notices even if most people will not see them; please use !isset($this->_error).

Also, after selecting the Adlib parser I was getting errors that said "Adlib parser: No mappings are defined." I haven't even gotten to the part where I set these mappings yet, and while I understand that you want to remind the user to do this step you should not be throwing errors around like this.

I also got a strict warning once I actually got to the mappings page:

    Strict warning: Only variables should be passed by reference in AdlibParser->getMappingSources() (line 60 of /var/git/working-copies/abielefe/d7_sandbox/htdocs/sites/all/modules/custom/adlibapi/modules/adlibfeeds/plugins/AdlibParser.inc).

I do not have an Adlib database so I was not able to progress past the "Create mappings at the processor" step. I will assume that it works fine after that point, but you should enable all error logging on your development site and fix any notices that you find, just in case.

Once you've done all this I think this project will be ready for promotion.

This application is not fixed? See http://drupal.org/node/532400

Verified the automated code review results. It checks out

I installed and followed the steps in the readme. I didn't get any error messages.

But I wasn't able to successfully complete all the steps. I could only get to the step "Now first create the mappings at the processor (select an adlib expression)". There were no fields to map. (See attached screenshot.)

The instructions give the impression that I should be able to complete the steps using the "adlib_sandbox" as the database. Should I be able to? If I should, there is an issue. If I shouldn't, the readme should be updated to reflect that.

The project page could use some work. It looks pretty unstructured. For Drupal users who aren't familiar with adlib, a brief layperson's description or example of what they could with this module and the adlib software would be nice. (That would fall under the "overview" section of the project page.) See Tips for a great project page.

Adding the security tag based on comment 15. I believe I also found potential XSS via the watchdog of an error message from the connection process. It seems ideal to use the ability of watchdog to do filtering and add context to the message like:

watchdog('adlibapi', 'Error connecting: @error', array('@error' => $response->getErrorMessage()), WATCHDOG_ERROR);

I only reviewed via the web so I didn't do an exhaustive review, but I couldn't find where theme_adlibapi_render_database_info is being called. Depending on where the rawtext and fieldname variables are coming from they should be similarly filtered by a function like check_plain or filter_xss.

In the ctools export code it seems that a user supplied url is used without validation. The url function seems like it could be handy (or more specifically drupal_strip_dangerous_protocols). I didn't trace through 100% to see where the result is being used, though, and perhaps the value is being filtered by something like url() on output, which is totally valid and appropriate.

Feedback that shouldn't block promotion:

This error message is not very easy to read and take action. I suggest adding a link to the place someone can configure the database and maybe using the word "adlib" to describe what kind of database. In that same function, you are currently returning FALSE on a fail or the populated connection. It might be a good spot to throw an Exception instead of returning FALSE.

Where is the errorstring used? I note two things with that line: it's not being translated using t and there's a typo "address" instead of "adres"

I see you're using CURL instead of drupal_http_request. Is there a reason to use CURL? If so a comment in the code explaining the reason would be helpful. If not, it would be nice to switch to gain compatibility on systems where CURL is not installed.

Thanks for making the updates jurcello. You've obviously been putting a lot of work into this module. I followed the instructions in README and was able to create a feed and import content. Then viewed the import log and successfully deleted all imported content. I got a bunch of nodes that had titles empty, but saw the README states the title in the sandbox may not always be populated. So it appears to be working as expected.

With the functionality down, I moved on to look at the code.

D7 code

AdlibFetcher.inc

drupal_set_message($response->getErrorMessage(), 'error');
watchdog(ADLIBAPI_WATCHDOG_ERROR, 'Adlib api error: %error', array('%error' => $response->getErrorMessage()), WATCHDOG_ERROR);

In the first function call, $response->getErrorMessage() is not escaped before output. In the second, it is. So I checked to see if it should be escaped and found the values of $response->getErrorMessage() can be populated by external XML data. So #42 is correct, it is a possible XSS. I see you made the correction for #42, but you should also sanitize $response->getErrorMessge() before sending to drupal_set_message().

Change the drupal_set_message() call to:

drupal_set_message(check_plain($response->getErrorMessage()), 'error');  //or filter_xss instead of check_plain

adlibapi.apifunctions.inc

You missed one. It should get sanitized the same as you did in AdlibFetcher.inc. (And probably change 'adlibapi' to your ADLIBAPI_WATCHDOG_ERROR constant. Though that's not something that should hold up promotion.)

watchdog('adlibapi', $response->getErrorMessage(), NULL, WATCHDOG_ERROR);

AdlibSearchResponse.php

You don't wrap the value of the errorString property in t() but do so in other places.

$this->errorString = "Query did not return valid XML";

Should change to:

$this->errorString = t("Query did not return valid XML");

ad-libapi.module

This shouldn't block promotion, but you might also want to use your constant ADLIBAPI_WATCHDOG_ERROR instead of 'adlibapi' in the watchdog() call.

D6 code

The Drupal 6 code has lots of places where the output needs to be sanitized. Start by doing a search in your code for watchdog and drupal_set_message.

Make sure you follow the same convention as you did to fix the watchdog message in response to #42.

Example:

watchdog("adlib_image", "An error occured while saving the remote file to $filename");
drupal_set_message(t("An error occured while saving the remote file to $filename", 'error'));

Should be:

watchdog("adlib_image", "An error occured while saving the remote file to @filename", array('@filename' => $filename)); //possibly with WATCHDOG_ERROR as the 4th parameter
drupal_set_message(t("An error occured while saving the remote file to @filename", array('@filename' => $filename)), 'error');

Note about that particular line of code in ad-lib_image.module. You also have a parenthesis out of place. You're passing 'error' to t() instead of drupal_set_message().

I just did a quick review of your module and just had a couple of quick comments. Overall, the code looks clean and straightforward. You have date_popup as a dependency for the adlibfeeds module, but that is actually in the Date module now. So I think you want that dependency changed to Date, as you have it written in your README.txt file. Also, it looks like Coder is still outputting a few code-related warnings, many of which recommend using count() instead of sizeof(). Also, in line 272 of adlibapi.apifunctions.inc, make sure to use check_plain() on your $link variable. Good luck!

Reviewing the 7.x-1.x branch.

Automatic Review:
It helps if you can use one of the automatic code style checkers, or alternatively using something like: http://ventral.org/pareview

FILE: /var/www/drupal-7-pareview/pareview_temp/adlibapi.apifunctions.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
81 | ERROR | Case breaking statements must be followed by a single blank line
82 | ERROR | Line indented incorrectly; expected 6 spaces, found 8
89 | ERROR | Case breaking statements must be followed by a single blank line
90 | ERROR | Line indented incorrectly; expected 6 spaces, found 8
94 | ERROR | Line indented incorrectly; expected 6 spaces, found 8
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/library/AdlibBaseResponse.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
106 | ERROR | Invalid @return data type, expected bool but found boolean
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/library/AdlibImageQuery.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
62 | ERROR | Invalid @param data type, expected int but found integer
72 | ERROR | Invalid @param data type, expected int but found integer
146 | ERROR | Invalid @param data type, expected int but found integer
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/library/AdlibImageResponse.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
26 | ERROR | catch must start on a new line
31 | ERROR | catch must start on a new line
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/library/AdlibSearchQuery.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
102 | ERROR | Invalid @param data type, expected int but found integer
112 | ERROR | Invalid @param data type, expected int but found integer
--------------------------------------------------------------------------------

FILE: ...var/www/drupal-7-pareview/pareview_temp/library/AdlibSearchResponse.php
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
55 | ERROR | catch must start on a new line
134 | ERROR | Function comment short description must start with exactly one
| | space
136 | ERROR | Invalid @return data type, expected int but found integer
146 | ERROR | Invalid @return data type, expected int but found integer
--------------------------------------------------------------------------------

FILE: ...pal-7-pareview/pareview_temp/modules/adlibfeeds/plugins/AdlibParser.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
130 | ERROR | sizeof() is a function name alias, use count() instead
--------------------------------------------------------------------------------

FILE: ...-pareview/pareview_temp/plugins/export_ui/adlibapi_ctools_export_ui.inc
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
46 | ERROR | sizeof() is a function name alias, use count() instead
59 | ERROR | sizeof() is a function name alias, use count() instead
72 | ERROR | sizeof() is a function name alias, use count() instead
93 | ERROR | A unary operator statement must not be followed by a space
93 | ERROR | sizeof() is a function name alias, use count() instead
108 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines
162 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines
--------------------------------------------------------------------------------

Very Quick Manual Review:
You are missing dependencies for "date" and "feeds" in your .info file (documented requirements from your README.txt)

jurcello, I think the main hold up is that you probably have not participated in pareview bonus mentioned in comment #22 and #32. Hope that helps.

x

Thanks for doing the reviews. Please keep the security tag on the project for metric purposes.

Automated review:

• ✓ -- PAReview gives a false positive due to the adlibfeeds submodule. No other warnings -- Looks good! Thanks for cleaning that all up.

Manual review:

• ✓ Issues from #42 and #15 seem to be resolved (Problematic code no longer in repo due to deletion of 6x branch).
• ✓ Issues from #47 are resolved.
• ✓ Issues from #44 are resolved.
• ✓ Project Page looks good -- few typos, "API" should probably always be capitalized, weird line breaks... Nothing major.
• ✗ adlibfeeds.info -- Possibly missing feeds_ui dependency: dependencies[] = feeds_ui. If you don't want to make this a dependency, it might be worth noting in the README that this should also be enabled to follow the instructions. (Shouldn't block promotion)
• ✗ README.txt -- Good walkthrough, though I'm confused with the mapping bit. I've mapped three expressions onto the "Title", "Body" and "GUID" fields -- "[title]", "[raw_xml]" and "[priref]" are the respective Adlib expressions defined the Processor settings. Note on that page, the only token listed is "[raw_xml]". Am I missing something obvious? Of the several hundred nodes I've imported, none have titles.

Leaving status for the moment: help me figure out why the other tokens aren't showing up and I'll change status to RTBC. :)

manual review:

• I would suggest to prefix the submodule correctly with your main module's name to avoid any confusion.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution, jurcello!

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.