CVS edit link for lavagolemking

I am applying for a CVS account to publish and maintain a new Drupal module written by the Electronic Frontier Foundation. I have been working with Tim Jones from the Electronic Frontier Foundation on a module called MyTube . MyTube was written in 2008 as a way of serving Flash content or other remote embeds without putting users' privacy at risk with things like Flash cookies or remote referrers . Unfortunately, the project had not been worked on since early 2008, was not very configurable, and only worked for Drupal 5 (what EFF uses). As a Drupal 6 administrator at opensource.osu.edu, who was interested in running it on our website, I picked up the project, fixed a few bugs, made some improvements, made it compatible with Drupal 6, and converted it from using nodeapi into an input filter for flexibility purposes. I then backported my changes to Drupal 5 and passed the code onto EFF . The current version is located at http://opensource.osu.edu/mytube and also in our git repository at https://opensource.cse.ohio-state.edu/git/swaneybr/mytube.git .

MyTube is an input filter (it used to use hook_nodeapi in original version) that replaces all embed, object, and iframe tags with locally-hosted thumbnails that, when clicked, run a JQuery script that swaps the thumbnail out with the original embed code. Effectively, until the user clicks play, no remote request is made to the remote site hosting the Flash (or whatever else) content, so the user cannot receive referrer headers or Flash cookies until opting in. Below each thumbnail is a (configurable) disclaimer warning users about the privacy risks in loading the content. The disclaimer includes a (configurable) link to a page about privacy concerns for Flash content, which by default goes to https://www.eff.org/deeplinks/2008/02/embedded-video-and-your-privacy and also has a link to the source of the video, with the link text being the top-level domain. This ensures visitors can know what they are clicking to load before they load it. In summary, MyTube can be thought of as a site-wide Flashblock . To alleviate the NoScript-esque inconvenience this may pose to some users, especially those who don't understand or care about the privacy implications of some video-hosting services, MyTube also automatically modifies embed code for a number of websites so it autoplays when loaded, and then locates and downloads the thumbnail for that embed, so the appearance and experience is roughly the same as if the embed code itself were pre-loaded with the page. See an example at http://opensource.osu.edu/node/299 . In this way, unlike with NoScript and the like, the user only needs to click ONCE to play the video for many video-sharing websites (I am continuing to add more as I find them). If the source is not supported, a default thumbnail is displayed instead, and all thumbnails (default or not) are overlayed with a play.png symbol. In such unsupported cases, users can also add a thumb= atttribute to the embed code to specify their own thumbnail. Wherever the thumbnail may be from, it will automatically be resized to the size of the embed, or (if so-configured) the embed resized to an administrator-set width and height. Currently, thumbnails and autoplay are supported for YouTube, Metacafe, and Vimeo, and autoplay alone is supported for a handful of other locations.

What I hope to accomplish by submitting this to drupal.org is 2 things: (1) I want MyTube to automatically find updates, especially if (god forbid) there were some security vulnerability in our code, and (2) more users will know about and feel comfortable deploying MyTube. If it's adopted on a large scale it will be highly beneficial to end users who are mostly unaware of the massive amounts of data user-profiling and marketing companies are collecting about them, but potentially also beneficial to some companies (such as security vendors) as a PR move to show they care about your privacy. While naïve visitors will not notice anything happening, the ones who know and care about online privacy will appreciate that visiting a page with a single YouTube embed will not be added to a central database somewhere. I have spoken with the technical staff at EFF about hosting it here, and Tim Jones has agreed to sign on as a co-maintainer for the project, while I plan on being the primary maintainer. MyTube is licensed under the GNU General Public License version 2 or 3 (currently a license file is included for both, but I can remove them if it's against your terms). If you would like to contact Tim, let me know; his primary e-mail address has changed since leaving EFF. If you wish to speak with someone at EFF about the project, their main contact is now Chris Contelini , but he has not contributed any code to MyTube.

I shamefully admit I am new to Drupal development, or web development in general, but to the best of my knowledge the code submitted complies with your security best practices and coding standards, and API calls are made in place of standard PHP libraries wherever possible. As far as I know, there is no other server-side protection that does what MyTube does, not just within Drupal, but anywhere. There are a couple of end-user tools in the form of add-ons for Firefox, including Flashblock and NoScript. While these are more secure for security-conscious users, and they work for all websites, MyTube will work for all** visitors of the website regardless of what security plugins they may or may not have.

