EMBridge

This looks really good. The only major issues I see are:

• The README.txt file is blank, it should contain a description of the module and instructions on how to install & use it.
• SWFUpload uses the MIT license so cannot be bundled for download with the module on d.o. You might instead bundle a Drush script to download the necessary components into the sites/all/libraries directory and load them from there.

A few additional minor issues:

• Many of the comments are missing the second * on the first line, i.e. they have "/*" instead of "/**".
• There are a couple of tabs remaining in some of the files - only a few, but they need to be replaced with two spaces.
• Some of the indenting is slightly off, e.g.:
  

      'embridge_formatter_plain' => array(
        'arguments' => array('element' => NULL),
    ),
  

  should be

      'embridge_formatter_plain' => array(
        'arguments' => array('element' => NULL),
      ),
  

  and

    $schema['entermedia'] = array(
    'description' => 'Stores entermedia asset related data based on asset id.',
  

  should be

    $schema['entermedia'] = array(
      'description' => 'Stores entermedia asset related data based on asset id.',
  

• There's an unnecessary Thumbs.db file in the 'images' directory.
• You don't need to include $Id$ in your files anymore, those are a hold-over from our CVS days and are no longer necessary now that d.o uses git.
• There are a lot of empty validator functions, you probably ought to add some logic to them.

• git release branch missing, see http://drupal.org/node/1015226
• lines in README.txt should not exceed 80 characters
• info file: remove "version", this is added by drupal.org packaging automatically
• hook_init(): This hook is called on literally every page request, even if your module has nothing to do with that page. Can't you add your CSS and Javscript elsewhere?
• "// 2GB in bytes" comments should be on a separate line
• "// Validate the file size (Warning: the largest files supported by this code is 2GB)" comments should not exceed 80 characters
• HandleError(): all functions need to prefixed with your module name to avoid name clashes. And they should start lower cased.
• @param doc block: the description needs to be indeted 2 spaces, see http://drupal.org/node/1354#functions
• you are using PHP cURL, so you should add a hook_requirements() to check if the PHP cURL module is installed.
• your module file is quite long, you should move all page callbacks to a separate .inc files (e.g. to the .admin.inc file)

Review of the 7.x branch:

• info file: wrong line endings ('\r' but should be unix style '\n')
• info file: It's only necessary to declare files[] if they declare a class or interface.
• "Implementation of hook_requirements()." should be "Implements hook_requirements().". Also for the other hook doc blocks.
• embridge_install(): empty function, so remove it.
• embridge_permission(): indentation error, run coder to check your code style http://drupal.org/project/coder
• "if ($scripts) $output .= $scripts;": if statements must be wrapped in "{}", even if thex are just one liners. Also in many other places.
• "// execute the curl command": all comments should start capitalized and end with a "."
• "//All catalog fields.": there should be a space after "//" for comments.
• "variable_get('entermedia_server_url'": your variables should all be prefixed with your module's name, i.e. "embridge" instead of "entermedia". Avoids name clashes.
• embridge_bulk_upload(): you should move your javascript code to a file. You should use drupal_add_js() to add javascript to a page. You can pass settings to the script with that function.
• "watchdog('entermediafield', ...": you should use your module name in there, i.e. "embridge"
• "$module_path = drupal_get_path('module', 'entermedia');": entermedia again, please search all your files and replace that with embridge.
• "$head = '
  ';": use drupal_add_css() to add stylesheets to your page.
• Remove all old CVS $Id tags from all files, e.g. in html--entermedia.tpl.php
• entermediafield_usage_add(): remove all empty functions
• entermediaassetfield_entermediaassetfield_download(): the whole function is commented out, so remove it.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 6.x-1.x branch:
Review of the 7.x-1.x branch:
(There is still really much to do. I attached the results instead of pasting.)

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Note:
There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.

@joebachana has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

To do an application, as you how done, is to apply to the possibility to publish full projects on d.o, why else have you done the application? Read more here: http://drupal.org/node/1011698

We have committed an update to the Embridge module which includes an AJAX upload feature for the Entermedia webform component. Work on cleaning up the module's code and structure to address #10 has been started. Some of those changes are now in git, others will be committed soon.

We have addressed all items in 6.x and 7.x branches based on #10.

Added PAReview tag.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 6.x-1.x branch:
Review of the 7.x-1.x branch:

I have attached the results instead of pasting.

Changed status

valeriod, PAReview.sh script returns a lot of false positive results. Script doesn't work properly in two cases:
1. Webform hooks. This bug already reported (http://drupal.org/node/1368722).
2. Submodules. If module has submodule PAReview.sh ask to rename all submodule functions to be prefixed with module name. Submodule functions should be prefixed with submodule name instead.

Sorry for the confusion. I cloned the project and will do a manual review today, it's quite large.

furamag, I didn't find any obvious security issue but I have a question about file handling. Can you describe the mechanism that _embridge_upload_file() uses -- it looks like it's pulling -- and in general how files are handled.

Thanks

Valerio

I'm confused - who is the primary author of this module that is applying for the git vetted user role? joebachana opened this application and owns the sandbox, but has no commits. So IMO joebachana cannot get the git vetted user role.

furamag seems to have done some work, but not all reviews of other projects were done by him. Please decide who is the responsible developer of this module and state it clearly in the issue summary (also that it is not joebachana).

Reviews of other projects should only be listed for the person that runs the application and is the main developer. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Klausi,

I'd like to add on to Joe's response to help clarify.

EmBridge is currently being developed by multiple people, including justy, jgao, furamag, and myself (rlangille) - while joebachana is leading the project. As Joe said, we usually assign a specific person to handle commits and changes, but it is a group effort.

Also, the reviews that were performed were done by both furamag and myself and were intended to be applied to the EmBridge module.

Klausi,
rlangille is right. Embridge module developed by multiple people. All three reviews were done to be applied to the Embridge module.

valeriod,
Thank you for the feedback!

Here is the answer to your question.
To be able to use Embridge module you should have symbolic link directory on your server. That directory should link to the media store folder on your Entermedia server. When user upload file, _embridge_upload_file() function copy it to the symbolic link directory and send asset details (including file name which you just uploaded) to the Entermedia server.

You can have Entermedia and Drupal on the same server. In that case you don't need to create symbolic link directory. You just need to configure Embridge module to copy files to Entermedia media store directory.

Thanks furamag: I was looking for possible issues with file handling and wanted to make sure I understood the process correctly.

re: removing the "PAReview: review bonus" tag and requesting that 3 additional reviews be posted:

I think there is a much larger issue here but I'm not yet sure where the best place is to discuss it.

I don't want to divert this thread, but I'll bring the issue up here and later post a link to wherever the conversation can be most productive.

Klausi, I understand that you were confused about the connection between rlangille and furamag's reviews and this project application. As the person coordinating DPCI's effort to release this module, I take some of the blame for that. We should have been more clear so it did not look like Joe was trying to game the system.

I would like to restore the PAReview: review bonus tag based on the reality that this is a group effort, not the work of Joe alone.

The larger discussion is about how to properly indicate such collaborations in the future for any sandbox project.

Suggesting that we can't qualify for review bonus status until an additional 3 reviews are posted seems punitive and unfair.

My understanding is that there is a policy in-place on drupal.org that states that a single user account should never be used by multiple people in one company or organization. Accounts are for individuals only.

Starting from there, to say that the reviews here should have only be posted by Joe seems to ignore the reality that many modules are the creation of a collaboration -- some from people in one company; some from people that simply share a common desire.

As well, a policy that requires the reviews to come from only one person seems to me like it would promote use of shared accounts and a general dishonesty -- having people post reviews done by others and removing a certain level of transparency and accountability.

So there are two questions for you:
1: do you object to me adding the review bonus tag back to this application?
2: where do you think the right place is to have this larger discussion of policy?

As far as your request to be given one person to consider the point person for communication on this project, furamag will take on that role for now.

accidentally changed status in my last comment, changing back to 'needs review'

Based on the above explanations and comments, and the fact that over a week has gone by without an explicit objection to the idea of adding the PAreview bonus tag back to this application, I am adding that back.

I am still looking for the right place to have the larger discussion of process for the future in cases like this.

I just get really confused by all this. Who is responsible for the module, and who is applying for the git vetted user role?

Responsibility for this module's development is shared among myself, furamag, rlangille and joebachana. For now, furamag will be the primary contact and I guess should be the one that gets the git vetted user role.

I'm sorry this is so confusing, it's the first time we've tried to push something through the new module approval process, we'll do a better job next time.

I do not know the proper way to do this, but it sounds like this application should be closed and a new one should be opened by furamag and the project should be moved to that users sandbox. And PAReview: review bonus should only be tagged if furamag has done reviews.

Why is it the case that the reviews have to come from him? I performed the reviews with the intent of applying them to the project, as I am one of the people that works on it. I may not have committed code to the project on drupal.org, however I've spent a good deal of time debugging issues, testing, and working with the team on the project.

Also, no single person has been completely responsible for this project, as it has been a group effort to build it. I believe the project should stay under joebachana seeing as how he is the one who laid out the initial requirements, came up with the idea, directed the effort, and funded our time to work on and contribute to it.

Because that, like stated, it is individuals that apply in the project application queue. Not companies, our group of people. It is also individuals that get full project access. This is my five cents. Someone else of the reviewers could disagree. And to start with, your group have to agree on who is responsible to do the application.

I fully agree and understand that companies and groups do not get access, and nor should they. In this case, joebachana has been determined to be the responsible party on our end. It was Joe's idea, he is responsible for it, he follows it, and ensures that issues are resolved (whether by himself or another contributor), and therefore he should get project access.

It would be the same way if there were a few friends that decided to work on a project together - someone would need to be chosen to be responsible for it. The only difference being that in our case it happens to be that we all work together, and that some of us are given time to work on projects like this one.

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

• "define('ENTERMEDIA_CATALOG_CONFIG_PATH', '/config/catalogs');": all constants should be prefixed with your module name, i.e. "EMBRIDGE"
• "entermediaassetcomponent": stick to your main module's name, i.e. use something like "embridge_component". Same for entermediaassetfield.
• "'title' => 'Configure Entermedia Settings',": All user facing text should run through t() for translation. Please check all your strings.
• Are you sure that ypu need cURL and cannot use drupal_http_request()?

I have no problem that you are collaborating on this project, now that furamag is identified as project owner. I will also add that to the issue summary. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Updated review of other projects.

Thanks Klausi, will address these issues and resubmit.

Out of curiosity, was there an update to Drupal Code Sniffer? We've ran it a few times, and it didn't detect those issues. Just wondering if it's a bad configuration or a new update.

Yes, I update drupalcs regularly as I encounter issues that can be easily autodetected.

Fixes based on #38 are complete. We will make the same fixes for D6.

• All constants now prefixed with module name.
• Module names are changed.
• We are using t() for all user facing text now.
• CURL: When we created that module we didn't check HTTP request API. I think CURL can be replaced with drupal_http_request(). But I don't think that should stop us to push that module to the live project.

Fixes for D6 based on #38 are complete.

reviewing......

Automatic review found errors:
http://ventral.org/pareview/httpgitdrupalorgsandboxjbachana1209614git
see attached file
Manual review coming...

-------------------------------------------------------------------------------------------------------------------------
README.txt

In the installation section please put link to "DAM" server download and installation guide

-------------------------------------------------------------------------------------------------------------------------
on this page:
http://drupal.org/node/1211132#comment-5938034

this link is broken:
http://entermediasoftware.com/download.html

-------------------------------------------------------------------------------------------------------------------------
I configured entermedia server and set it to the test server, when I tested conection in admin/config/entermedia/settings
I got JS popoup message:
Connected to Entermedia server successfully.

Then I created intermedia filed, but when I tried to upload image I got:

Warning: mkdir(): Permission denied in _embridge_upload_file() (line 635 of/home/darko/workspace/drupal7/sites/all/modules/embridge/embridge.module).
Warning: copy(\\entermedia\assets/photo/2012/04/1335811398.jpg): failed to open stream: No such file or directory in _embridge_upload_file() (line 639 of/home/darko/workspace/drupal7/sites/all/modules/embridge/embridge.module).
Notice: Undefined index: asset in embridge_field_managed_file_save_upload() (line 248 of/home/darko/workspace/drupal7/sites/all/modules/embridge/embridge_field/embridge_field_file.inc).
Notice: Undefined index: @attributes in embridge_field_managed_file_save_upload() (line 267 of/home/darko/workspace/drupal7/sites/all/modules/embridge/embridge_field/embridge_field_file.inc).
Notice: Undefined index: hit in embridge_field_managed_file_save_upload() (line 269 of/home/darko/workspace/drupal7/sites/all/modules/embridge/embridge_field/embridge_field_file.inc).
Failed to get uploaded Entermedia asset - Entermedia service error.

Looks like it can not see the server , so either there is an error or the "connection successful" message was wrong.

-------------------------------------------------------------------------------------------------------------------------

With the release of EnterMedia 8, we have discovered some issues that result in EmBridge being unable to retrieve asset details from Entermedia. It can however successfully connect and upload assets to the system once EnterMedia is configured. In the near future we hope to update the module to fix these issues and also include a quick tutorial on how to setup and configure both EnterMedia and EmBridge.

I'm going to mark this as postponed until this is accomplished. Thank you for the review!

What can we do to get this turned into an actual project? We need to use this module but our company does not allow us to use sandbox code. We would like to help resolve the EMbridge issues for D7 but it doesn't look like there's a good way for us to submit patches / code at this point.

I created a patch to address the css property alphabetical ordering issues identified in #44.

Also, here is a patch that removes t() from hook_menu().

Finally note that the _webform_*() functions that PAReview found are actually valid webform_component hooks. See http://api.acquia.com/api/webform/webform_hooks.php/function/_webform_table_component/7.x-3.x, etc.

* Implements _webform_defaults_component().
function _webform_defaults_entermediaasset() {
* Implements _webform_theme_component().
function _webform_theme_entermediaasset() {
* Implements _webform_edit_component().
function _webform_edit_entermediaasset($component) {
* Implements _webform_render_component().
function _webform_render_entermediaasset($component, $value = NULL, $filter = TRUE) {
* Implements _webform_display_component().
function _webform_display_entermediaasset($component, $value, $format = 'html') {
* Implements _webform_delete_component().
function _webform_delete_entermediaasset($component, $value) {
* Implements _webform_analysis_component().
function _webform_analysis_entermediaasset($component, $sids = array()) {
* Implements _webform_table_component().
function _webform_table_entermediaasset($component, $value) {
* Implements _webform_csv_headers_component().
function _webform_csv_headers_entermediaasset($component, $export_options) {
* Implements _webform_csv_data_component().
function _webform_csv_data_entermediaasset($component, $export_options, $value) {

Hello slefevre1,
Thanks for your help! We'll review your patches and commit them to the Git soon. Right now we are trying to resolve issues with Embridge working with Entermedia 8.x.

I agree on this topic, there should be more discussion about how to handle collaboration by groups. My organization has a team of 8 Drupal developers. If one of them leaves the company and is the primary maintainer on the account, that makes future upgrades for us difficult without going through the whole abandoned module process.

For my team, we would like to have a primary company "account" that may not have git commits, but can add maintainers from my team as needed to facilitate their activities.

I don't want to highjack the topic either, if you can point me to a broader D.O link on this topic I'll be happy to post there.

@crimsondryad: Module maintainers can add other maintainers as they wish without any approval process.

With the completion of the Git migration, maintainers can immediately add co-maintainers to a project without going through an application process by granting Write to VCS privileges on the Maintainers tab. The co-maintainer will need to agree to the Drupal contributions repository usage agreement before they will be able to write to the repository.

From http://drupal.org/project/projectapplications

So you will only have to go through the project application process again if you want to publish new projects as a user that does not have the git vetted user role.

We created separate Embridge branch (7.x-2.x). Embridge code in that branch compatible with Entermedia 8 server. Right now we are working on Drupal coding standards fixes.

We have addressed all formatting issues and fixed all bugs which we found in our module. Now it works with Entermedia 8. We made our fixes in 7.x-2.x branch. 7.x-2.x is only branch which we support now.
_webform_*() functions that PAReview found are actually valid webform_component hooks.
We are working on FAQ for Embridge module. It should be ready soon.

Yay!

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-2.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. "'#value' => 'Cancel',": all user facing text should run through t() for translation. Also elsewhere. Same in embridge_bulk_upload_form() which has a lot of untranslated text.
2. _embridge_asset_insert(): use drupal_write_record() instead of db_insert() and you get schema validation for free.
3. I can't say much about XSS security, as I'm not sure if data is already sanitized or if it is user provided. Example: theme_embridge_asset_search_result() uses search result data, where properties are directly concatenated into HTML output. Please check that for example $v['@attributes']['id'] is safe already.

Otherwise looks RTBC to me.

Thanks Klausi!

Out of curiosity are you using the dev version of DrupalCS? We've been using the 7.x-1.0 version and haven't seen any of those issues. In this case is it best to stick with the dev branch?

Also, furamag is on vacation right now so I'll try to take care of these issues later this week.

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

Thanks for your contribution, furamag!

I updated your account to let you 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 get 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.

We want to use Entermedia as a DAM and to display it in Drupal.
We use EMBridge module for Drupal and in Configuration>Entermedia>Entermedia Settings we have
the field "Media store path" for witch we tried everything possible and even searched all the Google for a solution, but nothing useful appeared. If you can post a suggestion or a better documentation will be very helpfull. We have both Drupal 7 Flight and last version of Entermedia installed on the same server Centos 6.4 with Red Hat (64 bit). Because the path might be wrong the images uploaded with Drupal module are not shown in Entermedia.

The error in Drupal is:
Notice: Undefined index: asset in embridge_field_managed_file_save_upload() (line 248 of /var/www/html/sites/all/modules/embridge/embridge_field/embridge_field_file.inc).
Notice: Undefined index: @attributes in embridge_field_managed_file_save_upload() (line 267 of /var/www/html/sites/all/modules/embridge/embridge_field/embridge_field_file.inc).
Notice: Undefined index: hit in embridge_field_managed_file_save_upload() (line 269 of /var/www/html/sites/all/modules/embridge/embridge_field/embridge_field_file.inc).
Failed to get uploaded Entermedia asset - Entermedia service error.

This project application is closed. Please report any bugs to the issue queue of the project.

added furamag as application owner