This is somewhat related to these issues:
#939992: "CSS classes must be alphanumeric or dashes only." I think underscores should be included
#1032380: More than one wrapper class

I would like CSS classes to be allowed to contain underscores, because those are heavily used in many popular CSS frameworks. Underscores get replaced by default in drupal_clean_css_identifier, unless an alternative filter is given as argument.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pisco’s picture

Status: Active » Needs review
FileSize
2.56 KB

Added a patch for review.

Pisco’s picture

Rerolled patch versus current head.

dawehner’s picture

Status: Needs review » Fixed

Thanks! commited to 7.x-3.x

perhaps we should reintroduce the views specific function.

Pisco’s picture

I'd recommend that, I think it's weird having the custom filter array('/' => '-', '[' => '-', ']' => '') spread all over the code. In my opinion, the most beautiful and robust solution would be to fix drupal_clean_css_identifier: #1109854: Overly aggressive transliteration in drupal_clean_css_identifier removes underscores from classes. But that's another story I guess.

dasjo’s picture

Status: Fixed » Needs review

the last commit causes the following site warnings for every row in my views:

Warning: strtr() [function.strtr]: The second argument is not an array in drupal_clean_css_identifier() (line 3637 of /drupal/includes/common.inc).

reverted from the stated commit (a445bf4c80fe72c9c5ca09a5c9973b214c00ebc8) back to the previous (d00cc335086dd8533a2eee8401f9b8ece86a2c7c) and the warnings are gone.

dawehner’s picture

Status: Needs review » Needs work

So this needs work.

You can't add additional parameters if you use a callback.

dawehner’s picture

Reverted the commit.

3rdLOF’s picture

subs.

BrightBold’s picture

Thanks for the fix. Un-patched my install and all it well with the world.

3rdLOF’s picture

So wait....patch #2 is good to go?

Pisco’s picture

kannary100, no #2 is not good.

#6, yes you can, but not the way I expected :-( sorry for that!

I think we need a wrapper function for drupal_clean_css_identifier, someone mentioned there once was views_css_safe, or how about naming it views_clean_css_identifier?

function views_clean_css_identifier($class) {
  return drupal_clean_css_identifier($class, array('/' => '-', '[' => '-', ']' => ''));
}

Where should I put this function?

merlinofchaos’s picture

Let's restore views_css_safe() and in the doxygen for the function, include a notation of drupal_clean_css_identifier's problems.

THen let's use views_css_safe everywhere instead.

Jerome F’s picture

Since I upgraded to views 7.x-3.x-dev datestamp = "1301660238"
running with drupal 7.x-dev release 2011-Mar-29 and ctools 7.x-1.0-alpha4

I have the following error message(s) :
Warning : strtr() [function.strtr]: The second argument is not an array in drupal_clean_css_identifier() (line 3630 dans (...)/public_html/includes/common.inc).

According to #5 this is related to this issue.

In addition the classes provided in the groups <div> and <ul> are now blank in spite of the settings in List class and wrapper class - is that a related issue or may I submit a new one ?

EDIT : I filed an issue there about that : http://drupal.org/node/1113482

3rdLOF’s picture

@Jerome F: That is the identical error I get..

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

So here is the new function.

Additonal you can use tokens now in more places.

dmsmidt’s picture

sub

Jerome F’s picture

Status: Needs work » Reviewed & tested by the community

#15 Excellent patch ! We now can use tokens underscores and spaces for multiple classes in wrapper, label and
HTML element. Works for me all right.

EDIT : see next comment - my mistake

off topic : I love the new features and interface in views ! Great work.

Jerome F’s picture

Status: Needs review » Needs work

My bad I spoke too soon - after saving and closing the view the following notices pops up :

Notice : Undefined variable: row_index in views_handler_field->element_wrapper_classes() (line 308 in (...)/sites/all/modules/views/handlers/views_handler_field.inc).
Notice : Undefined variable: row_index in views_handler_field->element_label_classes() (line 296 in (...)/sites/all/modules/views/handlers/views_handler_field.inc).

It works ok in the views preview though.

Pisco’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.52 KB

Thanks dereine, I modified your patch slightly to avoid the errors mentioned by Jerome F.

Pisco’s picture

Properly formatted the patch.

dawehner’s picture

As merlinofchaos pointed out row_index got lost somewhere for the other element_*_class functions.

Updated patch.

Jerome F’s picture

Patch in #21 works for me.

Steven.Pescador’s picture

Hi all, applied this patch but am still getting this error when viewing a Calendar...

Notice: Undefined property: view::$row_index in views_handler_field->get_render_tokens() (line 1187 of C:\wamp\www\my-site\sites\all\modules\views\handlers\views_handler_field.inc).

Apologize if this is the wrong place to post this, came here from http://drupal.org/node/1115222

dasjo’s picture

