LTI Tool Provider

Hi John,

Glad to see your group's work on this! From just reviewing code (am not set up at the moment to try providing or consuming), this is much more in line with Drupal standards (and much more developed) than samo's BasicLTI sandbox project.

After a scan last night (while waiting to see if the hurricane would cut out our power!) - here are a few responses:

Important

• Function naming does not always follow the convention of using module name as prefix. See http://drupal.org/coding-standards#naming
• require_once(drupal_get_path('module', 'user') . '/user.pages.inc'); calls a Drupal API function outside of a function body, which should not be done. See #1751704: Code should not call Drupal API functions outside of a function body
• All functions should contain Doxygen documentation. See http://drupal.org/node/161085#doxygen and http://drupal.org/node/1354
• Hooks should be documented in MODULE_NAME.api.php. See http://drupal.org/node/161085#api_php

Minor

• A good rule of thumb is to add only hook implementations and API functions in the main .module file (administration screens can be added to MODULE_NAME.admin.inc and included in your hook_menu() items, etc).
• It's a good idea to make sure cron is not running drupal_goto() (in _lti_tool_provider_launch(), _lti_tool_provider_return(), and _lti_tool_provider_home()). See #102138: cron breaks on drupal_goto and #308808: Drupal_cron_cleanup paints the wrong picture
• files[] = lti_tool_provider.install does not need to be listed in the info file because that file doesn't contain any class or interface declarations. See http://drupal.org/node/542202#files
• Inline comments should also follow comment standards (it's great to see them, but they need a bit of cleanup too). See Doxygen link above
• LICENSE.txt does not need to be in the repo, since Drupal.org's packaging script adds it automatically
• Authorship info (now in AUTHORS.txt) is perfectly acceptable to be included in the README

Opinions

• Personally I'd still like to see backend LTI functionality broken out into a separate API module - which is why we initially started the LTI API project (unfortunately other projects and switching jobs halted progress on that before I was able to spend much time on it - that namespace is available if anyone would like to take on that task!).
• Also, feature request - integrate with Course module ;)

I wrote up a review, but didn't post it yet. I think the approach is sound with some issues noted above. I don't think there are many "major" issues that would prevent this from being a contrib module other than the function naming that scottrigby mentioned. I would consider the drupal_goto() in cron a major issue because it could have some unattended side effects for sites.

Everything else is a minor issue.

Also, see #1827050: Consider using get_parameters method instead of $_POST.

Edit: Probably a note that it may be incompatible with oauth (oauth_common), oauth_api (oauth), and other modules that bundle OAuth directly on the project home page.

Edit 2: I was considering "major" as blocking issues in review, but my concept of major is not correct.

Agreed that "major" in #1 sounds a little too close to "critical". I think only the namespacing qualifies for that.

After a quick chat with mradcliffe on IRC I edited & changed the header in my comment to "important", because IMO those other points still are more than minor to sort out, whether before or after promoting to a full project.

Nice work John - This all looks great (aside from my other suggestions, which can be taken up in the issue queue).
The only other coding standards-related thing i noticed today I just made a follow-up issue in the queue [#Convert dos-style line endings to unix-style], but that's something that can also be done at any point (probably sooner than later though, otherwise patches would be difficult)

Accidental change of status - i need to remember to refresh the page.

In fact this should be RTBC from my point of view, but I'll wait for another opinion.

Hi John,
I had a quick look, overall it looks really good, well done.
I have a couple of suggestions for improvement.
Some of the functions are pretty big, it would be good to refactor them a little.
One example is function _lti_tool_provider_launch() you could pull out the og related stuff which is about half the function, possibly put it in an inc file.
You also have a class in the module file that would be better in it's own file.

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 :-)

Don't RTBC your own application. See http://drupal.org/node/532400

This module seems solid and ready for full release.
Changing the status to reviewed

Can also confirm working. I think this is ready for full project status given acceptance of changes asked for by the community in this thread as well as multiple confirmations of code working. This is a great need in the education community as it will allow Drupal to talk to all major LMSs on the market.