For security issues, MyTube does not currently access the database in any way other than the variable_get function. The primary security concern is that it is an input filter, and should handle user input in a safe and careful manner. While the removal of unsafe or potentially unsafe content is normally handled by the HTML Filter, users must technically be allowed (either by disabling this filter or whitelisting the appropriate tags) to submit embed, object, param, or iframe tags. Therefore, I consider any instance where such tags can pass through and automatically load (in any fashion) a security vulnerability. There has been one such vulnerability in the original version, which I can describe in more detail on request, but calling the HTML Corrector before MyTube processes the code will mitigate the issue (MyTube therefore calls said filter before invoking its own methods). Relevant embed tags can be selectively allowed or disallowed, and MyTube will process only what the user is allowed to use. In the event of a security vulnerability, if a fix is not immediately available, it can be mitigated simply by removing embed/iframe/object/param tags from the HTML Filter whitelist, and MyTube will pose zero threat. Simply disabling MyTube without revoking this access will allow everything to pass through unfiltered, but disabling MyTube will not create any benefit (or problems for that matter) if the tags are also removed from the whitelist. The only other issue I am aware of, which is minor but I'm considering mitigating anyway, is that in rare isolated cases MyTube has failed to detect where a video source is located, and therefore not display a link in the disclaimer. I'm considering adding code to handle these embeds differently, warning users that the code appears malformed and may be malicious, but this would probably fall more in line with a new feature. MyTube was originally written by Tim with the intention that it would be run in a tightly-managed environment, where everyone posting content trusts one another (or a single-user blog), but I hope to make it safe enough so administrators can safely allow many relatively anonymous users to post embed content without it every automatically launching for anybody without their explicit per-case consent.

**Note: There is an IE Message setting, which displays a warning to users of Internet Explorer about possible incompatibility. I have in the past month noticed that Internet Explorer is actually loading the videos, so it may have been a security setting. The feature is still there in case of problems and an administrator wants to explain, and does nothing if left blank, but I am no longer able to reliably reproduce the error in IE. In past testing, IE has failed to load any kind of embed/object that wasn't pre-loaded with the original page. This including loading the SWF directly, or any kind of adding of the object through, for example, ELEMENT.innerHTML. Such a setback would make it impossible for MyTube to function, no matter what the implementation, but has not been seen for over a month. Furthermore, no such issue has been encountered with any other browser that supports javascript.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

l@va’s picture

FileSize
44.09 KB
45.29 KB
apaderno’s picture

Status: Postponed (maintainer needs more info) » Needs work
Issue tags: +Module review

Hello, and thank you for applying for a CVS account.
We just review a theme/module per applicant; this means also a single Drupal version of a module/theme. Let us know which one you want reviewed.

siliconmeadow’s picture

Thanks for the comprehensive write up. I've not considered this issue before. I'm happy to test the D6 version. Considering the impending release of D7, and the fact that the module has only been in use on the EFF site (and few others?) there may be little enthusiasm from the community to test the D5 version.

apaderno’s picture

Status: Needs work » Needs review
wadmiraal’s picture

Status: Needs review » Needs work

Hya.

  • First: coding standards. Try the Coder module for help.
  • Then (not blocking, but just a thought), lines 358 to 363 encourage a bad practice: hack the source code !
        /*
         * Note to webmasters: If you replace play.png, change this 1 line for proper display.
         *
         * ((int)$width / 2 - 30) should be ((int)$width / 2 - (WIDTH_OF_YOUR_REPLACEMENT / 2))
         * ((int)$height / 2 - 30) should be ((int)$width / 2 - (HEIGHT_OF_YOUR_REPLACEMENT / 2))
         */
    

    You might add a field to the settings form: "Enter size of play.png" (defaults to 30x30).

  • Your .info file should just be:
    ;$Id
    name = MyTube
    description = Protect visitors' information from third-party embeds like YouTube.
    package = Input filters
    dependencies[] = filter
    core = 6.x
    

    The version will be added by CVS.

  • There's no need for a License file. Will be added automatically.
  • I think your dump() function is not necessary :-). Try using dsm() (Devel module) for debuging.
  • Finally, I activated your module...and it outputs the entire .module file content at the beginning of my pages (before the html tag). On all the pages...I read through your code and have no idea what is causing this. Might want to look at it.

Kind regards,

l@va’s picture

Sorry my reply took so long; I was expecting to get e-mailed about any coming updates, just like about applying in the first place. If I can only have 1 version reviewed, then definitely go with D6. Most of my testing was done on D6; that's what we run at the Open Source Club, and I believe you're going to discontinue support for D5 in the near future.

l@va’s picture

FileSize
27.23 KB

1. Coder is now returning zero messages. Could you please provide me with some examples of where I failed to adhere to proper standards, if they remain?
2. It wasn't there to encourage hacking the code (thus not found in the documentation anywhere). Changing the play icon seemed a fairly unlikely scenario to me, but I could stick it in the preferences instead. You'd only want to change it if you were swapping out the included play.png file, and when you saw it off-center, that's where you'd change it. I spent a while just looking over the code figuring out what it did, so I wanted to make understanding that easy, but now that you mention it I could see both ways. I'll probably add a resizeable play button as a feature eventually just because you mentioned it.
3. Updated per your specifications.
4. All license files have been removed.
5. I'll just take dump() out. It was there in the original version, and I didn't know what it did, but figured I should leave it alone in case something in Drupal depended on it.
6. Could you show me an example of this? I've never seen anything like that happen before. It's not happening on our live site, where the exact same code you're looking at is running (minimal modules), it's not doing that in any of our testing sandboxes, and it's not doing that on eff.org, which is running a slightly older version of this (no metacafe.com). I also routinely look at the page source when debugging, so I'd probably notice something like that happening. This isn't normally how I address errors, and I don't mean offense to suggest it, but are you sure it's not something goofy with your environment?

