"Let's say I have a field on a node, 'color', and I want to find other nodes with the same color in a block."

CommentFileSizeAuthor
#9 views-1182808-9.patch2.84 KBtim.plunkett
#6 views-1182808-6.patch2.83 KBtim.plunkett
#3 views-1182808-3.patch2.69 KBtim.plunkett
#1 views-1182808-1.patch2.59 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
views-1182808-1.patch2.59 KB

The strings need work.
Choosing 1 in the select results in arg(0).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Category: feature » task
Status: Needs review » Active

That's a valid task :)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
views-1182808-3.patch2.69 KB

"Path component" seems to be the best label, it made sense to the 3 people I polled for a better string :)
I hope the description is helpful, not as attached to that. It clarifies what's happening here, but might cause confusion in contrast to arg()

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work 
+++ b/plugins/views_plugin_argument_default_raw.incundefined
@@ -0,0 +1,33 @@
+      '#description' => t('E.g., on the page admin/structure/types, the 3rd path component is "types".'),

So the question is do we start at 0 or 1

+++ b/plugins/views_plugin_argument_default_raw.incundefined
@@ -0,0 +1,33 @@
+    if ($arg = check_plain(arg($this->options['index']))) {

That's wrong, you want index -1 according to your description.

Powered by Dreditor.

tim.plunkett’s picture

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

Given "admin/structure/types", arg(2) == 'types'.
range(1, 10) generates an array like

0 => 1,
1 => 2,
2 => 3,
...
9 => 10

So selecting 3 in the interface will pass 2 to arg(), magically avoiding the 0-based confusion for end users.

...right?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1182808-6.patch2.83 KB

Now with code comments to alleviate the above confusion.
I'm up for backporting this, once it's in.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented 
+  /**
+   * Using range(1, 10) will create an array keyed 0-9, which allows arg()
+   * to properly function since it is also zero-based.
+   */

This isn't an appropriate comment for doxygen, which should be explaining what the function/method does with a one liner and then a deeper explanation. For inherited methods doxy isn't really needed. The comment should be inside the funciton.

The #description should explicitly say that numbering starts from 1, and then follow with the e.g

+  function get_argument() {
+    if ($arg = check_plain(arg($this->options['index']))) {
+      return $arg;
+    }

Just doublechecking. ARe you sure we check plain the argument? I don't *think* we assume that arguments are safe since they're user input.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
views-1182808-9.patch2.84 KB

You were right, check_plain() breaks it for special characters, so I removed it.
I hope the new comments were what closer to you wanted? If not I guess we can work it out in IRC tomorrow.

joachim’s picture

joachim CreditAttribution: joachim commented 
 plugins/views_plugin_argument_default_raw.inc |   35 +++++++++++++++++++++++++
...
+      'raw' => array(

Maybe use a better key here...? 'Path component' is clearer than raw, we could use that in the code as well. Or am I nitpicking?

Powered by Dreditor.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

Committed to 7.x-3.x. Needs porting to 6.x (should just be the parentage change) and can go there too.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Patch (to be ported) » Fixed

Backported some time ago.

Status: Fixed » Closed (fixed)

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