CVS edit link for braahm

Hi,

I would like to apply for a cvs account to contribute modules back to Drupal. I've been using for a couple of years now and want to give back to the community.

More specifically, I've written a provider module for the Embedded Media Field (http://drupal.org/project/emfield) module. My module lets users embed CNN video's. It uses the API's set forth by the Embedded Media Field module.

Currently, there is no emfield provider module that let users embed CNN video's using the emfield module. So my module does not duplicate existing functionality.

The module was written for a client who needed this functionality and is currently running in production on her website.

I've reviewed it using coder so it should obey the Drupal coding standards.

Thank you for taking the time to review my application.

Toon

CommentFileSizeAuthor
#1 media_cnn.tar_.gz3.37 KBAnonymous (not verified)

Comments

Anonymous’s picture

Anonymous (not verified) commented
Component: Miscellaneous » miscellaneous
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new media_cnn.tar_.gz3.37 KB

Uploaded module to be contributed.
Changed status according to cvs account application instructions.

arianek’s picture

arianek commented
Status: Needs review » Postponed

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

  • The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
  • Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.
Anonymous’s picture

Anonymous (not verified) commented
Project: Drupal.org CVS applications » Drupal.org security advisory coverage applications
Status: Postponed » Needs review

Created a sandbox project on drupal.org/sandbox/braahm/1091036 as requested.

Thanks for taking the time to review my application.

meba’s picture

meba commented
Status: Needs review » Reviewed & tested by the community

You have few typo's in the module comments but otherwise I would say it's good to go. Thanks for fixing those typos.

Anonymous’s picture

Anonymous (not verified) commented

Thanks meba for reviewing my module. I cleaned up the spelling errors in the comments and interface strings as your requested. So we're good to go I guess.

lut4rp’s picture

lut4rp commented
Status: Reviewed & tested by the community » Needs work

Everything seems good to go, except the comments. Some of your comment blocks have 2 spaces after the * character, while some have a single space. As per convention, single space is the way to go.

Wrong way:

/**
 *  This is a comment.
 *  But with two spaces.
 */

Right way:

/**
 * This is a comment.
 * But with one space. YAY!
 */
lslinnet’s picture

lslinnet commented

Looks good for production but noticed a few code style and possible confusing parts:

Line 74: Always use a space between the dot and the concatenated parts to improve readability.

Line 129 to 133: could remove 2 spaces from each line to make it follow the usual way of indenting arrays

Line: 143 and 144: both lines declares the variable $output, i would recommend either to make line 144 concat or remove line 143

Line: 144 to 151: indentation should only consist of 2 spaces not 4.

Line 157: double line break after function name, only 1 is required.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

Thanks lut4rp and Islinnet for the specific feedback. I cleaned up the code and ran it again through coder. Should be fine now (knock on wood).

sreynen’s picture

sreynen commented
Component: miscellaneous » new project application
rteijeiro’s picture

rteijeiro commented
Title: braahm [braahm] » media_cnn
Status: Needs review » Needs work

Changed the issue title.

Please complete the README.txt file with the project installation, requirements and features.

The emvideo module no longer exists. Now it's named emfield. Please fix it in the module .info file.

Please, remove the commented code for debugging purposes (Line 144 of cnn.inc file)

Kind regards.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

Thanks rteijeiro for your feedback. I've updated the README file (resembles the meda_youtube more closely).

Added dependency for emfield module. I left the dependency for emvideo since it's a submodule of emfield (included in the emfield package) and it's necesary for the media_cnn module to function properly.

Finaly, removed the commented line.

Regards,

rteijeiro’s picture

rteijeiro commented
Status: Needs review » Needs work

Hi.

The module works like a charm but I am getting some errors like this:

user warning: Table 'testingrteijeiro.cache_emfield_xml' doesn't exist...

I don't know if it is my Drupal installation or the module itself but I get this error just when I create a new node with an emvideo field.

Kind regards.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Component: new project application » module
Status: Needs work » Closed (won't fix)

Closing, feel free to re-open if this was a mistake.