the warning is gone so far.

Warning: strtr() [function.strtr]: The second argument is not an array in drupal_clean_css_identifier() (line 3637 of /drupal/includes/common.inc).

in openlayers i get similar errors as SAF13 states in #23:

a list of

Notice: Undefined offset: 0 in views_handler_field_field->get_items() (line 650 of /.../modules/views/modules/field/views_handler_field_field.inc).

and

Notice: Undefined property: view::$row_index in views_handler_field->get_render_tokens() (line 1187 of /.../modules/views/handlers/views_handler_field.inc).

dawehner’s picture

Status: Needs review » Fixed

@dasjo, @saf13
This seems to be a bug of the openlayers module.

The default render function of style plugins set's the row_index

 function render() {
    if ($this->uses_row_plugin() && empty($this->row_plugin)) {
      debug('views_plugin_style_default: Missing row plugin');
      return;
    }

    // Group the rows according to the grouping field, if specified.
    $sets = $this->render_grouping($this->view->result, $this->options['grouping']);

    // Render each group separately and concatenate.  Plugins may override this
    // method if they wish some other way of handling grouping.
    $output = '';
    foreach ($sets as $title => $records) {
      if ($this->uses_row_plugin()) {
        $rows = array();
        foreach ($records as $row_index => $row) {
          $this->view->row_index = $row_index;
          $rows[$row_index] = $this->row_plugin->render($row);
        }
      }
      else {
        $rows = $records;
      }

      $output .= theme($this->theme_functions(),
        array(
          'view' => $this->view,
          'options' => $this->options,
          'rows' => $rows,
          'title' => $title)
        );
    }
    unset($this->view->row_index);
    return $output;
  }

Additional commited the patch itself.

Steven.Pescador’s picture

Status: Fixed » Active

@dereine
Thanks for the feedback, as I don't have the openlayers module installed, am not sure where to go from here to sort out this notice..?

dawehner’s picture

Status: Active » Fixed

Well the code in the openlayers module has to be fixed.

It has to set the $this->view->row_index for this.

dasjo’s picture

just wanted to note that SAF13 stated another problem with calendar module in #23

Steven.Pescador’s picture

Found this thread last night and added the line of code, the notice has gone away...

http://drupal.org/node/1119752

gmopinillosv’s picture

Hello there

What does "Reverted the commit." mean?

Could you deliberate please.

Thank you

gmopinillosv’s picture

Hello there

Where I put that patch? I apologize I am new in this. Thank you

dawehner’s picture

Better use the dev version, there is fixed a lot of stuff.

gmopinillosv’s picture

Thank dereine,

I used module "View version dev" and disappeared the error messages.

Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

aaronsnoswell’s picture

This is driving me mad - I'm using D7 and the 7.x-3.x-dev version of views (updated 2012-Mar-29) and underscores are still getting replaced with hyphens. I've tried applying the patch in #21, but it patch tells me it is already applied. That may be the case, but it's not having any affect.

Can anyone tell me how to stop drupal replacing underscores with hyphens for Views css classes?

aaronsnoswell’s picture

Status: Closed (fixed) » Needs work
FileSize
586 bytes

OK! After several hours of searching I found the one line fix that corrected this. Patch attached.

Jeordy’s picture

subscribing

gmclelland’s picture

FileSize
184.67 KB
171.92 KB
147.28 KB

I just ran into this as well. This is big problem not just for grids, but if you also use the BEM approach for theming:
see http://nicolasgallagher.com/about-html-semantics-front-end-architecture/
see http://csswizardry.com/2013/01/mindbemding-getting-your-head-round-bem-s...

@aaronsnoswell - Your patch applied cleanly but didn't seem to work when I tried to apply a class on the view row.

I tried clearing caches, but nothing worked.

Step 1
Step01.jpg
Step 2
Step02.jpg
Step 3
Step03.jpg

Anybody have any other ideas?

carwin’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

This patch should do the trick:

Mołot’s picture

Just a quick question - why is it views request and not core request, if what basically bothers you is how core function works? And is there any reason views should deliberately work different than Drupal's core, generating different class names from the very same strings?

carwin’s picture

There actually is a core request for essentially the same thing, but due to the vast number of modules that depend on the way it works now, we're limited in what we can do in core. That's here: http://drupal.org/node/1109854.

Views already had an essentially duplicate function, I just started using it and updated the regex pattern.

I think the short answer is that Views is a bit more flexible on this subject than core is.

BrightBold’s picture

Hmm. But if it's not likely to get changed in core, then is it unwise to put it in Views 7 since everyone's Views will break when they upgrade to Drupal 8 and Views is in core? (Oops, am I being a troublemaker?)

Mołot’s picture