l@va’s picture

Status: Needs work » Needs review

Just realized by looking at other people's requests that I should mark this as "needs review" myself. Also, is there a reason the name got changed to "http://www.wholesalenflstore.com/"? I didn't change it myself, and the page says it was last updated 3 hours, 40 minutes ago. [Changed back to what I think it was on submission.]

l@va’s picture

FileSize
27.26 KB

wadmiraal: Having read through your coding standards and searched through my code at each paragraph of reading said standards, I believe I have updated my code (new version attached) to be strictly 100% compliant with your coding standards wherever possible. There is one isolated case where it would rename a variable if I concatenate it within the string, but I suspect that's not a major problem for you. My .info file is updated per your specifications, the license file is removed, and the dump() function is deleted. Aside from your last point, which I cannot seem to duplicate no matter what I do, I believe I have met all of your criteria. Can I please have another review?

If you are still getting the .module dumped into all your pages, could you tell me what other modules you have installed? At this point, aside from working on your optional suggestion (which will come in time) I don't know what else to do.

wadmiraal’s picture

Status: Needs review » Reviewed & tested by the community

Good job :-).

You should remove:

/*************************
 * Intended for Drupal 6 *
 *************************/

from the start of the .module file.

Add:

// $Id$

at the very top of your .module file for CVS.

The problem did not occur again. Your module is working as advertised (as far as I can see). I say this is RTBC :-).

apaderno’s picture

Status: Reviewed & tested by the community » Needs work

This is a partial review.

  1. Menu descriptions and titles (as well as schema descriptions) should not be passed to t().
  2. The first argument of t() is a literal string; any dynamic string (including the concatenation of strings, such as 'first string' . 'second string' and "first string $second_string") would not allow the argument to be put in the translation template.
    The reason is that the translation template file is created by a script that extracts the string passed to t() by scanning the module files, which are analyzed as plain text. The script is then not able to get the value of a PHP variable, or any dynamic value passed to t().
  3. Avoid escaping the string delimiter inside a string, especially if the string is passed to t(); avoid using l() with t() as in t('See the !settings-page.', array('!settings-page' => l(t('settings page'), 'admin/module/settings'))).

    As reported in the documentation:

    Here is an example of incorrect usage of t():

      $output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
    

    Here is an example of t() used correctly:

      $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';
    
  4.       $output .= '<p><b>' . t('Note:') . '</b> ' . t('Due to a problem with the way that Microsoft Internet Explorer renders javascript and Flash, affected MyTube videos will not properly display in Internet Explorer.') . '</p><h3>' . t('Input format guidelines:') . '</h3>';
    

    <b> should not be present in XHTML output, which is the Drupal output.
    Avoid concatenating translated strings, but pass the full sentence to t(). HTML tags should be avoided inside the strings to translate, but the code should not become as the following one:

      $output = '<strong>' . t('Hello') . '</strong>' . t(',') . '<em>' . t('my friend') . '</em>';
    

    It is JavaScript, not javascript.

  5.   $form['mytube_text'] = array(
        '#type' => 'textfield',
        '#title' => t("'Privacy info' text"),
        '#default_value' => variable_get('mytube_text', ''),
        '#description' => t("Optional: Customize privacy info description below thumbnail. Use !embed and !domain for embed link and top-level domain, respectively."),
      );
    

    As !embed< could be confused with a t()-placeholder, it should be better to use <code>!</code><code>embed</code>.

  6.     $output .= t(" Note, if a thumbnail would normally be downloaded automatically, such as for youtube.com, you cannot customize it. This is done to prevent deliberately misleading video thumbnails and cannot be overridden by an administrator.") . "</p>";
        $output .= "<div class=\"mytube\" style=\"position:relative;width:320px;\"><img width=\"320\" height=\"240\" src=\"" . url(drupal_get_path("module", "mytube") . "/default.gif") . "\" class=\"mytubethumb\"><img style=\"top: 90px; left: 130px;position:absolute;width:60px;height:65px;z-index:5;\" class=\"mytubeplay\" src=\"" . url(drupal_get_path("module", "mytube")."/play.png") . "\">";
    $output .= "<div class=\"mytubetext\" style=\"font-size:10px;line-height:10px;\">" . l(t("Privacy info."), $privacy_url) . "$privacy_text</div></div>";
        $output .= "<p>".t("Depending on the webmaster's settings, you may be able to set a custom width and height in the same way. If no recognizable width or height is provided in the embed code, the system defaults") . " <u>" . t("will override") . "</u> " . t("whatever was in the original embed code.") . "</p>";
        $output .= t("Example") . ": &amp;lt;object &lt;b style=\&quot;color:red;\&quot;&gt;width&lt;/b&gt;=&amp;quot;&lt;u&gt;640&lt;/u&gt;&amp;quot; &lt;b style=\&quot;color:blue;\&quot;&gt;height&lt;/b&gt;=&amp;quot;&lt;u&gt;385&lt;/u&gt;&amp;quot; &lt;b style=\&quot;color:green;\&quot;&gt;thumb&lt;/b&gt;=&amp;quot;&quot; . l(file_directory_path() . &quot;/mytube/yt_VL3cetAodLc.jpg&quot;,drupal_get_path(&quot;module&quot;, &quot;mytube&quot;) . &quot;/yt_example.jpg&quot;) . &quot;&amp;quot; style=&amp;quot;text-decoration:underline;&amp;quot;&gt;&quot; . file_directory_path() . &quot;/mytube/yt_VL3cetAodLc.jpg&lt;/a&gt;&amp;quot;&amp;gt;&amp;lt;param name=&amp;quot;movie&amp;quot; value=&amp;quot;http://www.youtube-nocookie.com/v/VL3cetAodLc&amp;hl=en_US&amp;fs=1&amp;&amp;quot;&amp;gt;&amp;lt;/param&amp;gt;&amp;lt;param name=&amp;quot;allowFullScreen&amp;quot; value=&amp;quot;true&amp;quot;&amp;gt;&amp;lt;/param&amp;gt;&amp;lt;param name=&amp;quot;allowscriptaccess&amp;quot; value=&amp;quot;always&amp;quot;&amp;gt;&amp;lt;/param&amp;gt;&amp;lt;embed src=&amp;quot;http://www.youtube-nocookie.com/v/VL3cetAodLc&amp;hl=en_US&amp;fs=1&amp;&amp;quot; type=&amp;quot;application/x-shockwave-flash&amp;quot; allowscriptaccess=&amp;quot;always&amp;quot; allowfullscreen=&amp;quot;true&amp;quot; width=&amp;quot;640&amp;quot; height=&amp;quot;385&amp;quot;&amp;gt;&amp;lt;/embed&amp;gt;&amp;lt;/object&amp;gt;";
        $output .= "<p>" . t("All affected embedded objects will be appended with the caption shown above, alerting users that by clicking the thumbnail they are consenting to have remote content served and acknowledging the consequential privacy issues. The caption has a link pointing viewers to more information, and may also display where the content will be served from.") . "</p>";
        return $output;
    

    CSS styles should be applied through HTML classes, or IDs.

  7. /**
     * Implementation of hook_nodeapi().
     */
    function mytube_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL){
      if ($op == "view") {
        // filter the body text
        $js .= "function ieMessage()\n";
        if (variable_get("mytube_ie_message","") != "")
          $js .= "  {alert(\"".variable_get("mytube_ie_message","This video will not load in Internet Explorer.") . "\");}";
        else
          $js .= "  {return false; // Administrator has opted not to display messages to Internet Explorer\n}";
        drupal_add_js($js, "inline");
      }
    }
    

    JavaScript code should go in a separated file. It is possible to translate the strings that are output from JavaScript code through Drupal.t().

  8. Global variables should be named like $_mytube_index.
  9. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how constants should be written; how Drupal variables, PHP variables and functions, and constants defined from the module should be written.
  10. The module doesn't implement hook_uninstall() to remove the Drupal variables it defines.