Did a coder audit set to find minor issues and only 1 white space at end of line came up: #1845090: lti_tool_provider.admin.inc Line 443: There should be no trailing spaces (very clean)

Looks RTBC to me too.

John, it seems I can't email you at all as I get no reply and no bounce.

The project page is great, the overall coding looks good if not a little crowded (made in an IDE that auto spaces braces? no crime), translated strings are mostly there, but there are some small things to fix and some rather larger ones that should either be fixed or rebutted. As I don't have a system I can tie this into, there may be aspects that influence why these choices were made that I cannot know.

Look out, my list of things extended beyond a few. Some of this is just commentary that I hope will be digested and perhaps integrated. Given the interest in the module it shouldn't be hard to either rebut or fix most of this swiftly as it really should be the best it can be. Given the issues missed in reviews, I think those should be dealt with before the project moves to a full project.

It has taken 12+ hours to review this to this extent so let's consider this a constructive discourse.

Should be addressed or fixed now

Line endings - lti_tool_provider.module

This file is mostly DOS line endings. It has one function that is UNIX ended which is causing a large number of errors in the PAReview site. Once this is fixed, most of the errors will go away, leaving only the some code style issues that can be cleaned up any time. It is very important to ensure that only UNIX line endings are committed or you will end up with commits that replace the entire contents of files, completely hiding the changes in the patch.

File endings

There are a number of files with DOS file endings which will cause annoying patches that just consist of "No newline at end of file" and nothing else. Again, these are just empty commits and should be avoided by always using UNIX line endings even for the last line of the file.

Re-run PAReview

Once the above are fixed please "Repeat Review" on the PAReview site http://ventral.org/pareview/httpgitdrupalorgsandboxjzornig1809350git-7x-1x and see any the massive pile or errors have disappeared. I address class lowerCamelCase below.

.info file package and project

Related modules can be grouped together on the modules page in a package. It's not required, but with this being an API/Tool provider that will have many other related modules, it seems to make sense rather than having them all plonked in "Other".
The project line in the lti_tool_provider_og sub-module must be removed. This is added by the packaging system automatically.

Class filenames

While not specifically mentioned in the Coding Standards, it is highly unusual to have class files named the same as the class name sitting in the root directory. Most modules keep them either inside the code file that uses them, or in a module_underscore named file that makes it look like it belongs with the module, or in a sub-directory of the module. Drupal has no requirement for the filename to match the classname, just that it be include in the .info files[] listing if not already automatically included. Again, not required, but would make it match the rest of the d.o work.

hook_init and a conditionally wrapped class

There is a library module call in hook_init which seems only to provide an error message to anyone who attempts to view any page on the site, regardless of the library actually being needed for that page. Error messages like this should be moved to hook_requirements and if the module will absolutely not work without that library installed, it should be a fatal install requirement, not runtime.
Checking it subsequently at runtime would then not be needed, except perhaps to check the version after an update but that would be an even larger job and part of a major version release and update functions so again probably not needed.
The conditional wrapper around the class would also not be needed. Conditionally wrapping a class seems very questionable.
I would highly recommend clearing this up.
The loading of the library should done just prior to its first use in code rather than on every page load also as there will be many pages where it would not be needed, especially for anonymous users.

classes and lowerCamelCase

Both the LTIToolProviderOAuthDataStore and LTIToolProviderConsumerEntityController classes have underscore_spaced methods instead of lowerCamelCase. Since LTIToolProviderOAuthDataStore extends OAuthDataStore, you are pretty much locked into that one, but you entity controller isn't subject to such restrictions. delete_multiple => deleteMultiple; or just follow what Entity does and do multiple deletes all in the delete function with an array cast.
Coding standards on Object-oriented code

require/include_once

This is a language construct, not a function call. Syntax should be:
require_once $library['library path'] . '/OAuth.php';
And potentially include DRUPAL_ROOT unless $library['library path'] already includes this. Does it really need a require_once after using the libraries API to load the PHP library?

admin.inc

