Reverse transform for validators [#1147326]

The user argument validator plugin forces the argument value to be a uid. This is a problem when using validation with the "User: Name" argument, since this argument builds its query around a name rather than a uid; what happens is that the argument passes validation, but the argument value is changed, meaning that the View appears empty (because its' where clause looks like "username = 1").

This could be solved by removing the line that changes the argument value in the views_plugin_argument_validate_user::validate_argument() method. This change makes sense; arguments shouldn't change during validation.

Comment	File	Size	Author
#24	1147326-follow-up.patch	692 bytes	dawehner
#21	1147326-21-alter_arguments_display.patch	6.88 KB	becw
#20	1147326-20-alter_arguments_display.patch	5.61 KB	dawehner
#19	1147326-19-alter_arguments_display.patch	5.38 KB	becw
#18	1147326-alter_arguments_display.patch	4.62 KB	dawehner
#13	1147326-alter_arguments_display-13.patch	1.72 KB	becw
#12	1147326-alter_arguments_display.patch	4.54 KB	dawehner
#11	user_validate-example_view-1147326-11.txt	4.28 KB	becw
	views_plugin_argument_validate_user.patch	531 bytes	becw

Comments

Comment #1

becw commented 4 May 2011 at 23:29

Title:	views_plugin_argument_validate_user changes the argument value	» views_plugin_argument_validate_user changes the argument value (patch)
Status:	Active	» Needs review

Comment #2

merlinofchaos commented 5 May 2011 at 00:11

Status:

Needs review

» Closed (won't fix)

If using that validator, just use the User: UID argument instead. You'll get a better performing query, which is the whole reason for doing it.

Comment #3

becw commented 5 May 2011 at 03:05

I want to have the user name in the URL, not the uid. The validator plugin will tranform a user name in the URL into a uid for the handler, which allows properly constructed URLs to work, but using an argument summary with the "User: UID" argument handler produces links with uids.

Removing this behavior from the validate plugin allows the "User: UID" argument handler to work when the validator is set to check for uids, and the "User: Name" argument handler to work when the validator is set to check for user names.

Unless the validator will intelligently transform the argument into the requested form (user name or uid), or preferably leave it untouched, it is broken.

Comment #4

becw commented 5 May 2011 at 03:08

Status:

Closed (won't fix)

» Active

Comment #5

merlinofchaos commented 5 May 2011 at 04:34

Status:

Active

» Fixed

The validator will accept a user name, and transforms it into a uid.

The URL will still be a name, because the validator accepts a name. Since it looked up the name and has a uid, using a uid in the query performs better.

Comment #6

becw commented 5 May 2011 at 12:41

Status:

Fixed

» Active

The "User: UID" argument handler won't build summary links that include a username.

Comment #7

merlinofchaos commented 5 May 2011 at 16:10

Status:

Active

» Closed (won't fix)

In that case, I guess you can't use the validator.

I will not be committing this patch.

Comment #8

merlinofchaos commented 5 May 2011 at 16:13

The reason I won't commit this patch is that it will break all existing arguments that are currently set up this way. Plus, it is totally the intended behavior. The issue, then, is that the validators don't currently participate in the summary argument creation, which has always been an issue that I've wanted to fix, and this isn't the fix for it.

Comment #9

becw commented 5 May 2011 at 18:42

So the validator is not usable for this argument handler. Ah, well.

Thanks for your response in #8. I appreciate you putting a moment into something that is, for you, a non-issue.

Comment #10

merlinofchaos commented 5 May 2011 at 18:53

I would like to fix it so that validators can participate in the summary building process. The way Views 3.x works, it actually should be relatively easy. There just needs to be a method on the validator that the argument can call to reverse the transformation, and then the validator needs to support transformation in both directions.

Comment #11

becw commented 5 May 2011 at 19:30

Status	File	Size
new	user_validate-example_view-1147326-11.txt	4.28 KB

@dereine -- here's an example view.

Comment #12

dawehner

German

commented 5 May 2011 at 20:33

Status:

Closed (won't fix)

» Needs work

Status	File	Size
new	1147326-alter_arguments_display.patch	4.54 KB

So here is a patch.

The patch has quite some todos:

* move the altering of the arguments out of the theme preprocess function, it's wrong there and has to be copied to work
* Find better function names, this doesn't seem to be the right one at the moment.

The only positive thing is that it works.

Comment #13

becw commented 8 May 2011 at 03:57

Status:

Needs work

» Needs review

Status	File	Size
new	1147326-alter_arguments_display-13.patch	1.72 KB

So, as you say, this patch has several issues. At the very least, we can not use the standard $var = &drupal_static(__FUNCTION__) pattern in an argument handler method, since __FUNCTION__ does not yield a variable name specific to this implementation of this method (so that argument handlers using different validators would end up using the same validator plugin); instead, a specific instantiation of the validator plugin should be stored as a property on the object.

My main problem with this patch, though, is that this code duplicates the purpose of an existing method: views_handler_argument::summary_argument(). I see that this is done for efficiency reasons--an argument handler's ::summary_argument() method is run for every link in the summary--but we could easily build all the url arguments on the first run of ::summary_argument() and then return the correct pre-built argument on each subsequent call. And really, how can one talk about efficiency and then run user_load_multiple()?

In this case, the argument we wish to use is already present--it is the 'name field' from the summary query. By changing the field alias used in retrieving the summary argument in views_handler_argument::summary_argument(). I've implemented this approach in the attached patch. The obvious issues with this patch are:

It doesn't introduce the desired argument validator transformation symmetry
It adds a special case to the views_handler_argument_user_uid that checks whether the 'user' validator is in use

Even if this feature were implemented in a way that refers to the the validator plugin for transformation, the argument handler's ::summary_argument() method should ultimately be used to pass the correct summary link argument to the theme layer.

Comment #14

dawehner

German

commented 12 May 2011 at 20:17

And really, how can one talk about efficiency and then run user_load_multiple()?

Please don't look at any kind of views which involves any kind of fieldapi data you will cry ...

Will review this tomorrow.
In general this patch does not provide this nice way to get the original input of an argument which earl suggested,
but your patch seems to be much simpler.

Comment #15

dawehner

German

commented 13 June 2011 at 18:00

Title:

views_plugin_argument_validate_user changes the argument value (patch)

» Reverse transform for validators

Better title

Comment #16

dawehner

German

commented 13 June 2011 at 18:13

We had quite some discussions on irc around this topic.

As result it will be more like patch #12 because we can't rely on the data availible in a summary_alias.

Comment #17

dawehner

German

commented 14 June 2011 at 18:59

New name: process_summary_arguments

Comment #18

dawehner

German

commented 14 June 2011 at 19:15

Status	File	Size
new	1147326-alter_arguments_display.patch	4.62 KB

Okay here is a new version which

* keeps the things in the preprocess, because that's not really such bad and there aren't many summary plugins in the wild
* renamed the function and changed the comments a bit.

@becw:
Please test the patch.

Comment #19

becw commented 14 June 2011 at 20:16

Status:

Needs review

» Needs work

Status	File	Size
new	1147326-19-alter_arguments_display.patch	5.38 KB

This patch mostly works. There are two remaining issues:

the use of the &drupal_static(__FUNCTION__) pattern in an argument handler method (see my comment #13 above). I've rerolled the patch with this change.
the "jump menu" summary display plugin uses argument handlers' ::summary_argument() method directly, which will yield bad URLs if used with the user argument validator 'username' setting--so this issue is still present when using the "jump menu" summary style. I've added a @TODO to this rerolled patch, but I haven't actually come up with a solution here.

Comment #20

dawehner

German

commented 14 June 2011 at 20:27

Status:

Needs work

» Needs review

Status	File	Size
new	1147326-20-alter_arguments_display.patch	5.61 KB

ups the first one was left.

About the second one, here is an update of the patch, which works for this view:


$view = new view;
$view->name = 'summary';
$view->description = '';
$view->tag = 'default';
$view->base_table = 'node';
$view->human_name = 'summary';
$view->core = 7;
$view->api_version = '3.0-alpha1';
$view->disabled = FALSE; /* Edit this to true to make a default view disabled initially */

/* Display: Master */
$handler = $view->new_display('default', 'Master', 'default');
$handler->display->display_options['title'] = 'summary';
$handler->display->display_options['access']['type'] = 'perm';
$handler->display->display_options['cache']['type'] = 'none';
$handler->display->display_options['query']['type'] = 'views_query';
$handler->display->display_options['query']['options']['query_comment'] = FALSE;
$handler->display->display_options['exposed_form']['type'] = 'basic';
$handler->display->display_options['pager']['type'] = 'full';
$handler->display->display_options['pager']['options']['items_per_page'] = '10';
$handler->display->display_options['style_plugin'] = 'jump_menu';
$handler->display->display_options['style_options']['hide'] = 0;
$handler->display->display_options['style_options']['path'] = 'title';
$handler->display->display_options['style_options']['default_value'] = 0;
$handler->display->display_options['row_plugin'] = 'node';
/* Relationship: Content: Author */
$handler->display->display_options['relationships']['uid']['id'] = 'uid';
$handler->display->display_options['relationships']['uid']['table'] = 'node';
$handler->display->display_options['relationships']['uid']['field'] = 'uid';
/* Field: Content: Title */
$handler->display->display_options['fields']['title']['id'] = 'title';
$handler->display->display_options['fields']['title']['table'] = 'node';
$handler->display->display_options['fields']['title']['field'] = 'title';
$handler->display->display_options['fields']['title']['label'] = '';
$handler->display->display_options['fields']['title']['alter']['alter_text'] = 0;
$handler->display->display_options['fields']['title']['alter']['make_link'] = 0;
$handler->display->display_options['fields']['title']['alter']['absolute'] = 0;
$handler->display->display_options['fields']['title']['alter']['word_boundary'] = 0;
$handler->display->display_options['fields']['title']['alter']['ellipsis'] = 0;
$handler->display->display_options['fields']['title']['alter']['strip_tags'] = 0;
$handler->display->display_options['fields']['title']['alter']['trim'] = 0;
$handler->display->display_options['fields']['title']['alter']['html'] = 0;
$handler->display->display_options['fields']['title']['hide_empty'] = 0;
$handler->display->display_options['fields']['title']['empty_zero'] = 0;
$handler->display->display_options['fields']['title']['link_to_node'] = 1;
/* Sort criterion: Content: Post date */
$handler->display->display_options['sorts']['created']['id'] = 'created';
$handler->display->display_options['sorts']['created']['table'] = 'node';
$handler->display->display_options['sorts']['created']['field'] = 'created';
$handler->display->display_options['sorts']['created']['order'] = 'DESC';
/* Contextual filter: User: Uid */
$handler->display->display_options['arguments']['uid']['id'] = 'uid';
$handler->display->display_options['arguments']['uid']['table'] = 'users';
$handler->display->display_options['arguments']['uid']['field'] = 'uid';
$handler->display->display_options['arguments']['uid']['relationship'] = 'uid';
$handler->display->display_options['arguments']['uid']['default_action'] = 'summary';
$handler->display->display_options['arguments']['uid']['default_argument_type'] = 'fixed';
$handler->display->display_options['arguments']['uid']['default_argument_skip_url'] = 0;
$handler->display->display_options['arguments']['uid']['summary']['number_of_records'] = '0';
$handler->display->display_options['arguments']['uid']['summary']['format'] = 'jump_menu_summary';
$handler->display->display_options['arguments']['uid']['summary_options']['base_path'] = 'foobar';
$handler->display->display_options['arguments']['uid']['summary_options']['count'] = '1';
$handler->display->display_options['arguments']['uid']['summary_options']['hide'] = 0;
$handler->display->display_options['arguments']['uid']['summary_options']['default_value'] = 0;
$handler->display->display_options['arguments']['uid']['specify_validation'] = 1;
$handler->display->display_options['arguments']['uid']['validate']['type'] = 'user';
$handler->display->display_options['arguments']['uid']['validate_options']['type'] = 'name';
$handler->display->display_options['arguments']['uid']['validate_options']['restrict_roles'] = 0;
$handler->display->display_options['arguments']['uid']['break_phrase'] = 0;
$handler->display->display_options['arguments']['uid']['not'] = 0;
/* Filter criterion: Content: Published */
$handler->display->display_options['filters']['status']['id'] = 'status';
$handler->display->display_options['filters']['status']['table'] = 'node';
$handler->display->display_options['filters']['status']['field'] = 'status';
$handler->display->display_options['filters']['status']['value'] = 1;
$handler->display->display_options['filters']['status']['group'] = 0;
$handler->display->display_options['filters']['status']['expose']['operator'] = FALSE;

/* Display: Page */
$handler = $view->new_display('page', 'Page', 'page');
$handler->display->display_options['path'] = 'summary';
$translatables['summary'] = array(
  t('Master'),
  t('summary'),
  t('more'),
  t('Apply'),
  t('Reset'),
  t('Sort by'),
  t('Asc'),
  t('Desc'),
  t('Items per page'),
  t('- All -'),
  t('Offset'),
  t('author'),
  t('All'),
  t('Page'),
);

Comment #21

becw commented 14 June 2011 at 20:52

Status	File	Size
new	1147326-21-alter_arguments_display.patch	6.88 KB

Looks good!

One last request: can we add a ::process_summary_arguments() method for the taxonomy term validator plugin, too? I've rolled it into the patch, which is my only change from #20.

Comment #22

dawehner

German

commented 14 June 2011 at 21:01

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

Commited to 7.x-3.x

This needs some rerole because there is dbtng involved.

Comment #23

dawehner

German

commented 31 August 2011 at 21:29

Version:	6.x-3.x-dev	» 7.x-3.x-dev
Status:	Patch (to be ported)	» Active

+++ b/modules/taxonomy/views_plugin_argument_validate_taxonomy_term.incundefined
@@ -184,4 +184,31 @@ class views_plugin_argument_validate_taxonomy_term extends views_plugin_argument
+      if (!empty($vocabularies)) {
+        $query->leftJoin('taxonomy_vocabulary', 'tv', 'td.vid = tv.vid');

Just realized during porting the vocabularies can't be defined at this scope.

Let's get this first fixed in d7 and backport everything.

Comment #24

dawehner

German

commented 31 August 2011 at 21:31

Status:

Active

» Needs review

Status	File	Size
new	1147326-follow-up.patch	692 bytes

Here is a patch for the fix of the bug.

Comment #25

dawehner

German

commented 10 September 2011 at 22:56

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

Okay commited so let's concetrate on the backport.

Comment #26

chris matthews commented 14 March 2019 at 17:55

Issue summary:	View changes
Status:	Patch (to be ported)	» Closed (outdated)

The Drupal 6 branch is no longer supported, please check with the D6LTS project if you need further support. For more information as to why this issue was closed: #3030347: Plan to clean process issue queue

Reverse transform for validators

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Comment #24

Comment #25

Comment #26

News items

Our community

Documentation

Drupal code base

Governance of community