Getting a few of these in core (latest checkout of DRUPAL-5)

modules/aggregator/aggregator.module
    * Line 200: use <br /> instead of <br>
          '#default_value' => variable_get('aggregator_allowed_html_tags', '<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>'),

    * Line 1399: use <br /> instead of <br>
  return filter_xss($value, preg_split('/\s+|<|>/', variable_get("aggregator_allowed_html_tags", '<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>'), -1, PREG_SPLIT_NO_EMPTY));


filter.module
Line 1169: use <br /> instead of <br>
 * Convert line breaks into <p> and <br> in an intelligent fashion. // this is in a string.

node.module
Line 184: use <br /> instead of <br>
  $breakpoints = array('</p>' => 0, '<br />' => 6, '<br>' => 4, "\n" => 1);

I wonder if it's worth removing this check. While the coding standards advocate for proper XHTML, it's outside of the scope of checking PHP code.

Comments

douggreen’s picture

douggreen commented

I think it's a useful warning.

There are a couple of options:

  1. remove the warning as you suggest
  2. move the rule to another review
  3. add comment style markup to the source to instruct coder to ignore warnings - I don't like this idea because it actually adds characters that need to be interpreted when in fact they are only helpful to coder... if php were compiled, then that would be a different matter.
  4. allow the user to disable certain warning - IMO, this is the best option

The warnings that I'd kinda like to get rid of are all of the drupal_string functions. While the coding standards suggest using them, it also acknowledges that they aren't always the best choice. I've found myself adding some drupal_ functions just to get rid of the warning, when the native string function would of done the trick.

I'm inclined to say "won't fix", but kinda like the "disable warnings" capabilities.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I'd rather disable warnings than won't fix...

But the XHTML error parser is imo broken... either it should be flagging all of those things in the 'allowed tags' (as <a> isn't valid XHTML either.. though <a /> would be), or it should insert exceptions for patterns like that.

Some background.

I'd like to see this tool made the definitive tool for automated testing new Drupal patches and modules... as ubiquitous as devel module. Every developer's test setup then becomes HEAD + Coder module, and patches can be instantly checked to know whether or not they coding standards violations, without having to waste time eyeballing it.

But this transformation can't happen when:

a) Core is spewing out errors by itself (I'm working on that)
b) The tool is flagging errors that aren't actually errors (like these)

So I'm not sure what to do. :(

douggreen’s picture

douggreen commented

I'd like to see this too:

I'd like to see this tool made the definitive tool for automated testing new Drupal patches and modules... as ubiquitous as devel module. Every developer's test setup then becomes HEAD + Coder module, and patches can be instantly checked to know whether or not they coding standards violations, without having to waste time eyeballing it.

I suggest a severity level, i.e., "critical", "major", "minor". Someone would need to pass "major" warnings to submit and "minor" warnings would be ignored by default. I'll need to identify the "critical" and "minor" rules and add this "#severity" flag to the rule definition.

I don't like the language. "Critical" seems apt for security violations, but "major" and "minor" don't quite fit. IMHO, most of the style warnings are trivial (for example, placement of dot after a quote), but in our system these need to be tagged as "major".

So, first, do you like the idea?

And second, do you have a suggestion for the severity names?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Ahhh... Yes! I like that a lot.

How about "critical", "normal", and "minor"? Just like issue priorities. :)

douggreen’s picture

douggreen commented
Status: Active » Fixed

I implemented the severity and checked it into the HEAD branch. HEAD also includes some major UI improvements. Let me know when you've had some good testing with all these changes so I can move them over to the DRUPAL-5 branch.

As always, Thanks for your support and suggestions!

douggreen’s picture

douggreen commented

See also 109875 for other changes in HEAD.

douggreen’s picture

douggreen commented
Status: Fixed » Closed (fixed)