After discussion with dawehner, I have created this patch. Basically it does the following;

  • Adds an is_groupable() method views_handler_field class that returns either TRUE or FALSE (TRUE is default)
  • Adds a new paramater ($groupable_only) to the get_field_labels() method on views_plugin_display
  • Adds TRUE as an arguement in the options_form method in views_plugin_style class so only fields that have is_groupable() are returned

This is just an initial idea, it might be better to have the is_groupable stored differently on the field handler, it makes sense to add this as a default though. I am also not sure about adding a parameter to the get_field_labels method. It might be better to have a new method? I didn't want to duplicate the code in get_field_labels as it needs to access the handler directly anyway.

CommentFileSizeAuthor
#11 views-1548948-11.patch656 bytestim.plunkett
#9 1548948-9.patch2.71 KBdamiankloip
#6 1548948-6.patch2.71 KBdamiankloip
#5 1548948-4.patch2.57 KBdamiankloip
#3 1548948-3.patch2.57 KBdamiankloip
views_groupable_field_list.patch2.31 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
Title: Add ability for field handlers to declare whether they can be grouped by or not. » Add ability for field handlers to declare whether they can be 'grouped by' or not.
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work

This might be won't fix only because it will cause fatals for any module that overrides it.

However, I'm not sure that any modules are overriding it, or why they would. And this doesn't change the default behavior.

Docs nitpicks:

+++ b/handlers/views_handler_field.incundefined
@@ -371,6 +371,15 @@ class views_handler_field extends views_handler {
+   * Determine if this field will be available as an option to group a query by in display handler settings.

Needs to be one line AND under 80 chars. And should start "Determines ".

+++ b/handlers/views_handler_field.incundefined
@@ -371,6 +371,15 @@ class views_handler_field extends views_handler {
+   * @return (boolean) TRUE or FALSE.

@return bool
  TRUE if this field is groupable, otherwise FALSE.

or something.

+++ b/plugins/views_plugin_display.incundefined
@@ -967,7 +967,7 @@ class views_plugin_display extends views_plugin {
    * Retrieve a list of fields for the current display with the
    *  relationship associated if it exists.
    */
-  function get_field_labels() {

This needs an @param

damiankloip’s picture

damiankloip commented
StatusFileSize
new 1548948-3.patch2.57 KB

Thanks Tim. I have made these changes, plus the change of method name from 'is_groupable' to 'use_string_group_by' as suggested by dawehner.

So it just leaves the issue about if any modules are overriding this get_field_labels method? If so, we can try and do this another way.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
damiankloip’s picture

damiankloip commented
StatusFileSize
new 1548948-4.patch2.57 KB

Here is a new patch removing the function signature from get_field_labels method and using func_get_args(0) instead.

damiankloip’s picture

damiankloip commented
StatusFileSize
new 1548948-6.patch2.71 KB

WIth the @param back in the docblock.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

This looks fine now.

From my perspective this would have caused just a notice.

dawehner’s picture

dawehner
Primary language German
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/handlers/views_handler_field.incundefined
@@ -371,6 +371,17 @@ class views_handler_field extends views_handler {
+   * Determines if this field will be available as an option to group a query

I'm a bit confused, isn't this option for the string groupby and not for the query? What about "as an option to group the result by in the style settings"?

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 1548948-9.patch2.71 KB

Good point, Here we go.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Fixed

Thanks for the fix.
Committed to 7.x-3.x and 8.x-3.x

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Category: feature » bug
Status: Fixed » Needs review
StatusFileSize
new views-1548948-11.patch656 bytes

I continually get "Warning: func_get_arg(): Argument 0 not passed to function in views_plugin_display->get_field_labels() (line 981 of /Users/tim/www/d7/sites/all/modules/views/plugins/views_plugin_display.inc)."

func_get_arg returns FALSE on error, but still produces an error.

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community

I didn't think it would throw an error here, sorry. Proposed followup patch looks good.

dawehner’s picture

dawehner
Primary language German
commented
Status: Reviewed & tested by the community » Fixed

Thanks for the quickfix!

Committed to 7.x-3.x and 8.x-3.x

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

grammar!