There seem to be a number of menu items that are included in the admin.inc file which would be better served in their own file. It's not a requirement but there does seem to be a difference between administration functions and user related functions which is not matched by separation of the code. It would only come into some sort of usefulness when not loading all the admin code when only a user is present.
eg. lti-tool-provider/return, lti-tool-provider/home, lti-tool-provider/info into a pages.inc?

hard coding in lti_tool_provider_get_lti_roles

These roles seem to also be stored in variable, but are hard coded here. Should these be run off the variable, or have an interface for editing them? Running check_plain on a hard coded value in lti_tool_provider_global_roles is a tiny bit of overkill too.

lti_tool_provider_consumers_admin - table theme

Use #empty table key to provide the empty table message rather than wrapping it in a condition. This keeps the display consistent when it is empty.
The method used in here to fill the table cells is normally used when making a table full of FAPI fields, not just displaying text. Bit of an overkill, but it works.
The values should be run through check_plain as they appear to be user entered values in the next form. It seems also needed in lti_tool_provider_consumer_view as the values are dumped directly to HTML there too.

form_set_error('', 'msg')) and lti_tool_provider_consumer_form

There are quite a lot of places with an empty field selector for the form_set_error. This isn't great for usability as it will only give a generic error and not specifically highlight the item that needs attention. eg. form_set_error('', t('Domains must be unique.')); This error knows very well that it should be highlighting lti_tool_provider_consumer_domain
There preg_match of the domain just below that should probably be before it's run through every other validation function. It would almost be the first thing to look for, not the last.
Consider putting the action buttons of a form inside of a '#type' => 'actions' as "Use of the 'actions' element as an array key helps to ensure proper styling in themes and to enable other modules to properly alter a form's actions."

lti_tool_provider_retrieve_user_field_types - user output

$fields['label'] is not display safe. If used in a FAPI select field, the value is run through check_plain for you. All the current uses of the values returned from this function are safe, but it should probably be indicated to be an internal use only function by prefixing it with an underscore, and note that the returned description is not display safe.

lti_tool_provider_get_column_desc - user output

The description field from an entity schema - not something seen by anyone but developers. The method by which this value is returned and displayed is not safe, although any unsafe value would be hard coded so any issues would be very limited in scope. Considering most developers never expect this to be displayed, it may well have unsafe characters. Sending it though t() does not make it safe without it being passed as a parameter and then given it would be t('@var', array('@var' => $var)), is also insane when check_plain would do. The value itself is recommended to not be translated in hook_schema since it is pretty much never displayed to anyone via the site. As such, it will not be picked up by localize.drupal.org as a string to provide a translation for, making the call to translate superfluous. It only picks up t('Hard coded string') type values. There may be a way around this I am not aware of.
My assumption for the next part is that not all schema fields will want to be included in the display of a single consumer entity. There will be fields on there that do not need to be included, and the creator of the entity is most likely the person to know that. As a result, you could create a new hook_XXXXX_info() setup to describe only your entities' interesting fields in a translated format. At present it really is using secret sauce.

lti_tool_provider_create_account - example.com

example.com should never be used in such a way that it may generate emails to that address eg. password reset emails. Please allow this to be configured, or use an address guaranteed to fail routing into the future, which given the GTLDs now days could be hard. Perhaps .local? It is a reserved name and should never resolve on the internet at large.

Entity module vs implemented entity

The entity lti_tool_provider_consumer does not seem to take advantage of the Entity API module but instead only use Drupal core entity functions. Should the Entity API module really be a requirement of this module? See module key below.

lti_tool_provider_entity_info - invalid keys

'access callback' does not belong here. The current value is also a permission string which is itself wrong as this key is only valid with a function name as part of a bundle declaration.
The 'module' key is only used by Entity API, but nothing else is enabling any aspects of that module such as views detection or making them exportable. Am I missing something?
http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

lti_tool_provider_launch

(this is where the library should probably be loaded instead of hook_init or inilne wrapping the class)
There are variables that are not guaranteed to be here, that are even being casually checked for existence that if not present will cause PHP Notice errors. Please ensure that code is E_ALL clean.
Exception message should be sanitised for display as it's source is external code/interface. Some of the error messages seem not to be translated? Is this intentional?