wadmiraal’s picture

@kiamlaluno
Wow...sorry about that...I missed a lot it seems.

l@va’s picture

FileSize
27.29 KB

1. This is now fixed. I was not sure if by schema descriptions you meant descriptions below the settings, so those are still using t() for now. If you want the #description parts of $form in mytube_admin_settings() to not use t() either, let me know.

2. Fixed to the best of my knowledge throughout the module.

3. Also fixed.

4. All instances of <b> tags were replaced with <strong>. I could not reasonably completely remove all HTML tags and completely avoid concatenating t() strings, but I did my best to accomplish the latter.

5. Fixed.

6. There was at least one case where, in testing, the class/id CSS was not applied for reasons unknown to me. Furthermore, the CSS is defined on either a per-video or per-setting basis. In this case, the inline CSS remains. Here is a snippet:

    $_mytube_index++;
    $out  = "<div class='mytube' style='width:$width" . "px;'>";
    $out .= "<div class='mytubetrigger' id='mytube$_mytube_index'>";
    $out .= "<img width='$width' height='$height' class='mytubethumb' src='";
    $out .= $thumb . "' alt='mytubethumb' />";

Also, for the section of escaped embed code you quoted, it would seem (in my opinion) redundant to define a separate ID for each color where I'm highlighting sections of the code and add those into a CSS file, just for a single instance where they appear, so I still have it in CSS. If you wish, I will put them into the CSS file and remove the inline CSS, but inline CSS seems most appropriate here.

7. What I was looking here for was not a translation, but configurability. Earlier on in testing, Internet Explorer was not loading any kind of object (Flash or not) that was not preloaded with the page. It didn't matter whether a .swf was loaded in a popup, whether I used ELEMENT.innerHTML, or whatever; unless it automatically loaded when the page loaded the objects would not load. This has not been the issue lately, and I cannot reproduce the issue anymore, but I had the setting there just in case. What would happen here is that if an administrator found their videos weren't loading in Internet Explorer, they could explain the problem to IE users in an alert dialog, instead of just inexplicably failing, and let the user know they can still load videos in another browser of their choice. Since I'm no longer able to reproduce the issue, and since I have no idea how to make this setting both configurable and appear in a static js file, I removed that feature.

