[D7] Scald: Vine

Dear NerOcrO,
thanks for your work on this module. This sort of innovation is what drives our Drupal community forward.

Here are the things I would like you to fix:

1. Your git repository
it should say
git clone --branch master http://git.drupal.org/sandbox/nerocro/2202367.git scald_vine

2. Move from master branch to version specific branch
View https://drupal.org/empty-git-master to see how to perform that change.

3. You are missing a README.txt file
Consult https://drupal.org/node/447604 for further details.

4. Pareview.sh is your friend
Visit http://pareview.sh/pareview/httpgitdrupalorgsandboxnerocro2202367git to view more issues with the code.

Good luck,

imgio

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.

Hi there,
I don't use vine or scald, so my input may be out of context. A few thoughts are that the description/documentation on the project page and on the application page needs work. I think the functionality is pretty unique, but you should still consider beefing things up a little as far as what you have explaining the module itself and how it works. Currently there is pretty much nothing. The README.txt is equally vague, so there's really nowhere to turn for more information as to how this works. Along those lines, maybe a '#description' field in your scald_vine_scald_add_form function could be useful.

Code wise I'm concerned about two things. First,

function scald_vine_video($id) {
  return scald_vine_feed($id);
}

says that it returns an object, but scald_vide_feed supposedly returns an array. At least that's what it says in the comment before the function. This means that unless I'm mistaken, scald_vine_video is doing nothing other than calling up scald_vine_feed, i.e. it too is returning an array. This would affect everything that is based on $infos in line 61 as well. In other words, you might consider doing one of a few things. 1) return (object) scald_vine_feed($id). This will return an object. 2) get rid of scald_vine_video as it is currently redundant and adjust your $infos variable accordingly to reflect the fact that it is actually an array. 3) update your comment -- if scald_vine_feed actually returns an object -- in which case you can still get rid of scald_vine_video because it is still redundant.

The other thing code wise is that having a loose t() function here

function scald_vine_scald_atom_providers() {
  // This code will never be hit, but is necessary to mark the string
  // for translation on localize.drupal.org.
  t('Video hosted on Vine');

  return array(
    'video' => 'Video hosted on Vine',
  );
}

seems pretty questionable. Again I'm not sure what hook_scald_atom_providers() is implementing, but it seems like the translate function should be applied directly to the 'video'=>t('Video hosted on Vine'). Your inline comment sounds like you are trying to get it to pass standards on drupal, but adding the t() to the actual array value itself normally would be the way to do it. If hook_scald_atom_providers for some reason doesn't let you do this, and I don't see why it wouldn't, you might want to consider patching that so that you don't continue developing something that does not match standards.

Again, I'm not familiar with scald, although it looks great. And I don't use vine, so these are just some things I noticed looking at the code without manually testing it. Perhaps they are quickly answered.

Good luck!

On point two, I've never seen a t() function placed in another function without anything it's referencing. I'm not sure of the exact use case for you though. If you think the video value should not be wrapped in t(), then I'm sure that's fine. That said, really only for hook_menu $items->title and $item->description is when you are supposed to use untranslated items, everything else should run through t(). I don't think the video value is a well known place per the info on https://drupal.org/node/323101. But regardless, never have I seen a t() function sitting unaffiliated in the middle of a function. There is no way that is best practices. If you are doing it to pass the PAReview.sh site, and you really think you don't need a translate function on the video value, then just ignore the error from the PAreview site. After all pareview.sh is a computer and is going on what it knows, i.e. it doesn't have every possible scenario worked out and perhaps the hook you are using is a unique case. But this needs to be addressed differently from having a random unattached t function floating around in your code.

And don't forget about documentation - for instance what goes into the scald_vine_scald_add_form you've created? Is it the full URL or just a vine ID or both? This should go as '#description' if the hook accepts that value and/or be included in the project page and application issue summary. Tell people exactly what they need to do to make it work. The more info the better!

@j.branson: It's not a useless t() function floating around in some random piece of code ; this is completely needed to get your string to show up in localize.drupal.org to get a translated result.

This isn't really specific to Scald either ; this is in fact related to pretty much all the info hooks that want to have their output cached. Concretly, if you want to have the result of module_invoke_all get cached and not serve random garbage to your site visitors depending on the language that built the cache, you have two options:

• let the hook implementors use t() around their values, and hardcode the language in the cache key (this is the approach used for example by _node_types_build)
• mandate that the hook implementors use English values, translate on display (used by hook_menu for example)

Having only one copy in the cache is obviously friendlier to your cache backend, so that's what's used in the last release of Scald. Translating on display means that you end up with calls to t($provider['title']) ; it works fine when used locally, the string end up in your custom translation UI to be tweaked, but it obviously fails for static code analysis tools that tries to parse your files for t() calls, which is what PotX, used internally by localize.drupal.org does… Which means that you need to give it a hint that the string should be translated somehow, by placing a t() call somewhere, preferably in a piece of code that's never really hit during runtime. Views does it by placing a fake ['translatables'] entries in their .views_default.inc files ; the recommanded approach for Scald is to place that t() call in the function that defines the string, after the return statement, as stated in the comment. It makes the original string and the t()-call next to each other, which improves drastically the odds that the two will be kept in sync.

Leaving at NW for the description stuff.

Defr, thanks I'd never seen that before but I'll take your word for it. Eitherway, the code I looked at yesterday had mived the t() function to after the return. My guess is that it needs to go before since the function is no longer running after return. No? NerOcrO sorry if I confused you there, it still seems pretty obscure to me.

manual review:

• project page is too short. Link to Scald is missing, see the tips for a great project page: https://drupal.org/node/997024

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.

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

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

Thanks for your contribution, NerOcrO!

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.