mediaWrapper

Welcome!

Issue to fix:

• You are working in a missnamed branch in git (You won't be able to create a dev release with that name). Please reread the release naming conventions and creating a branch in git.
• README.txt is missing, please have a look at the guidelines for in-project documentation.
• Remove the translations folder, translations are done on http://localize.drupal.org
• Please move all your assets (images, etc) into a misc or a image folder

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxmusikpirat1508144git-...

regards

Nah, that's not so much, just minor things ;)

Thanks for your reviews, just make sure that you pick applications that did not get a review in a long time.

manual review:

• remove the empty folder "mediawrapper".
• "drupal_add_js(drupal_get_path('module', 'mediaWrapper') . '/js/jQuery-mediaWrapper . js');": white space in the file name, does the inclusion even owrk?
• "Implements filter_tips().": should be "Implements hook_filter_tips()."
• "please read <a href="http://foo/bar">this</a> article.": wrong link.
• mediaWrapper_doFilter(): use the respective drupal sanitization functions, see http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-...
• mediaWrapper_doFilter(): do not build link markup yourself, use l() instead.

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

Hello musikpirat,
I installed the module in a test system and successfully used a youtube video integration protected by your JS.
But because this is a subfolder the icons are not displayed. Your JS is building a path like example.com/sites/all/modules/mediawrapper/images/youtube-icon.png and not example.com/subfolder/sites/all/modules/mediawrapper/images/youtube-icon.png
By the way: What about license of the icons? Are you sure we can host this images on drupal.org? But this is a question for lawyers.
Additionally I found some german text in the JS file.

Oops your branch is name 6.x-0.x I think it should be 6.x-1.x

http://drupalcode.org/sandbox/musikpirat/1508144.git/blob/refs/heads/6.x... is very long should be around 80 chars

don't use camelcase for function names. those are only for objective stuff (classes, methods, etc)

function mediaWrapper_doFilter

$media = explode("]", $matches[0][0]);
$description = NULL;
$url = NULL;
$link = NULL;
$id = NULL;
$icon = NULL;

dont pre-initialize your variables into nulls.
http://drupalcode.org/sandbox/musikpirat/1508144.git/blob/refs/heads/6.x...

if ($key == "url") {
  $url = $value;
}
elseif ($key == "id") {
  $id = $value;
}
elseif ($key == "link") {
  $link = $value;
}

^ should probably be a switch statement
if (is_null($url))
You can just use the empty() function, since is_null will check for NULL not for an empty value

You also have a .save file committed - haven't seen one of those - is that maybe from your ide?
If so, add it to .gitignore

Also I think you should put mediawrapper_oembed into a separate subfolder

Both module names should not be camelcase

use single quotes, and double only when necessary - http://drupal.org/coding-standards#quotes

http://drupalcode.org/sandbox/musikpirat/1508144.git/blob/refs/heads/6.x...

You have some commented out code around that line ^

Do an overall read through of http://drupal.org/coding-standards and check your code agains that :) Helps a lot w/ review :)

Also make sure you follow the coding standards for documenting your functions - http://drupal.org/node/1354 and document your params for functions

Gotcha. Cool. /me do wonder what interesting things, but purely for an educational sake ;)

1. Keep your commit messages a little more detailed then "fixed code", also have a look at Commit messages - providing history and credit
2. Think about putting the _oembed submodule into it's own subdirectory (it's more structured and maybe wont work this way in d8)
3. you're doing drupal_get_path('module', 'mediawrapper') in hook_init, but you already know where your module is by doing dirname(__FILE__) instead (works just faster)
4. not sure but isn't the basepath already set at Drupal.settings.basePath ?
5. don't spit sentences over multiple t()'s, this will make your module very hard to translate. use t()'s replacement patterns whenever possible
6. I personally don't like this _do_-function naming, it's more common to name them [module]_[verb]_[object] (eg- mediawrapper_do_prepare to mediawrapper_prepare_text)
7. never ever encapsulate text with t() in hook_menu() (and watchdog())
8. your not deleting the used variables in your submodule on hook_uninstall()
9. your using $st() but you've never set $st = get_t();
10. drupal should already make sure that its temporary directory is writable, you don't have to duplicate this requirement

removed "review bonus" tag, you can add it again after you've done 3 more reviews, remember to pick old ones first
thanks

regards

hmm, I'm sorry for that, corrected the documentation you mentioned. (making everyone able to edit wiki pages is both, blessing and curse)
This whole process is about making sure you can write good secure code, we DO care what your doing afterwards, but we hope you take the lessons learned with you so we don't have care too much ;)

looks fine for me now

// btw, rather then relying on any wiki page you should always have a look at api.drupal.org and look how core does things (because it's [most of the time] right and not editable without beeing checked before)

manual review:

• jQuery-mediaWrapper.js: Asterisk indentation errors in the first doc block.
• The input filter has no label on the settings page on the left of the checkbox, only a description.
• when adding the example media tag I get PHP notices after saving: notice: Undefined index: url in mediawrapper/mediawrapper.module on line 96. Make sure to enable PHP notices in your development environment.
• mediawrapper_oembed.module: "'access arguments' => array('access administration pages'),": that permission is too generic, provide your own or reuse one that makes more sense.

Although the last point could be considered a security blocker for this approval, I'm not that strict today as I can see that you are highly motivated and you'll fix that ASAP anyway :-)

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

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.