Some items that caught my eye

• module_invoke_all usage doesn't allow for variables to be passed by reference which makes the two variables immutable. The API documentation in .api.php doesn't mention the variables passed in and tells the user to look in the session variable, which 5 lines later is overwritten by the variable after it's been through an alter call. This seems quite confusing but it may have its purposes. Normally this pair would build an array from a set of modules and then allow any other modules to alter that information. It almost seems like the hook_X is a bit of a dud and the power is really in the hook_X_alter call immediately after it. This might just be a case of clearing up documentation regarding what can and can't be done in the function.
• The $destination variable also has an interesting life? It's first allowed to be altered by all functions as the 1st context variable, then appended to by an altered launch_info value? Better served by having a single path to create the goto path and just using the custom_destination value instead? Based on the duplication of this almost exact functionality in lti_tool_provider_home(), this does actually seem to be on purpose, even if the result in this case is a little unpredictable.

'Launched' string should be translated.

lti_tool_provider_return_title and lti_tool_provider_home_title

static string without t(). Unknown source label not run through check_plain. This is a title callback, meaning it has replaced the default t() and as such, must replace this functionality.

lti_tool_provider_return and lti_tool_provider_goto

@return description doesn't match reality - function always returns no content, not "page content if any". The two paths of wrapped drupal_goto mean that the return statement should never be reached, removing the need for this fake doxygen param.
lti_tool_provider_goto should behave just like drupal_goto and never return, instead exiting via drupal_exit() and preventing the need for a return statement in the other function.

lti_tool_provider_info and lti_tool_provider_user_attributes

A theme table that should be using #empty instead of a wrapper to determine if it's empty to keep the display the same at all times. In lti_tool_provider_user_attributes, does this need an #empty too? The default value for this variable seem to indicate the possibility of it being empty and making the table disappear, but then the variable does seem to be guaranteed to always be there.

#submit array provided to lti_tool_provider_user_attributes is superfluous as this is the default value added by FAPI, as shown by the undeclared #validate. Same with the explicitly set default #submit array in lti_tool_provider_global_roles. No real need to change this, it's just unexpectedly stating the obvious.

For the future

git commit messages and attribution

Please refer to Commit messages - providing history and credit about giving yourself some credit and properly formatting commit messages to provide more information. eg Issue #1234 by Santa: Marked all children as bad. Adding this issue to commit messages is not needed.
Given the model employed in this project, any other contributors should also have d.o accounts which can be credited with the commit even if they are not maintainers, allowing you to vet all the commits.

ED: remove uninstall issue (I'm blind), add some clarifications, and fix spellings.

Lots of fixes, but a few issues from the list still exist, however the project is being managed as a full project already and it is ready for going to a live project as it would be better served as one. The community of users for the project will be able to find and fix any issues much better than anyone reviewing it in the queue.

I've opened up the following issues on the project to be rectified like normal bugs.
#1858628: lti_tool_provider_goto wrapper should function like the thing is wrapping
#1858646: Static strings not translated

I was going to completely call it all off when I saw that "else if" in there ;) #1851820: Lookup user by lis_person_contact_email_primary

There's no way to change these now, but you've actually credited me and others for work that you have done in the commit messages - I only reported the issue, but you did all the work. The commit message would then be "Issue #1858646 by jzornig: Fixed some untranslated strings." unless I'd actually made some kind of useful contribution towards the solution instead of just reporting it.

If someone submits a patch that you use verbatim (or a high percentage of), you would also credit them as the author of the commit using cmd line options. eg. for you --author="jzornig <jzornig@446626.no-reply.drupal.org>. This is listed on every users profile page. http://drupal.org/user/446626
See Adding a commit author and Commit messages - providing history and credit

Anyway, since this project is well on it's way and you have demonstrated a good understanding on the Drupal API ..

Thanks for your contribution, John! 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.

Thank you ELC and congratulations jzornig, this is a huge step forward for the drupal in education community!