Regression: spaces in multivalue row class token separators are replaced with dashes, preventing multiple classes (patch)

Patch fixes my issue:

I am using a multiple value field separator value of " service-" to generate output as "11 service-31 service-52 service-87" (etc). The field output is then used to generate row classes via a token of "service-[field_name]".

In Views 3.11, the spaces were output as entered, producing separated row classes: service-11 service-31 service-52 service-87.

In Views 3.14, the spaces are replaced with a dash and I get service-11-service-31-service-52-service-87.

So this is a regression introduced sometime between the two versions, and is a bug rather than a feature request. Sites relying on the older behavior may be broken. This patch fixes the style plugin code so it actually does what the code comment describes.

Let's get it tested ... setting to needs review

[Edit] Test fails due to use of the += array concatenation operator.

OK, the patch doesn't entirely fix my problem anyway. My row class string:

region-All region-[field_areas] service-All service-[field_trades]

Where [field_trades] contains spaces.

Without the patch, classes are converted to lowercase (not so in 3.11) and spaces in tokens are replaced by dashes (not so in 3.11).

With this patch, I do get separate classes from spaces in tokens, but the region[field_areas] and service-All classes are removed. The problem is the use of the += array operator, which does not append elements with the same array index.

This patch uses array_merge instead of the += concatenation operator to preserve appended elements with identical numeric indexes.

EDIT: THIS IS WRONG.

The latest patch changes all dashes to spaces, even those that were part of the original values. So for instance, I wanted the values meeting and adult-education to appear as classes:class="meeting adult-education". This used to work before the regression (I think).

The regression is that I now get class="meeting-adult-education".

The patch views-split-tokenized-row-classes-2765107-9.patch unfortunately replaces all dashes with spaces, so I get class="meeting adult education" with the patch installed.

I apologize for the misleading comment above. Before the latest views fix, I used to see:
class="Meeting Adult Education"
where "Meeting" and "Adult Education" were my taxonomy terms.

Then this got changed by the Views update to:
class="meeting-adult-education"

The patch does this:
class="meeting adult education"
which is what I would expect due to the space in the taxonomy term.

I had forgotten about #1262630: Raw value tokens not replaced if used in css class and didn't know about the newer issue #2750021: View Styles Plugin - row CSS class name is forced to lowercase. I had a CSS class that altered styling based on a capitalized one-word taxonomy term (I was lucky I didn't need to restyle a two-word term.). So I just changed it to the lowercased version instead.

This patch is work on more then two word terms.

Removed trailing white space.

removed white space.

Fixing Attribution.

Reviewed and tested the patch locally. It seems be working fine.
We can move it for community review.

Thanks.

The patch at #18 does not fix the issue for me. The patch at #9 still works.

Splitting by comma-space just assumes the default separator is being used.

It would be impossible to guess the site-builder's intent by getting the multiple-value separator and splitting by that - in the example at #3 I am using the separator to separate and prefix values. Splitting by the defined separator would remove my prefixes.

The code should not be making any assumptions about the multiple-value separator being anything other than the standard css classname separator, space.

So in the case where you have multiple values and want to use the values as classnames, you need to change the default separator from comma-space to space.

In my view, #9 is still the correct patch.

Opinions?

Patch #9 worked for me

#9 worked for me as well.

Somebody set its status to RTBC then. I can't RTBC my own patch.

If I understand this issue properly it is about whether spaces in a token replacement term are allowed to percolate through into css or not (and thus by implication become several css classes?

My prior expectation is that if a single taxonomy term that is included into a css context contains a space they are converted into dashes, so as to retain the terms integrity as a single thing. Of course there's the whole thing about when is a css string meant to be a single thing and when multiple things, and perhaps that's where I am confused..

If this patch is about changing that, I think it needs a lot more buy-in from the community and my initial feeling would be 'no': this could affect far to many sites adversely.

If not, please educate me :-)

Checking out the code alone. It needs tests, and:

+++ b/plugins/views_plugin_style.inc
@@ -130,7 +130,7 @@ class views_plugin_style extends views_plugin {
+          $classes = array_merge($classes, explode(' ', strip_tags($this->tokenize_value($token_class, $row_index))));

Code readability note: please split this tree of 4 nested calls at least into pairs; it is hard to read and very hard to debug through if needed.

I'm not sure if your prior expectation is how it actually worked before the changes were introduced. If that's the case, no that should not be changed.

But my main concern is that the changes caused a token representing a multiple value field to be combined into a single css classname, when that was not previously the case.

Edited title to better describe the specific problem.

I can confirm too that #9 is indeed working nicely! Thanks a lot - this saves a lot of time.

#9 works for me too. Tested on local env works great.

@Vj @hartsak can you elaborate on the testing you did?

[We need to know whether this fix might cause regressions in any site, not just aspects of sites needing this change.]

The issue itself is a regression. The longer we leave it before committing a fix the more chance that some sites will be relying on the new broken behavior, while others need the original output.

Put it this way: My rather complex example with tokens and prefixes on multiple concatenated field values is about as tricky as things are likely to be. The patch fixes that, and does not break any of the simpler tokenized classes that I use liberally in other sites.

I agree 100% that there should be tests clearly documenting what the expected behavior is, and then make sure the code works that way. It seems like we have several possible outputs here, we need an agreement on what those are, tests to document it and then the code can be fixed to work that way, possibly with a variable to control which way it works. In the meantime, there are patches above for people who are dealing with a regression.

So yes, it definitely needs tests.

@rivimey:

Here are the steps which i followed to test this.

1. created a view with article content type which has multiple tags.
2. Added token of tags in Row class at unformatted settings.(screenshot settings)
3. Classes are added at row level with combined into single (screenshot before_patch)
4. After patch apply Classes are divided at row level(screenshot after_patch)

Thanks,
Vj

For anybody looking to apply patches from this issue:
The patch (I believe #9) is included in Views 7.x-3.16

I'm not sure if this is due to the patch above, but with Views 3.16, a number of our single-value tokens, which we were creating classes from, changed from values like in-progress to In Progress which breaks the styling. (see screencaps)

It matches the output of the field the value is being pulled from, but there's not an obvious way to rewrite these with the Views UI.

The patches in #9, #13, #15, and #18 do not apply to the latest 7.x-3.x-dev. I think #9 is the correct patch and needs a reroll (and tests).

#9 doesn't apply because an identical patch was already applied to master in 5df2dc4fdb020c571f955e930490dae210d1bede for issue #2752617 . After looking them both over, I'm pretty sure this is a straight duplicate. Nothing is covered in this one that isn't solved in that one (although tests were never written in either case), so I'm going to go ahead and resolve it as a duplicate.