Duplicate project, too many releases, not adhering to Drupal's coding standards

Thanks for looking at this. My Drupal contributions are as a hobbyist at the moment, so it doesn't surprise me that I made some mistakes.

I sincerely apologize for the rapid releases. You can see some psudo-apologies in the release notes of the releases themselves. I made the initial release, and after further testing found out that the code was unusable except in the narrow circumstances that I was testing it. The first release fixed an issue where a site would whitescreen, not just on the bespin pages, but everywhere once enabled. It was due to my having mistakenly encoded the source file in UTF-8. The second release fixed a functionality bug which was fairly major. The third release made it work on webkit, which was needed to make this useful to more people than myself.

I had read in the maintainers guide that making a release means effectively that you think it is worth people spending their time to upgrade (meaning don't release every time you change something). I thought that since I had just made the first release and no one was likely to be using it yet, it was worth making the rapid release to fix a major bug. In retrospect, I should have left the release in -dev status for a couple of days to make sure that everything was working, but of course I thought each release was going to be the last one.

I tried to make the code compliant under the Drupal coding standards by documenting, using proper whitespace, etc... I missed the importance of the namespace section. I assume you are not referring to OO namespaces, but rather prefixing all of my functions with some consistent tag (e.g. bespin_embedded_function). Does it have to be bespin_embedded_ (which is rather clunky), or can it be something simpler like bespin_?

The only security issue that I am aware of is the ability to use the manual rule feature to insert javascript into the body of a page. I don't see any reasonable way to avoid this, so I tried to make it clear that the "administer bespin embedded" permission should only be given to trusted administrators on both the project page and the readme. If you know of a function to properly escape input for use as a javascript string, I want to fix this. A further security audit would be very helpful, as all I know is to pay attention to what happens to user input (xss) and I would learn from such an audit.

Last, but probably most important is the duplicate module issue. I looked for a module that does what bespin_embedded does before I started coding (it would save me work), and found the B4D module. I decided that it didn't count as a duplicate for a couple of reasons:

• The focus of the B4d module is collaborative editing. I don't care (for the scope of this module) about collaborative editing. I just want a bespin editor in an arbitrarily defined textarea.
• The B4D project is developing for and releasing on Drupal 7. Drupal 6 is what I am using, and I would guess the majority of people are and will be using for a while. At least for the Drupal 6 cycle, this is not a duplicate.
• The B4D project has no releases. This project is immediately useful, while B4D is still being developed.
• I have no idea what the state of the code is, whether it is usable, or just a basic structure. It is probably not possible to write a reasonably sized patch to get it to do what this module targets. I also do not want to try and hijack the B4D GSoC project with the priority to release a simple editor.

The B4D project looks really cool, and I hope the author completes it, continues to maintain it, and further contributes to Drupal. I intended once the B4D project had consumer quality releases, to reevaluate and if B4D overlapped to the point that a simple patch would offer all of the features in bespin embedded, I would attempt to offer such a patch and shut down bespin_embedded, pointing users to B4D.

Thank you for looking at this. I try to take maintainership seriously and greatly appreciate you taking the time to correct my mistakes.

I just committed a fix for the namespace issues.

I think I just committed a fix for the security issues. I would appreciate an audit of the code.

Regarding the frequency of releases - you don't need to create official releases at all, at this stage. If people want the bleeding edge (which is what the code is, to be honest), they can just download the -dev versions.

Sun, could I get a re-evaluation of this project? In CVS is what I believe to be a fix for both the security issue and the namespace issue. I would like to know whether I addressed them correctly.

I would also like to know whether my points against this being a duplicate of the B4D project at this time are valid. I believe that they are, but if the Drupal leadership disagrees with me, I will delete the project.

Lyricnz, when you refer to the bleeding edge state of this code, are you referring to the quality of the code itself, or the fact that it is new? In either case I would like clarification so I can not make this mistake in future. If code quality, could you (if you have time) be more specific so that I can fix the issues?

Is it standard practice when releasing a new module to leave it in dev for a while (how long?), or create a release immediately if you consider the code to be stable?

Thanks!

This is a brand new project. If the code is stable, and you have been using it for a while, then you can probably create an initial stable release. Otherwise leave it in -dev until a quite few other people have used it with good results. "Bleeding Edge" refers to the maturity of the code, not necessarily its quality - new code, or rapidly changing code, is just more likely to have issues - so it shouldn't be "released" until it's stable, reviewed by others, etc.

Case in point - you've had 3-4 releases in a single day, and *still* had a security issue. Can you imagine a "real" software vendor doing this? Relax, let people (knowingly) use the -dev release, and work on code quality etc. Have you run Coder module against it?

I note that the code has a few "TODO" or "this doesn't work yet" comments. Plus quite a few style/code issues. These all strongly indicate that the module should be in -dev for now.

Please don't take this as criticism - thanks for your contribution - we're just trying to help you do it better.

Just at the code layer, some comments:
- module should not contain redundant code
- function description comments should end with a full-stop
- by convention, permission names should generally be lower-case
- redundant initialization

$rules = array();
$rules = module_invoke_all('bespin_embedded_presets');
return $rules;

- bespin_embedded_get_presets() is your function, it always returns an array. Either use is_array() or empty(), or just iterate it anyway (empty array = no problem).

// Add presets
$presets = bespin_embedded_get_presets();
if ($presets != array()) {
  foreach ($presets as $preset) {

- Agains, rules is always an array, and checking for inequality against an empty array is silly. http://au.php.net/empty

  if ($rules != array()) {

Thanks for the review. I have fixed those issues now, plus some code style issues that the coder module brought up.

I appreciate the help!

@sun: Could I get a resolution of this bug? I think I have addressed the security and duplication concerns. Would you please review this to see whether it is resolved?

Thanks!