Production sites should not have Views UI enabled, but currently Views Distinct relies on Views UI for configuration. This dependency could be solved in at least 2 ways:

  1. Allow configurations to be exportable: the configuration can happen on a development site, and then just be ported to the production site. Support should include Views' Export/Import admin interfaces, Features.
  2. Allow configurations via code: either implement some new hook(s), or tap in to an existing hook such as hook_views_default_views() (support for which is mostly free if item #1 is solved).
CommentFileSizeAuthor
#22 interdiff_16_22.txt409 bytesrobpowell
#22 views_distinct-use_variables_as_storage-2098557-22.patch7 KBrobpowell
#16 interdiff-15-16.txt2.93 KBjay.dansand
#16 views_distinct-use_variables_as_storage-2098557-16.patch7 KBjay.dansand
#15 interdiff-8-15.txt3.16 KBZeiP
#15 views_distinct-use_variables_as_storage-2098557-15.patch6.64 KBZeiP
#8 views_distinct-use_variables_as_storage-2098557-8.patch6.58 KBtucho
#7 views_distinct-use_variables_as_storage-2098557-7.patch6.8 KBmpv
#5 exportable-config-2098557-5.patch7.61 KBgildir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

bleen CreditAttribution: bleen commented

In the mean time you should make views_ui a dependency of the module so its a bit more obvious why my features weren't being marked as overridden when I changed "distinct" settings

jay.dansand’s picture

jay.dansand CreditAttribution: jay.dansand commented

The Views UI dependency was removed by request (and for good reasons: Views UI isn't always a great thing to have enabled in production). See #2098393: Remove dependency for Views UI.

(Updated to add this) The fact that the Views Distinct settings aren't managed as part of a Views plugin or other exportable structure is why Features does not detect the change, and it's a great observation - we'll need to make sure to do the exportable settings in such a way that we integrate with modules like Strongarm and Features. Thanks!

slucero’s picture

slucero
he/him
Primary language English
CreditAttribution: slucero commented
Issue summary: View changes

Exporting configuration into Features is a crucial part of the development workflow for many people. Until this is resolved can a note that the configuration may not be exported into code be added to the "Known Issues/Shortcomings" list on the module description page?

jay.dansand’s picture

jay.dansand CreditAttribution: jay.dansand commented

Thanks for the suggestion! I've added this note to the top of Known Issues/Shortcomings:

Currently, Views UI is required to initially configure Views Distinct, and Views Distinct settings are not exportable - see #2098557: Allow configuration from code/exportable configurations

gildir’s picture

gildir CreditAttribution: gildir commented
FileSize
exportable-config-2098557-5.patch7.61 KB

Attached is a patch which makes the module use variables instead of the database (allowing the settings to be exportable through strongarm). Final use of this would mean removing the schema from the install.

andreic’s picture

andreic CreditAttribution: andreic commented

The patch doesn't work. As I hit the patch command, it outputs the following:

patch unexpectedly ends in middle of line
patch: **** Only garbage was found in the patch input.

mpv’s picture

mpv CreditAttribution: mpv commented
Assigned: jay.dansand » Unassigned
Status: Postponed » Needs review
FileSize
views_distinct-use_variables_as_storage-2098557-7.patch6.8 KB

The patch in #2098557-5: Allow configuration from code/exportable configurations was corrupt, so I had to apply the changes mannually. This patch also removes the schema and adds hook_update_N and hook_uninstall.

What @gildir did was save the settings for a view in a variable views_distinct:VIEW_NAME. This way the settings are exportable via features with Strongarm.

The update hook takes all the views stored into the databases and saves them as variables, then drops the table, so if you are already using the module run db updates after applying the patch. The uninstall hook removes all the views_distinct variables.

tucho’s picture

tucho
Primary language Spanish
Location Buenos Aires
CreditAttribution: tucho commented
FileSize
views_distinct-use_variables_as_storage-2098557-8.patch6.58 KB

When executing the function _views_distinct_field_settings_get after applying the patch, the module throws a warning when trying to sanitize the value of the aggregate_separator index key.

The structure of the settings array now have three levels (View -> Display -> Field), so I just add two nested foreach to reach the correct level of the array.

ciss’s picture

ciss CreditAttribution: ciss commented

Wouldn't it make more sense to implement this as a display extender plugin? That way we'd have the storage part covered out of the box and wouldn't need as many workarounds.

scottrigby’s picture

scottrigby
he/him
Primary language English
Location Brooklyn, NY
CreditAttribution: scottrigby commented

@ciss Agreed this would be the best (this is option 2 in the issue description).

edutrul’s picture

edutrul CreditAttribution: edutrul as a volunteer and at Anexus commented

I've tested patch #8 and doesn't make views exportable to code as should be mentioned in this issue.

Gold’s picture

Gold
they/them
Location 20 minutes in the future
CreditAttribution: Gold at Catalyst IT commented
Status: Needs review » Reviewed & tested by the community

I've just patched the 1.0 branch with the patch at #8 and it went fine.

  • The drush updb worked as advertised
  • The previous configuration was maintained in the Views UI
  • The configuration was exposed via strongarm as variable:views_distinct:[view_machine_name]
  • All displays and fields for a view were in the single variable
  • The variables were exportable into a feature
  • Standing up the feature on a new test server gave me the views with the Views Distinct settings configured correctly

Until someone takes the time develop this as a display extender plugin I would consider the patch at #8 to be RTBC.

Perhaps switching to the display extender plugin approach could be a minor point release update?

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Works for me as well.

c0rtex’s picture

c0rtex CreditAttribution: c0rtex commented

Patch #8 works perfectly!

ZeiP’s picture

ZeiP CreditAttribution: ZeiP at Citrus Solutions Oy for City of Pori commented
Status: Reviewed & tested by the community » Needs review
FileSize
views_distinct-use_variables_as_storage-2098557-15.patch6.64 KB
interdiff-8-15.txt3.16 KB

There were a few issues with the coding style in the patch. Also the update hook was incorrectly numbered (7100 should be the first update) and caused a few notices because it didn't check for the existence of the table and didn't initialise the result array.

Attached is a new patch with these minor tweaks; since it contains some logic changes setting back to Needs review; however the essence of the patch worked great.

jay.dansand’s picture

jay.dansand CreditAttribution: jay.dansand commented
FileSize
views_distinct-use_variables_as_storage-2098557-16.patch7 KB
interdiff-15-16.txt2.93 KB

I love the direction! Thanks for all of the work.

I haven't tested this yet, but here are just a couple of quick changes to the patch from #8 + #15:

  1. If we're changing the functionality, we should change the comments so they continue to make sense.
  2. Drupal coding standards require spaces on either side of concatenation operator "."
  3. Testing $a != NULL or $a == NULL allows type coercion: array() == NULL is TRUE (see PHP's loose type comparison table). Either use strict comparison === and !== or, and this is my personal preference, use the functions isset() or is_null() which to my mind do a better job of self-documenting the code.

Again, thanks everyone for the hard work. If a few people can test this in different scenarios, I'll happily commit it.

sakiland’s picture

sakiland CreditAttribution: sakiland as a volunteer commented

#16 Works for me.

Tested with:

  • Features module 7.x-2.10
  • Views Distinct 7.x-1.0

Features module is patched with https://www.drupal.org/files/issues/features-rules_dont_revert-2701957-2... but that should have no influence to this functionality.

Thank you!

goron’s picture

goron CreditAttribution: goron commented

Patch in #16 worked well for me on latest views_distinct and features.

ciss’s picture

ciss CreditAttribution: ciss at yousign GmbH commented

Can someone please explain to me why a variable is used to store the settings, instead of a display extender plugin (that would also get rid of a lot of alter hooks)?

JimoftheSlowclick’s picture

JimoftheSlowclick CreditAttribution: JimoftheSlowclick commented

Has this passed testing? Or are you still looking for a few more test results? Quite a lot of time has passed since first opened, but glancing over the comments it looks good to go.

Gold’s picture

Gold
they/them
Location 20 minutes in the future
CreditAttribution: Gold at Catalyst IT commented 
+++ b/views_distinct.install
@@ -5,57 +5,34 @@
+function views_distinct_update_7100() {

On the topic of code standards this should be 7101. The '00' is reserved for D6 to D7 db updates.

Apart from that this looks good.

robpowell’s picture

robpowell
he/him
Location Boston
CreditAttribution: robpowell at Mediacurrent commented
FileSize
views_distinct-use_variables_as_storage-2098557-22.patch7 KB
interdiff_16_22.txt409 bytes

update 7100 to 7101, will test and post results.

pontus.froden’s picture

pontus.froden CreditAttribution: pontus.froden commented

Patch in #22 works fine for me.

Thank for that one, really helps when I realised why the settings wasn't pushed to prod.