I've always wondered why you can only select the context substitutions as input for a views argument. However, context's already have an argument string - so why not allow passing that through?

This is very useful to allow contexts holding multiple values to be passed accordingly and works great with the patch from #1874006: Expose entity references as CTools relationships.

CommentFileSizeAuthor
#15 d7_ctools_views_panel_context_argument_15.patch8.21 KBjaperry
#12 d7_ctools_views_panel_context_argument_12.patch2.9 KBa.milkovsky
#6 d7_ctools_views_panel_context_argument_6.patch3.11 KBa.milkovsky
#4 interdiff-2180775-2-4-do-not-test.diff960 bytesa.milkovsky
#4 d7_ctools_views_panel_context_argument_4.patch3.03 KBa.milkovsky
#2 d7_ctools_views_panel_context_argument.patch2.35 KBfago
#1 d7_ctools_views_panel_context_argument.patch1.5 KBfago

Comments

fago’s picture

fago
Primary language German
Location Vienna
commented
StatusFileSize
new d7_ctools_views_panel_context_argument.patch1.5 KB
fago’s picture

fago
Primary language German
Location Vienna
commented
StatusFileSize
new d7_ctools_views_panel_context_argument.patch2.35 KB

I figured that we should handle not existing context arguments better, right now the code fails to include other subsequent arguments if it doesn't exist (as it goes with NULL).

Fixed that by correctly adding in the exception value if no context argument is there.

Status: Needs review » Needs work

The last submitted patch, 2: d7_ctools_views_panel_context_argument.patch, failed testing.

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
commented
Status: Needs work » Needs review
StatusFileSize
new d7_ctools_views_panel_context_argument_4.patch3.03 KB
new interdiff-2180775-2-4-do-not-test.diff960 bytes

Extended the patch for the 'users' case.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

Wolfgang Ziegler

+++ b/views_content/plugins/content_types/views_panes.inc
@@ -159,7 +159,12 @@ function views_content_views_panes_content_type_render($subtype, $conf, $panel_a
+              $args[] = isset($arguments[$id]['exception']['value']) ? $arguments[$id]['exception']['value'] : 'all';

You need to use this exception value, not just hard-code 'all'. 'all' is the default of views though, so this can be the fallback/default value. Else this looks good.

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
commented
Status: Needs work » Needs review
StatusFileSize
new d7_ctools_views_panel_context_argument_6.patch3.11 KB

Fixed

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Reviewed & tested by the community

Right - the same issue exists with NULL values of type 'user'.

Meanwhile we have this fix running on a customer site just fine + verified the fix works correctly. -> RTBC.

rivimey’s picture

rivimey
she/her
commented
Parent issue: » #2785697: Plan for CTools 7.x-1.11 release

Patch in #6 applies cleanly to 7.x-1.x

Patch looks ok, but I would appreciate another set of eyes on it, as there seems to be a possibility of regressions in custom modules because of slightly changed behaviour. However, I cannot point at any specific reason for caution, so good to go.

japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Needs work

This patch will need to be re-rolled due to #1910608: Ajax + Allow settings: Allowed settings lost on ajax (exposed forms/pager) being committed.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: #2785697: Plan for CTools 7.x-1.11 release »

This wasn't added to 1.11.

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
commented
Assigned: Unassigned » a.milkovsky

I will re-roll it

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
commented
Assigned: a.milkovsky » Unassigned
Status: Needs work » Needs review
StatusFileSize
new d7_ctools_views_panel_context_argument_12.patch2.9 KB

Re-rolled the patch #6

rivimey’s picture

rivimey
she/her
commented
Status: Needs review » Reviewed & tested by the community
Parent issue: » #2819121: Plan for CTools 7.x-1.12 release

Patch from #12 applies cleanly to 7.x-1.x.

I don't feel competent in this case to judge whether the fix is good, but the code looks reasonable.

Only thing to make it better would be some 'simpletest' tests... any takers?

japerry’s picture

japerry
Location KOMK
commented
Category: Task » Feature request
Parent issue: #2819121: Plan for CTools 7.x-1.12 release » #2828925: Plan for CTools 7.x-1.13 release

This looks good to me. Bumping for inclusion in our next feature release.

japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new d7_ctools_views_panel_context_argument_15.patch8.21 KB

Re-marking needs review. I re-rolled the last patch because it wouldn't apply anymore.

rivimey’s picture

rivimey
she/her
commented
Status: Needs review » Reviewed & tested by the community

Patch looks good to me.

japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Fixed

Great, tests out ok. Committed.

  • japerry committed 5958ba4 on 7.x-1.x authored by fago 
    Issue #2180775 by a.milkovsky, fago, japerry: Allow views argument from...
joaogomes_cd’s picture

joaogomes_cd commented

Gentlemen, the decision add the following lines of code using md5 with request time and rand():

    $this->view->dom_id = !empty($this->view->dom_id) ? $this->view->dom_id : md5($this->view->name . REQUEST_TIME . rand());
    if ($set_cache) {
      cache_set('view_panel_pane_' . $this->view->dom_id, $conf);
    }

Means that websites that have views_content module enabled and for some reason the dom_id is not present, will have the cache table be filled with gigabytes of caches in a few days, such as was our case, with aprox. 30GB of caches created in aprox. two weeks. I would strongly advise to roll back these changes. My suggestion is to NOT cache if dom_id is not present and not generate one based on MD5 with the request time and rand().

  • japerry committed 506b5c0 on 7.x-1.x 
    Revert "Issue #2180775 by a.milkovsky, fago, japerry: Allow views...
japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Needs work

Per comment #19, I'm reverting this issue for the 1.13 release. Also, it looks like the code causes a regression which was brought up in #2822472: Views content pane ignoring existing view args -- which will need to get tackled in this patch.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: #2828925: Plan for CTools 7.x-1.13 release » #2941916: Plan for CTools 7.x-1.14 release

This didn't get into 1.13, maybe it will get into 1.14.

We need to try getting this to RTBC.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: #2941916: Plan for CTools 7.x-1.14 release »
japerry’s picture

japerry
Location KOMK
commented
Status: Needs work » Closed (outdated)

Drupal 7 is no longer supported, closing.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.