[D7] PA11Y Integration

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.

variable_get('pa11y_integration_url', 'http://192.168.2.195:3000')
The default url for the sebservice is http://192.168.2.195:3000, I would suggest this would not be a useful default in the majority of cases. Maybe localhost would be better or alternatively do not supply a default and have a hook_requirements error untill a url is entered?

curl_exec()
PA11YClient::curl() uses curl for http requests. Unless there is a good reason for using curl it may be preferable to override this in PA11YDrupalClient using the included function drupal_http_request().

PA11YDrupalClient::formatTask
This method contains a fair amount of html markup, it may be better to using a theme function here (possibly theme_item_list). Also strings such as 'Results not ready for task...' should be passed through t().

/admin/config/system/pa11y_integration
Most content validation modules seem to live at '/admin/config/content' rather than 'admin/config/system/'.

Invalid error message.
Upon clinking 'Start PA11Y test' with an invalid webservice url, the response was '2 items successfully processed. 0 documents successfully sent to PA11Y for testing.' This was an INFO message but should really be an ERROR message. The occurance of the error is really only present in watchdog. Also, could the message give a link to the results page? The request works in drupal_http_request (RC:202) but not in curl (RC:0).

If you could fix this I can review the remainder of the module?

Thanks

I noticed some problems in pa11y_integration.admin.css:

Line 5 - color is missing the '#'
line 12 - empty url()
line 53 - color is missing the '#'

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

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Michel

Thank you for your module, I have noted a few possible improvements:

In your settings form, pa11y_integration_admin_settings_form, you have a field called pa11y_integration_url. This field currently has no validation but some aspects of validation could easily be added (such as using parse_url() to check for a valid url).

Adding a 'configure' to your info file would help as this adds a link in on the modules page to your settings page.

In PA11YDrupalClient (being a Drupal client), shouldn't the formatTask() be a theme function?

Best of luck with your module,

- Alastair

manual review:

1. PA11YClient.php: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
2. pa11y_integration_install(): the check_plain() is wrong here since no user provided text is involved. Make sure to read https://drupal.org/node/28984 again.
3. pa11y_integration_init(): why do you need hook_init()? This will run on every single page request, regardless if your module is relevant or not. You should only include libraries and CSS files when you actually need them, i.e. in page callbacks or similar.
4. pa11y_integration_theme_task(): indentation errors for the doc block.
5. pa11y_integration_theme_task(): HTML tags should be outside of t() calls where possible.
6. pa11y_integration_get_results(): the output generated here should ideally be a render array, see https://drupal.org/node/930760 . Do not call drupal_render() or theme() yourself, Drupal will render a nested array automatically for you. And for all the custom tags you should use a proper theme function or template with hook_theme().
7. pa11y_integration_batch_test_tasks(): do not set the PHP execution time in a module, this might not even work on some hosts.
8. "watchdog('PA11Y delete operation failed', $e->getMessage(), NULL, WATCHDOG_ERROR);": usually the first argument to watchdog() should only be the module name and the message should elaborate.
9. pa11y_integration_admin_settings_form_validate(): doc block should follow https://drupal.org/node/1354#forms
10. pa11y_integration_get_content(): this will not scale for any site that has more than a couple of hundred nodes.
11. pa11y_integration_get_content(): do not build the URLs yourself, just use the url() function which will give you correctly aliased absolute links.
12. pa11y_integration_batch_test_tasks(): so instead of querying all nodes upfront you should probably just get a count of nodes that you want to test, and then creating the URL paths in each batch step for a limited amount of nodes.
13. pa11y_integration_theme_task(): this is vulnerable to XSS exploits. If I'm no mistaken $task->name is an unsanitized node title, correct? That is user provided input and needs to be sanitized before printing. Make sure to read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

manual review:

1. PA11YDrupalClient.class.inc: Oh, I thought that class was a straight copy from the Github repository, that's what we should not do. If you wrote the class specifically for the module and it is not hosted anywhere else that is fine, just mention that in the @file docblock like "inspired by the javascript implementation by author_xyz at github ..." or similar.
2. pa11y_integration.info: should have a dependency on PHP 5.3 (Drupal 7 core only requires 5.2), since you are using PHP namespaces, see https://drupal.org/node/542202#php
3. pa11y_integration_form_alter(): if you are only targeting one specific form you should use hook_form_FORM_ID_alter() instead.
4. pa11y_integration_menu_breadcrumb_alter(): why do you need this? Please add a comment.
5. pa11y_integration_theme_task(): the output generated here should ideally be a render array, see https://drupal.org/node/930760 . Do not call drupal_render() or theme() on nested elements yourself, Drupal will render a nested array automatically for you.
6. pa11y_integration_theme_task(): if you have a render array at hand use #attached instead of drupal_add_css() and drupal_add_library(), see https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

But otherwise looks RTBC to me.

Assigning to chx as he might have time to take a final look at this.

Realistically, I need to pass. I don't have free time left in January.

no objections for more than a week, so ...

Thanks for your contribution, michel_v!

I updated your account so you can 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 stay 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.

Great to see this get approved:
https://drupal.org/project/pa11y_integration

Is this the web service that it leverages?
http://pa11y.org/

I should just go ask this question there I suppose...