When RDF is enabled, grouping can behave incorrectly

These are two separate issues. A format that has no time should not be appending 'All day' to it, so I just committed a fix for that.

I can confirm that the grouping is still not working right, at least in a case like this where we are trying to group by month and year. The grouping is handled by Views, Date module is doing nothing about how that is done. I have to dig into how Views is doing it to figure out why we're not grouping by the rendered value.

Found the problem. This works fine if you have the RDF module disabled. Views uses the rendered value for the field, including its wrappers. When RDF is enabled, the value for the field looks like:

<span class="date-display-single" property="dc:date" datatype="xsd:dateTime" content="2011-05-08T23:00:00-05:00">May 2011</span>

Because the ISO value is included in the rendered HTML, each date is actually different, even if the rendered values are the same. So if you have multiple different dates that are displayed as 'May 2011', they won't get grouped together.

I'm not quite sure what to do with this. This is going to affect other fields that use RDF I think, so there might be a bigger Views issue here. I think Views should be doing a strip_tags() on the rendered value, and then grouping by *that* instead. Except then the headings might not display correctly.

Bumping this to the Views issue queue to see if they have any ideas.

I guess the only question is whether the RDF values should be truncated to match the displayed value, but I don't think we should assume that every field that is displayed the same will have identical RDF markup. The RDF markup can get quite complex and certainly might be somewhat different for fields that are being manipulated to display the same way so they can be grouped. Plus I'm not sure it's valid or even logical to have RDF markup in the heading. The more I think about this the more I think it would be better to strip that information out before grouping.

So what i think this should wait/be done during the improvement of the groupby mechanism as this is somehow unflexible at the moment.

I think the solution would be in the render_grouping function in views_plugin_style.inc. The field is rendered first. Then we do $grouping = $this->get_field(); If you examine the value of $grouping at that point when RDF is enabled you have the displayed value surrounded by a span that has RDF markup in it, and as noted above, the RDF markup may not match the displayed value. If we added something like:

$grouping = strip_tags($grouping);

The field would group correctly. The only question is whether there is any valid reason why that span with its markup is really needed. I don't know that it is. I don't think RDF markup is needed for a heading anyway - that would be a question for an RDF expert.

Thanks KarenS. was having this issue with 7.x-2.0-alpha4. Disabling RDF did it for me.

I can reproduce the bug. I'm not sure what was the use case for grouping with the rendered version of the field. Maybe dereine will remember what he had in mind when he committed this code (commit 37c83cd66aaf2d1edfceeac270550d132d97d011).

I don't think RDF markup is needed for a heading anyway

I agree. From an RDFa perspective I can't think of any use case where this would be needed in the heading, since the RDFa markup would require other elements around it to be make sense anyways.

I'm attaching a patch implementing the fix suggested by Karen in #8.

Patch in #11 works for me, with and without the RDF module enabled.

I guess there's not only an issue with rdf but in general with the rendered content.
E.g. if there's an id attribute in the (html-)templated for the grouping field - filled by using drupal_html_id() the id's are enumerated automatically to avoid collisions - what of course breaks the grouping functionality.

But because of the way how "sets" are handled actually a simple strip_tags() would introduce a new problem. The array key (the value of $grouping) is directly used as $title in views_plugin_style::render(). That simply means a strip_tags() would break any html output.

The attached patch changes the structure of the grouping array to allow to group by the raw value of the field while use the rendered field as its output.
To still provide the possibility to affect the grouping by the theme there's now a new option to specific whether to use the raw or the rendered field value as group identifier (default is to use the rendered output for backward compatibility).
The new function views_plugin_style::get_field_value() needs some attention on review - I'm not sure if this is the right way to get the field value ;)

The second patch contains the necessary change to keep the ctools views_content plugin up to date.

Ouch previous patch misses the modified non-group case.

Slight fixes as mentioned by dereine via IRC.

Commited to 7.x-3.x as reviewed. In general this is a patch which should probably be backported but it doesn't apply clean.

This is probably not that hard to backport.

Sadly this creates bugs for each existing style plugin, so this patch got reverted.

@dereine is there an issue for this bug with the style plugin, or how do you reproduce it?

Yes there is #1315320: Jump menu doesn't loop through rows
The problem is not the style plugin, it's that the "api" was changed with this patch.

in IRC @merlinofchaos suggested to introduce an optional API change, which would be triggered by a DX-only option inside the style plugin.

Oh sorry guys I forgot to create a separate ticket for ctools *blush* - #1235994-13: When RDF is enabled, grouping can behave incorrectly already contains a necessary ctools fix.
At least this should work in the default case - I'll take closer look now.

Ok, here we go.

Now the patch returns the old structure if the parameter $group_rendered isn't explicitly set.
And since older code doesn't even know this parameter, these code will always get the return structured the old way.
Unfortunately there's a slight performance drawback for older code because I decided to restructure the already built grouping if necessary.
The reason why I decided to do so is that it's much more maintainable if you see that there is some special code for compatibility reasons. Besides that it's relatively unlikely that there are that much records that this approach really slows down the execution.