@BrightBold #42 - well, that was one of my 2 concerns. Second one is - I'm using the same data via views and outside views. Explaining to my designer why he needs to write same rules twice is irritating. One of the reasons to use Drupal was to avoid things like that. I know my opinion probably hardly matters, but "a bit more flexible" hardly seems reason enough to break core compatibility in something that will BE core in next release. Contrib module doing thing it's own way may be inconvenient, but is understandable. Parts of core doing same thing different ways should be no-go. Versions of the same project breaking on upgrade due to such a minor thing should be no-go too. Well, that's how I see it. But I'm open to others seeing things different. I would just like to know why they want what they want and why they think it's worth the risk and incompatibilities.

From my point of view, best thing would be to allow underscores in core. And in core or not, it should be configurable, with old way default on upgrade.

carwin’s picture

@Molot, I think I understand what you're saying here but I'm not positive.

It sounds like you're thinking of this in terms of 8.x / Views in core, but this issue specifically relates to the 7.x branch of Views, does that have any effect on your position?

Also, I don't know of any place where using views_clean_css_identifier() over drupal_clean_css_identifier() is going to break other modules. That function already existed in the code, it just wasn't being used. There's a good chance I missed a conversation about maybe deprecating that function since I'm not very familiar with Views' codebase – I may have missed something obvious about why this wasn't being used.

I don't have a horse in this race, I'm just scratching an itch with this patch.

Mołot’s picture

@carvin - "Versions of the same project breaking on upgrade due to such a minor thing should be no-go" that's my opinion about 7.x and 8.x and how should it matter that 7.x is not in core.

And I always thought that 2 functions / modules / APIs etc doing exactly the same thing is not a "Drupal way", that Drupal way is to have one tool for one task and cooperate, not compete. That's why many _clean_css_identifier() functions looks bad for me. Now, if I could be sure they give the same result for same input, or only one will be used by all modules in my project... I will have the same data rendered via views and our custom views plugins for readers, and more directly for management. They should share classes, for css readability if nothing else. But views extensions avoiding views-provided function? How can I be sure my co-developers will not use it? It would be natural to prefer views_* functions in views submodules, after all. And I bet I'm not alone with use-case like that.

carwin’s picture

While I agree that having two similar functions isn't the best situation, I don't see how this specific patch is going to harm anything in Views by allowing you to throw an underscore in your class name. If/when you move your project over to d8 this shouldn't be a problem anymore because, as I understand it, d8 won't strip underscores. Drupal's CSS coding standards encourage the use of underscores in class names.

I don't think I grasp what you're getting at Mołot, so I'll just leave this up to others to review. If you want to remove the views_clean_css_identifier() function open a separate ticket about it.

KrisBulman’s picture

Harald Groven’s picture

KrisBulman’s picture

Just wanted to point out that there is an RTBC core issue for 7.x that allows double underscores #2009584: Allow double underscores to pass through drupal_clean_css_identifier as per new CSS standards

drupov’s picture

I have the same thing going as in #38.

Both the patches from here (#39) and from issue #2009584 (#16 there) worked.

My question: what would be the right one to choose?

DedMoroz’s picture

Issue summary: View changes

#39 - works great.
Thanks!

emerham’s picture

So are we waiting for core to push a patch for this? It sounds like they have no intention on doing it yet for D7, but it made it into D8 core. If core isn't going to push it then I think views should do it.

carwin’s picture

I agree, but I think this issue may be stuck in the bike shed.

ChaseOnTheWeb’s picture

#39 breaks the ability to provide multiple space-separated classes. Patch attached.

(edit: Not sure why testbot failed to apply patch. It's working with drush make.)

Status: Needs review » Needs work

The last submitted patch, 54: views-allow-underscores-css-classes-1111258-54.patch, failed testing.

ChaseOnTheWeb’s picture

ChaseOnTheWeb’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 56: views-allow-underscores-css-classes-1111258-55.patch, failed testing.

zalak.addweb’s picture

Issue tags: +views
sphism’s picture

Wow... i just wasted an hour of my life because views was converting my nice BEM formatted class name underscores to hyphens

riddhi.addweb’s picture

Issue tags: +CSS classes
pvasili’s picture

Priority: Normal » Critical

Any progress in this error?

In php, you use your variable $my_var as $my_var.
Why in views $my_var variable I should use as $my-var :)??? LOL

pvasili’s picture

FileSize
6.62 KB
11.8 KB

The module panel works absolutely correctly in contrast views

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
1.72 KB

Reroled agains latest dev

DamienMcKenna’s picture

Priority: Critical » Normal
lwalley’s picture

An alternative patch that uses drupal_clean_css_identifier(), which now allows for double underscores, see #2009584: Allow double underscores to pass through drupal_clean_css_identifier as per new CSS standards.

Chris Matthews’s picture

The 2 year old patch in #64 does not apply to the latest views 7.x-3.x-dev, but the alternative patch in #66 applies cleanly and is probably the better approach.