8. All global variable names are now prepended with "_mytube", or "_" if they already started with "mytube".

9. All the constants I can think of are now in all caps. If any are not, then let me know because I missed it. I think functions and (non-constant) variables were already compliant.

10. Added a .install file with an implementation of hook_uninstall(). Do I also need to delete the sites/default/files/mytube folder where thumbnails are stored? I am not sure if unlink would work on a folder.

l@va’s picture

Status: Needs work » Needs review
apaderno’s picture

By schema descriptions I mean the ones used in the database schema, the one declared from the hook_schema() hook.

l@va’s picture

Ah, thanks for clearing that up. MyTube doesn't implement hook_schema() because it never accesses the database (except in variable_get() and now variable_del()); it has no need to use the database as an input filter. Do I still need to add an implementation of hook_schema() for some reason?

apaderno’s picture

Do I still need to add an implementation of hook_schema() for some reason?

No; a filter doesn't normally use databases.

l@va’s picture

So, am I missing anything?

l@va’s picture

Anybody going to review this?

l@va’s picture

Component: Miscellaneous » co-maintainer application

Still waiting...

apaderno’s picture

Component: co-maintainer application » new project application
sirkitree’s picture

Status: Needs review » Reviewed & tested by the community

This has gone back and forth enough to demonstrate that the author is a responsive and responsible contributor IMHO. Please grant a CVS account based on this fact - not deny him any longer based on the fact that the code is not perfect. He's certainly demonstrated that he is willing to improve and this should be done in his project's issue queue, not in this thread any longer. Further deliberation only serves to frustrate and confound new contributors. Marking RTBC.

siliconmeadow’s picture

Status: Reviewed & tested by the community » Fixed
apaderno’s picture

Status: Fixed » Reviewed & tested by the community

Nothing has been fixed, as the CVS application has not been approved, yet.

siliconmeadow’s picture

Nothing has been fixed, as the CVS application has not been approved, yet.

Sorry - that was a cheeky attempt for me to see if the application would be approved automatically if someone other than the applicant changed the status to fixed. I was feeling sorry for him being put through this purgatory and in agreement with sirkitree's comment.

siliconmeadow’s picture

Others have scoured through your code technically, and your willingness to address each and every point raised shows good attention to detail. This review will be primarily about the way the module is presented to the end user. It's much more subjective and open to interpretation, of course, but there are plenty of guidelines for documenting and "getting the tone right" which you can find here on d.o, although you might have to dig around a bit. The links mentioned will drop you into the book where you can use the navigation on the right-hand side to gain further understanding.

Filter module as a dependency - no need to specify as it's a required core module. Not a show stopper though.

Readme file - Have a look at the first recommended guideline here:

http://drupal.org/node/161085

which has a link here:

http://drupal.org/node/447604