+++ b/plugins/views_plugin_style.incundefined
@@ -266,10 +279,18 @@ class views_plugin_style extends views_plugin {
+  function render_grouping(
+    $records,
+    $grouping_field = '',
+    $group_rendered = NULL

It's probably enough if this stays in one column

In general this looks fine, but assign to merlinofchaos for a proper review.

Okay fixed this style and added some tests, just to be sure it works as expected.

The tests are running as expected.

Patch still applies & tests are passed.
Added sprint tag.

Added documentation to help/api-upgrading.html and fixed typo.

Great this update documentation looks fine!

Committed to 7.x-3.x

Unfortunately, this is still badly broken for the use case of grouping by a date field, grouping by day, but with different times on the same day all getting put under the same day heading. The patch in #26 does not account for fields that are an array instead of a string, such as a date field. At any rate, grouping by the raw values still would not work for this use case, since the times would be different. Would the maintainers accept a patch that added a checkbox to allow stripping tags when grouping by the rendered output?

thanks,

Adrian

@Adrian: This is a really interesting use case - I guess I missed the fact that a field also could return a complex datatype, which can't be used as array key.
Stripping tags isn't the way to go since this doesn't solve to root cause - at least this is my perspective.
I'd say we always serialize the key which we use for grouping - that way we ensure also complex datatypes can be used as group.

@dereine, @merlinofchaos: If there are no objections I'll create a patch :)

Hi Peter,

unfortunately, your proposed solution won't meet the use case, as I (and the OP) wish to group fields with different times having the same date. (or month, year, hour as the case may be)

I agree that stripping tags from the rendered output is not the most elegant solution, and am open to another one. However, getting into special-casing fields by type could get very complex, and grouping by a formatted date is a very handy thing that has worked for some time in previous versions of views AFAIK.

thanks much,

Adrian

@Adrian: I've just discussed that with dereine via IRC. Unfortunately we couldn't find a solution yet - I'll continue work on this asap. But for now I need some hours of sleep :)

Ah. After fiddling for hours, I found this issue. Disabling RDF for now until there is a better solution..

I really don't think there is much else I can suggest that doesn't involve stripping tags -- there's not a lot that Views can really do here. If the output is different, it's different; you can't expect Views to try and render the HTML and compare that, it's just not possible.

You can strip tags on the value you're using for grouping, at least; you can exclude that value and print different values, and potentially in the template you could use tricks to improve the actual rendered grouping.

Warning: Illegal offset type in views_plugin_style->render_grouping() (Zeile 312 von /home/www/cleantech/sites/all/modules/views/plugins/views_plugin_style.inc).

After updating Views from RC3 to 3.0 in the hope of grouping by a date field (see #1235994: When RDF is enabled, grouping can behave incorrectly) I have also encountered this error when I turned off the "Use rendered output to group rows" option.

The $grouping variable looks like this:

    0 (Array, 5 elements)
        value (String, 19 characters ) 2011-12-23 19:00:00
        value2 (String, 19 characters ) 2011-12-23 19:00:00
        timezone (String, 17 characters ) Europe/Bratislava
        timezone_db (String, 3 characters ) UTC
        date_type (String, 8 characters ) datetime

But arrays can not be used as keys in:

  if (empty($sets[$grouping]['group'])) {
    $sets[$grouping]['group'] = $group_content;
  }

That's odd. That's a raw value but the grouping variable always pulls from rendered content. I don't quite know how that can happen.

I am attaching a patch with a possible temporary workaround. I don't think this is a good solution, but maybe it can give you some clues on how to fix this properly.

#1391668: Group by all taxonomy terms is another example that this concept can't work at all :(

Now with the feature to group by multiple levels md5(serialize($value)) is used here, so it should work for date fields again.

Additional the old feature is still there, but hey it still don't work for rdf.

Here is a patch which allows to configure that, it works quite well, but there is not simpletest yet.

This is a version with a test.

Manual testing worked fine, but the tests doesn't work at the moment.

I've reviewed this patch and can confirm that grouping by a date field works correctly. thanks much!

How can a patch which has failing tests be RTBC :(

Here is a new version with a working test.

Removed one debug()

Committed this patch now, as it has a quite good test coverage.

Great, it works. confirmed I was able to use grouping with date (year in my case) and rdf enabled.

Late to the game, but I can confirm that this works.

I applied the patch but it does not seem to allow me to group using a date field and a custom date format. Do I need to do something to force reload of the .inc file? I cleared caches and disabled & re-enabled views.

Did you checked "strip tags" in the style settings ui?

@misterT: you only need to apply the patch if you are running the latest stable 7.x-3.1. this patch is included in the latest dev release.

Click "settings" in the format section of your view, and make sure to check the "Remove tags from rendered output" check box in your grouping field. see screenshot of a working example.

Thanks guys. Thought I had replied to this already. The culprit was "remove tags from rendered output" IIRC.

Hi,

I originally posted a bug report on http://drupal.org/node/1371252#comment-5417016 but was told in a subsequent post (http://drupal.org/node/1371252#comment-5426442) that it was being addressed here.

Since confirming that I was experiencing this bug, I have updated the Views module twice since 31st December, and am now at version 7.x-3.2. However, I am still experiencing this same problem. Additionally, enabling and disabling the RDF module has no effect on the occurrence of the problem.

Can anyone please tell me if there are any plans to address this bug in a future software release, and if there is any indication of when that might happen. It should come as no surprise that I have a website waiting to go live, but I am waiting for this issue to be resolved.

Cheers,

Garry.

@musicologist1964:

Additionally, enabling and disabling the RDF module has no effect on the occurrence of the problem.

If the bug is not disappearing when disabling RDF then it's likely not directly related to this issue directly (unless there is another contrib module at play here). In any case, I'd advice to start a new support request to investigate this further.

hello
im sorry to tell you this but it looks like this issue has came back.
using views 7.x-3.13 and drupal 7.43

@ssm2017 Binder: Can you confirm that the problem goes away if you disable the rdf module?

yes i confirm that the issue dissapeared after disabling the rdf module.
on today, i have updated one of my websites and then i could see that i had all days in separated tables instead of the same one (the items are grouped by date using the computed result).
i have searched for a workaround and i have found this issue entry so i have disabled the rdf module and then the problem dissapeared.

A very easy solution to solve this problem is just stripping the HTML tags of the date field you are using for the grouping. It also strips the RDF information on the fields so grouping works properly again and you don't need to disable the RDF module.