lukio [gcoop]

This module leverages the functionality of FileField and behaves nearly identically.

That makes your module a duplicate of an already existing project. Apart the fact the output of Drupal is still XHTML 1.0, why didn't you open a feature request for the existing project, if you wanted HTML 5 compatible output?

Drupal output is still XHTML 1.0 transitional; how can you use tags from HTML 5 if the output is not HTML 5?

Drupal themes can output valid HTML5. There's one in CVS already. I've published Drupal sites that do it. So there's clearly a context in which HTML5 CCK fields would be useful. It seems kind of ridiculous to imagine Drupal will completely ignore the next generation of the most widely used web standard.

On the other hand, HTML5 is pretty new (still in last call status), very few Drupal sites are using it yet, and Drupal defaults to XHTML1.0 output. So it wouldn't make much sense to include the functionality directly in filefield, where it would produce invalid output on most sites. A separate module seems like a good way to make it available only to those who actually want it.

Drupal themes can output valid HTML5.

Indeed they can, but out of 423 themes for Drupal 6 you report only one theme that output HTML 5.

Indeed they can, but out of 423 themes for Drupal 6 you report only one theme that output HTML 5.

Right, most HTML5 is currently being done in custom themes (such as those I've written, mentioned above), because HTML5 is very new. As evidence, here's a tutorial for making HTML5 Drupal themes. Clearly there exist HTML5 Drupal sites, so there is a clear use case for this module. As more evidence, here's a comment asking for the functionality this module provides.

If there's a published policy prohibiting anything but XHTML 1 in Drupal, please point to it, as I would like to work to change such an insular policy. If there is no such policy, I suggest it's harmful to apply opinion about what people should be doing as policy on what we can do. Experimentation with new technology is vital to the long term success of Drupal.

It is not necessary HTML 5 tags to reproduce audio content; in fact, there is already a module that reproduce audio files but that uses normal XHTML output.
Having a module that outputs HTML 5 tags when all themes out XHTML 1.x is like having a module that outputs HTML 4 tags when then the theme is XHTML 1.x.

@kiamlaluno this assumes that people only run themes which are available on Drupal.org CVS. In fact, in the dozens of sites I've helped develop over the years we have never once done that. There was clearly a use case for someone originally developing this module, there's also evidence of multiple discussions on HTML5 themes, as well as forum comments asking for the functionality this module provides. Additionally, there is at least one HTML5 theme which is in the Drupal CVS repository.

I believe even if there was an absence of documented issues of people discussing and using HTML5 in Drupal we should avoid rejecting a CVS application on the grounds that someone is of the opinion that the module is not useful. If we want to have assessing code quality and security as metrics for CVS gate keeping I am all for that, but attempting to dictate use cases is harmful practice which we should avoid engaging in.

lukio, Drupal coding standards require valid markup, and HTML5 audio and video tags are only valid in HTML5 documents, so using them in most Drupal themes would violate Drupal coding standards. This application process is intended to enforce Drupal coding standards, so the question is over whether or not HTML5 documents are allowed in Drupal, as that's the only situation in which your module would produce valid markup.

It's not really a question of HTML5 vs. XHTML 1 so much as (X)HTML5 or XHTML 1 vs. XHTML 1 only. Until the wider Drupal community clearly answers that question, I'm suggesting we defer to the more open answer.

If the browser supports HTML5 (see the table) will use the tags audio or video.

See what also reported by sreynen:

Drupal coding standards require valid markup, and HTML5 audio and video tags are only valid in HTML5 documents.

It's not really a matter of what the browser supports; HTML5 tags are valid when document is HTML5. So far, Drupal is not producing HTML5 output.

So far, Drupal is not producing HTML5 output.

This is demonstrably false. There's some room for debate (though this is the wrong place to have the debate) over whether Drupal should be producing HTML5 output, but Drupal clearly is producing HTML5 output on some sites.

In addition to the theme cited earlier, there's a module called HTML5 already in CVS, as well a couple other modules that output HTML5 markup: geolocator, which uses HTML5 geolocation and visualize, which uses HTML5 canvas element.

A quote from Larry Garfield on HTML5 in Drupal:

"there's certainly nothing stopping a module or theme from using HTML 5-specific tags."

This is demonstrably false.

That depends on what you mean by Drupal. I am referring to Drupal core modules, and themes, which are not producing any HTML 5 output, so far.

Drupal core modules produce markup that is valid HTML5. Here is evidence. There has been no evidence presented contradicting this, only opinion.

Larry Garfield:

"So if you wanted to, for example, develop a module that used the <video> tag, Drupal won't get in your way."

That's exactly what this module does.

I've reviewed and tested this module and found it meets Drupal coding and security standards, so I'm changing the status. I acknowledge kiamlaluno disagrees, but don't believe that personal opinion is sufficient grounds for rejecting the module.

don't believe that personal opinion is sufficient grounds for rejecting the module

Actually it was you to report that Drupal coding standards require valid markup, and HTML5 audio and video tags are only valid in HTML5 documents, so using them in most Drupal themes would violate Drupal coding standards.

Considering that one of the criteria reported from Apply for contributions CVS access is the respect of Drupal coding standards, I would say that it's not a personal opinion.

Then, before to mark it as reviewed & tested by the community, somebody should verify if the module doesn't duplicate the purpose of an already existing module.

There are modules in contrib that are actively testing the very bleeding edge of the web; for example, a lot of the RDF-related stuff, MongoDB integration, etc. Similarly, there are also modules that require PHP 5.2 or PostgreSQL 8.3 or whatever in order to do really advanced, cool stuff.

I think this is a great thing, and helps put Drupal out front as a leader in pushing forward web standards and new technology, and also tests the waters for things that Drupal core could be doing in the future, to help enable global spread of these tools.

It seems the concerns with this module causing incompatibility with themes or similar could simply be addressed by a disclaimer on the project page. This is how it's done on most 'normal' modules that conflict with others due to other things, such as function naming conventions, etc. Any support requests caused by conflicts would end up going into the module's issue queue for the maintainer to address anyway, so I'm having a bit of a hard time figuring out where the resistance to this is coming from...

There are many modules for handling audio, or video output that I find hard to believe there isn't a module with the same purpose, or to which require a feature request.

The fact the module outputs HTML 5 tags is not something that differentiates this module from other modules; differently, one could create html5_emfield.module, html5_emvideo.module, etc.

Seems to be a good module. Thanks for posting it.

@lukio: See my previous comment. The output generated from the module is a detail that doesn't change the purpose of the module.

I hope you didn't injure yourself in that sudden spin from the output being important enough to refuse the module to the output being an insignificant detail.

Duplicity aside, if this needs work, what specific work does it need?

If the features of HTML5 audio and video have been determined to be insufficiently unique from Flash alternatives, that's effectively a declaration of "won't fix" and the status should be changed accordingly.

The absurdity of an open source community refusing to allow use of an open standard in place of a proprietary alternative is almost as funny as it is disappointing.

If people could knock off the unhelpful, combative attitude, that'd really help us work through this much faster/more easily.

So it sounds like the concern is not that HTML 5 breaks compatibility with existing Drupal core / majority of contrib theme output, but rather that this module duplicates the work of other modules in contrib (which seems quite clear, given that it's mostly a copy/paste/modify of files with quicksketch as the author), and that adding HTML 5 output could be done as a patch to one of the existing modules, rather than a completely separate module.

I think that there is merit in this concern, although looking at the code I'm pretty sure these modifications would not be accepted upstream; for example, there are hard-coded assumptions about file extension types, etc. In the future, the way to go about this, before you grab code and start modifying it, is to post a feature request to the module issue queues; that way you can perhaps work with the maintainer on a solution. I don't see 'lukio' anywhere in the FileField/ImageField queue, so doesn't look like this was done. I'd recommend you do that now, regardless, though. The maintainers might mark it "won't fix" (which will probably be the case, since these modules were moved into core in D7 and D7 is closed for feature requests) or they might say "Awesome! Please give me patches and I'll work with you to make them worthy of commit!" but you'll never know unless you try. :)

But a more serious issue to me is that due to the duplicated code in this module, there would need to be coordination to ensure that bugs fixed in FileField/ImageField cascaded down to this module, which sounds like a headache (with potential security implications if an SA is missed). This is concerning.

I'm generally ignorant about CCK, so this might not totally make sense, but I wonder if this entire module could be slimmed radically down to just a set of formatters for FileField/ImageField, rather than handling all of the logic for uploading as well. See http://drupalcode.org/viewvc/drupal/contributions/modules/cck_extras/cck... as an example of a module providing formatters for media, without all the baggage that goes along with it (unfortunately, D5).

I think due to concerns about potential conflicts with HTML 5 and existing core/theme output, if this module could be slimmed down to /just/ the output part, rather than the field storage/widget part as well, that would be a worthy module ("HTML 5 formatters" or similar) of its own right, and wouldn't increase the support burden on FileField/ImageField. lukio, you obviously have done more work on this than anyone else here. Do you think this is possible?

Unlike CCK Extras, this module necessarily puts restrictions on the input (only audio formats make sense) in addition to formatting the output. Maybe it could just apply that same check on output and output nothing for invalid types?

Yeah, something like that would work, I think. Unless there's a way to inject your own validation into other modules' CCK widgets, which I am not sure. I've had very mixed (and often frustrating ;)) results doing seemingly "normal" hook_form_alter() stuff with CCK fields.

I hope you didn't injure yourself in that sudden spin from the output being important enough to refuse the module to the output being an insignificant detail.

The fact the output is not correct is important enough to refuse the module, and the fact the output is HTML5 doesn't make an important difference to justify the creation of a module that duplicates another existing module.
I hope you are able to see the difference between output not correct, and HTML5 output. HTML5 output is not automatically not correct output; it would be correct if Drupal coding standards would contemplate it.

Talking of sudden spins, it was you who said that the coding standards require XHTML output, but then you said that I was blocking the CVS application for personal opinions.

The code needs to be changed, even if for a moment we would forget what reported by the Drupal coding standards, and what reported by webchick.

• The module includes a copy of the jQuery library. Modules should never include the jQuery library (the only exception is for jQuery Update, but that module replaces the jQuery library Drupal comes with, and make it available for all the modules); including the most recent version of the library when Drupal already includes its own copy of the library can just create problems, including compatibility problems (which are instead resolved by jQuery Update).
  if the module really needs to use a more recent version of jQuery, then it should require jQuery Update to be installed.
• The module includes a copy of the jQuery UI library. As there is already jQuery UI, the module should use that module, which has been created with the exact purpose to make available the jQuery UI library to other modules, and avoid to have multiple copy of a library. To notice also that files available from third-party sites should not be included in CVS repository; there are some exceptions to this (which applies to jQuery Update, in example), but considering that jQuery UI (the module) doesn't include the jQuery UI library in the repository, I don't see the reason why for this module should be valid the exception.
• The module includes jquery.jplayer, which is available from third-party sites, and then it's licensed under MIT License.

Of course, what reported by webchick must be taken in consideration, and the module as it is now cannot be accepted. If it would be accepted, then we should accept all the other modules that are modified versions of modules already existing in Drupal.org CVS repository.

To notice it was already pointed out in comment #2 that the module was a duplicate of an already existing module, but the OP said it was not (except that the module is a modified version of an already existing module, as also webchick confirmed). It took us 23 comments to return to say that the module is effectively the duplicate of another existing module. Would not have been better to be honest, and say that effectively the module is a modified version of another existing module? Would not have been better to be honest, and admit that the code effectively could not be accepted as CVS application, rather than saying that who was reviewing was expressing personal opinions?

I regret my participation in this thread. I joined in to to advocate for HTML5 experimentation in Drupal, and in the process completely derailed the thread. Now that HTML5 is no longer of questionable validity, please disregard my comments and go back to focusing on the module review.

it was you who said that the coding standards require XHTML output

That's not quite what I said, though I can see how you misunderstood me. I said the standards require valid output, and XHTML is the most common DOCTYPE. That was my attempt to explain your initial objection to the module to lukio, who seemed confused about it. I am firmly against reinforcing the status quo without good reason, and I've never seen a good reason to disallow HTML5 in Drupal.

I completely agree with the most recent critiques of the module code rather than the very concept of the module.

What I wrote in comment #2 is clear, and it was not about the module output, which is still a problem when the output of the enabled theme (which cannot be controlled by the module) is not HTML5 (which has a doctype different from the one used by XHTML).

I will clean up the code from imagefield/filefield module. But i don't understand if you want us (gcoop) to change the module so it can be another cck field and not to be integrated with filefield? because we tought that was the better to do. what do you think?

No, sorry if I was unclear. You're absolutely right that you want to build off of existing modules, so creating a dependency on FileField makes a ton of sense.

But the thing that worried me was all the copy/paste from FileField/ImageField module, and how those things will be kept in sync in the future. For example, most of the functions in htmlmediafield.file.inc ... and html5mediafield_file_download() is a function with security implications.

Just glancing through, I also was not understanding why this module needed to implement its own widget, and couldn't just piggy-back off of FileField's. But now that I look closer I see that it /is/ piggy-backing off of FileField's, but you're adding additional validators, e.g.:

  // Start with the FileField widget as a basic start.
  // Note that FileField needs to modify $form by reference.
  $element = filefield_widget($form, $form_state, $field, $items, $delta);

  // Add html5mediafield specific validators.
  $element['#upload_validators'] = array_merge($element['#upload_validators'], html5mediafield_widget_upload_validators($field));

So my concerns are probably due to my general ignorance about CCK's API. I would've expected this module to be do-able with just a few theme functions, a couple of hook_formatter_X() hooks, and a hook_form_alter() for the extra validation. But it appears it's not so simple...

But if you could make a run through that cleaned up and removed as much of the copy/paste code as possible, and clearly outlined in comments places where there's a good reason you are duplicating hooks from filefield (like you've done in hook_widget_info()), that'd really help with the review, I think.

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.