I needed a bit of a refresher of what I was evaluating and your README.txt file was the first place I went to this morning. I was on a train and my internet access was a bit patchy. You mention three URLs for more info, but a module's README.txt should give the end-user enough detail to get started. It doesn't say anything about installing (even if it's the standard way), permissions, that it's a filter module, nor what the module's purpose is.

I don't know what more info I'm going to get by going to the links you mention in the README.txt file. Should I go to them in the order they appear in the URL? Put yourself in the user's shoes - they shouldn't have to make too many decisions at this stage. Let them know what they can expect it to do. Also, what it doesn't do - perhaps some people attempted to use it for the wrong purpose and after some time, discovered it wouldn't work for the purpose they had hoped. You could probably copy and paste some of what you'd posted in the beginning of your application to get started.

The help page appears to provide much of the detail you should have in the README.txt file. Perhaps some of this, too, could be in the README.txt file?

Proofreading the help as displayed

Last sentence of first paragraph of help should read:

...those who are not technically-inclined...

Third paragraph - should probably be "customizable" instead of "customizability"?

The first paragraph after "Input format guidelines" seems to repeat the first sentence of the third paragraph, but much more awkwardly. Could it be deleted? Maybe the first paragraph of "Input format guidelines" could be:

Video hosting websites (such as YouTube), often allow visitors to "share" or "embed" the video they have just watched by offering a snippet of HTML the visitor can post on your site. If you (or contributors to your site) post such videos on your site, this module eleviates the privacy concerns mentioned above. By replacing the tags they've been provided with clickable thumbnails, the Flash content is prevented from automatically loading. The thumbnails will be replaced by the original HTML snippet when clicked on by a viewer and therefore provide the video as originally intended.

The previous paragraph I suggested is just an idea, and upon reflection, it seems to repeat a lot of information from your previous paragraphs. Perhaps you and Tim Jones could collaborate on revising the what, why and how text in both the README.txt and the inline help for clarity and conciseness?

You might find it helpful to follow some of the stylistic guidelines the Docs team has outlined:

http://drupal.org/node/700542

In the same section (the "Contribute to documentation" of the "Getting involved guide") - is an example of the care and attention the community has put into accuracy of terminology. I notice in #12 that kiamlaluno picked up on your case insensitivity of the word JavaScript.

I hope this helps and doesn't cause a huge amount of work for you, especially since it seems you've jumped through every other hoop by now.

l@va’s picture

FileSize
28.84 KB

Filter module as a dependency - no need to specify as it's a required core module. Not a show stopper though.

Fixed. I added it because it was specifically required; there is a security vulnerability it mitigates by calling HTML Corrector's filter. This is more an issue with Drupal 5, since HTML Corrector is not included by default.

Added heavy documentation to the README.txt file and added an INSTALL.txt file. Also, I added an FAQ section to README.txt and heavily emphasized installing the PHP5-CURL library in the INSTALL.txt.

Last sentence of first paragraph of help should read:

...those who are not technically-inclined...

Fixed.

Third paragraph - should probably be "customizable" instead of "customizability"?

Fixed.

The first paragraph after "Input format guidelines" seems to repeat the first sentence of the third paragraph, but much more awkwardly. Could it be deleted? Maybe the first paragraph of "Input format guidelines" could be:

Video hosting websites (such as YouTube), often allow visitors to "share" or "embed" the video they have just watched by offering a snippet of HTML the visitor can post on your site. If you (or contributors to your site) post such videos on your site, this module eleviates the privacy concerns mentioned above. By replacing the tags they've been provided with clickable thumbnails, the Flash content is prevented from automatically loading. The thumbnails will be replaced by the original HTML snippet when clicked on by a viewer and therefore provide the video as originally intended.

The previous paragraph I suggested is just an idea, and upon reflection, it seems to repeat a lot of information from your previous paragraphs. Perhaps you and Tim Jones could collaborate on revising the what, why and how text in both the README.txt and the inline help for clarity and conciseness?

Note: The portion below "Input format guidelines" is a preview of what the regular user will see when they click on the "More information about formatting options". The hook_help displays the output of hook_filter_tips(0, 0, TRUE) to be helpful, so the administrator can see what the user sees and what exactly the filter does. I cannot explicitly add a link to the filter tips from the help page, and this is why you have some redundancy (some of the details are explained to users).

I notice in #12 that kiamlaluno picked up on your case insensitivity of the word JavaScript

In that case, I simply didn't know. What you found were typos and grammatical errors in my writing, and I think they're all fixed in this version.

By the way, I do appreciate your above attempt to get this approved, as well as your input. This is getting old, and I'm getting tired of waiting around here without even being told why the application is not approved. I suspect there are at least a few who don't quite appreciate it as much, if you know what I mean...

I am not sure what else is needed here to get MyTube hosted on drupal.org.

siliconmeadow’s picture

Indeed it's frustrating when others don't act as quickly as you'd wish and don't explain why. We all make mistakes and have communication breakdowns though, so please bear with us. I'm certain there is nothing personal going on. Drupal does appear to be growing exponentially and there are over 7k modules now. I suspect a good number are poorly maintained, if at all. And there are hundreds of duplicates. There are a number of balancing acts going on simultaneously and being orchestrated by a diverse population. kiamlaluno and the rest of us have a duty in the approval process to ensure CVS account holders have the skill to do produce quality code and documentation. Of course none of us can know everything, so we hope you know where to find the stuff you need on d.o, and show the initiative to find it.

Think of the application process as a learning exercise and hopefully a fun one at that. There is a wealth of information here to help you grasp the standards expected. Conversely, there is quite a bit of out-of-date information as well as documentation and code which does not reflect the standards the community aspires to. It appears as hypocrisy, but hopefully benign. I do encourage you, in any case, to immerse yourself in the documentation so you get a feel for what this place is about. You do admit that your new to Drupal development, afterall.

It was a glaring oversight that no one picked up on the lack of mandatory information in the README.txt file. I believe that omission on it's own has stopped applications.

The grammar issue is something not to gloss over. A significant number of Drupal users do not have English as their mother tongue.

Anyway, I guess it's time for me to stop pontificating.

"Technically-incline" and "customizability" have crept into the README.txt. Doh!

wadmiraal’s picture

FileSize
527.87 KB
649.69 KB

Hi lavagolemking,

Sorry I didn't get back to this review, was very busy. I won't go through the copywriting of the README.txt file.

So I downloaded the new version, activated it and... It outputs all the content from your .module file before the HTML (similar to #5). This is a fresh install of Drupal 6.20 we're talking about (Garland theme). Tried it on another install (Drupal 6.20 as well), and the same problem occurs. Now this is with different settings then #5, and not on the same computer (all this is tested locally). I tried deactivating every module except the core "Menu" module...and it still happens. And I've never encountered this with other modules, so it definitely has something to do with yours.

Can anyone else try to install this module and see what happens ?

I don't know why this only happens to me, but admit that it is weird.

l@va’s picture

FileSize
163.24 KB
29.99 KB

"Technically-incline" and "customizability" have crept into the README.txt. Doh!

Fixed. I guess I made my edits a little out of order.

It outputs all the content from your .module file before the HTML (similar to #5).

I still cannot reproduce this issue on a clean installation, just downloaded from drupal.org and with the database just created. Screenshot attached. A couple things though:

  1. Did you check the permissions to all your files (including MyTube's)? I can't seem to reproduce your issue with incorrect permissions, but historically I've only gotten that type of error in web development when either there was something that couldn't be executed or I didn't have PHP interpreting set up properly.
  2. Are you using Drupal 6 or 7 for testing? This is a Drupal 6 version, and a base install of Drupal 6 will not have the administrative overlay I see at the top of your screenshots.

I did find and fix an unrelated issue though. At some point in these changes, and I have no idea when but I know I ran into and fixed this before, MyTube stopped displaying thumbnails for sites with Clean URLs disabled. I fixed it to again strip the "?q=" from the IMG SRC attribute.

wadmiraal’s picture

FOUND IT !!

I was taking my shower this morning and suddenly it hit me: PHP opening tags ! You used <? opening tags, but you should use <?php, as not all servers support short tags.

siliconmeadow’s picture

Can anyone else try to install this module and see what happens ?

I've tried it on two sites so far and can't duplicate the problem.

  • Vanilla 6.20 aegir installation
    • Ubuntu Maverick
    • PHP Version 5.2.10-2ubuntu6.7
    • Apache/2.2.16 (Ubuntu)
    • MySQL 5.1.49
    • PHP Memory Limit 256M
    • with clean urls on and off
  • Production server (dedicated server at Rackspace - RHEL 5)
    • Drupal 6.19 (doh!)
    • PHP 5.2.16
    • Apache/2.2.3 (Red Hat)
    • MySQL 5.0.77
    • PHP Mem Limit 128M
    • with clean urls on and off
siliconmeadow’s picture

FOUND IT !!

I was taking my shower this morning and suddenly it hit me: PHP opening tags ! You used <? opening tags, but you should use <?php, as not all servers support short tags.

Ah - good find!

siliconmeadow’s picture

Are you using Drupal 6 or 7 for testing? This is a Drupal 6 version, and a base install of Drupal 6 will not have the administrative overlay I see at the top of your screenshots.

Admin menu. You'll love it: http://drupal.org/project/admin_menu

siliconmeadow’s picture

A couple of things I found in use:

  • warning: curl_setopt() [function.curl-setopt]: CURLOPT_FOLLOWLOCATION cannot be activated when safe_mode is enabled or an open_basedir is set in /path/to/mytube.module on line 754.
    safe_mode is off, but open_basedir is set. A minor problem?
  • I can't get the iframe version of YouTube working. The default image loads and when you click it, the image goes away, the gap where it was is still there, but it remains empty. The legacy embed version works.
apaderno’s picture

Status: Reviewed & tested by the community » Needs work

I think that the coding standards also report to not use the short tags.

l@va’s picture

Status: Needs work » Needs review
FileSize
40.49 KB

You used <? opening tags, but you should use <?php, as not all servers support short tags.

I thought that's what I was supposed to use, but I guess I misunderstood. The tags are now back to <?php.

warning: curl_setopt() [function.curl-setopt]: CURLOPT_FOLLOWLOCATION cannot be activated when safe_mode is enabled or an open_basedir is set in /path/to/mytube.module on line 754.
safe_mode is off, but open_basedir is set. A minor problem?

Depends. Are you still getting thumbnails for YouTube/Vimeo/MetaCafé embeds? If so, nothing is wrong. CURL is only used for downloading thumbnails, so if it gets called and you don't get a thumbnail from it, then you have a major problem (usually you'll get the black, rounded default.gif). If you are getting the thumbnails, then it's just alerting you that something isn't ideal between your setup and the code being run.

Unfortunately, my expertise related to CURL is a little limited here, and I don't know what open_basedir is for. This portion of the code is more or less copied/adapted from what Tim Jones had, but with the exception of my next point, it sounds like things are working for you. I thought the particular line of code being mentioned was required before starting a CURL session, but if it causes a problem other than warnings let me know.

I can't get the iframe version of YouTube working.

This one is a serious problem. There is a known and documented problem with YouTube playlists; neither Tim nor I had taken them into account when working on this, and YouTube embeds were always re-written in a very specific format when adding the custom YouTube parameters. It depends on the embed URL being of format youtube.com/v/ Naturally, this broke YouTube playlists (which use /p/), so I added a TODO tag. It seems I was unaware they were using a new embed code (/embed), and furthermore this new embed didn't make the first parameter start with an & instead of ? (unlike the legacy embeds). The new iframes should now be accounted for, and thumbnails should work.

The default image loads

This is very surprising to me. There is no code to handle this new format, of which I am just now becoming aware. MyTube will try to find the youtube/v/ portion in the URL, and take the video ID from right after it to find the thumbnail. Since the /v/ string doesn't exist, it will fail, and fall back to the default.gif instead. Are you sure you weren't just talking about that black background with rounded corners? If so, that problem should be fixed now, and when the code is run again (clear Drupal's cache) the thumbnail should be downloaded and displayed.

siliconmeadow’s picture

I thought that's what I was supposed to use, but I guess I misunderstood. The tags are now back to <?php.

It's in the coding standards. You've missed out the the full <?php opening tag in the .install file:

aegir@slartibartfast:~/platforms/drupal-6.20/sites/mytube.com/modules/mytube$ grep -rn "<?" .
./mytube.install:1:<?
./mytube.module:1:<?php
./mytube.module~:1:<?php
Binary file ./yt_example.jpg matches
aegir@slartibartfast:~/platforms/drupal-6.20/sites/mytube.com/modules/mytube$
I don't know what open_basedir is for.

http://www.php.net/manual/en/ini.core.php#ini.open-basedir

It seems I was unaware they were using a new embed code (/embed), and furthermore this new embed didn't make the first parameter start with an & instead of ? (unlike the legacy embeds). The new iframes should now be accounted for, and thumbnails should work.

I've tested a few videos from YouTube and they seem to be working fine now.

Are you sure you weren't just talking about that black background with rounded corners? If so, that problem should be fixed now, and when the code is run again (clear Drupal's cache) the thumbnail should be downloaded and displayed.

Yes, sorry - it was the black background with rounded corners that appeared. And now, as you say, this version of the code displays the thumbnails directly from YouTube. And remind me to not piss off the person who has thumbs that large.

I also took the opportunity to try each of the other site that the README.txt file are supported, with the exception of myspace, as I don't have an account, nor could I get to mtvnservices.com to load.

I applaud your perseverance! I highly recommend you start immersing yourself in the Drupal documentation - particularly coding standards, CVS account holder docs so you're prepared for when your account is approved.

l@va’s picture

FileSize
29.91 KB

Alright,

  1. The other <?php is added.
  2. If I understand open_basedir correctly now (which I might not), then it restricts which files can or cannot be opened by the PHP interpretor. This should not be a problem for MyTube unless PHP cannot open files in the location specified by file_directory_temp() or file_directory_path(). MyTube will download a file (or possibly multiple files, as in the case for Vimeo) to the temp folder, then when all is said and done and it has a thumbnail, it moves the file over to sites/default/files/mytube/ and returns a path to it. If it fails somewhere, it returns a path to the default.gif. This means that if you are denying CURL the ability to open folders in one of these locations, then you'll get nothing but default.gif. It sounds like you might have changed this setting, since the error indicates CURL was restricted to MyTube's own folder.
  3. And remind me to not piss off the person who has thumbs that large.

    Could you clarify this for me? Large images (width/height) should not affect anything because MyTube resizes them to the same width/height of the embed. The thumbnails are generated by the video sharing sites, and generally pretty small in terms of file size so you shouldn't normally run into problems even if you download a lot of them.

  4. with the exception of myspace

    No need for an account on MySpace; just check Google Video with search terms site:myspace.com. For your reference - http://opensource.osu.edu:7280/node/402

  5. Do you have another review, or do you believe it's ready to be committed?
siliconmeadow’s picture

Status: Needs review » Reviewed & tested by the community

The other <?php is added.

Excellent - thanks :-)

If I understand open_basedir correctly now

Sounds like you do. Plesk users in particular will likely see open_basedir warnings, due to its effectiveness in restricting where ill-informed end users install php files for execution. You can imagine a web site novice downloading a freebie set of php scripts and being completely unaware of the malicious purpose of the code. If you get a lot of open_basedir items in your issue queue, let me know and I'll write up some instructions for what they (or their hosting company) need to do to make a more friendly vhost.conf file. The vhost.conf file is used to tweak non-standard apache conf features for vhosts in Plesk on a per-domain basis.

Could you clarify this for me?

It was a completely lame attempt at humour. Please ignore the comment, and sorry I diverted your attention with it.

No need for an account on MySpace; just check Google Video with search terms site:myspace.com. For your reference - http://opensource.osu.edu:7280/node/402

Thanks.

Do you have another review, or do you believe it's ready to be committed?

I can't see any reason for your account to not be approved. I hope someone with the power to do so will approve it rather sharpish. Good job, and once again, thanks for your perseverance.

siliconmeadow’s picture

Hi Brian,

I hope you are still interested in having your module hosted on d.o. I speculated in the past, and still believe, the delay is due to the "Great git migration". You might want to read #1065558: How to handle migration from CVS Applications to Project Applications (new Git workflow) in the infrastructure queue.

Cheers,

Richard

l@va’s picture

Still interested, and still watching.

Skimmed over linked document, and I'm not quite sure what this means for me, other than more waiting. Do I need to run this in some kind of CVS/GIT sandbox before it's published? Do I need to re-do the application as a GIT application? Do I need to re-apply for a GIT account, already having applied here?

l@va’s picture

I have created a sandbox project for this module:
http://drupal.org/sandbox/lava/1074760

I have created a new issue at http://drupal.org/project/projectapplications based on the new instructions about applying for permission to create full projects.
Link to the issue: http://drupal.org/node/1074792

apaderno’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
apaderno’s picture

Issue summary: View changes
Related issues: +#1074792: MyTube