Clean up code

@yannickoo:

Just running across the code, I have some questions/comments:

... $this->options['group'], <code>"$this->table_alias.tid", $this->value, ...

I find it a little bit misleading when reading "$this->table_alias.tid" as a string, because $this->table_alias seems like it's really a part of the query: wouldn't it be nicer to simply concatenate it?
Like this:
$this->table_alias . '.tid'
I think this way it's easier to find out which is a simple PHP variable.

The same for "$this->table_alias.nid".

I find the name of the check_for_alone function a little bit "mysterious". :) Maybe a better name should be given. (I know it wasn't your idea, it's just a suggestion.)

There's a typo in the patch:

-   * @param integer $tid
+   * @param ont $tid

ont --> int.

Btw. thanks for your efforts! :)

All functions should have the module name as prefix.

check_for_alone is class method.

check_for_alone is class method.

I didn't mean that. I just find the name of this method somehow wrong. :)
I'm not a native English speaker, I just feel that this is somehow linguistically incorrect, AND the function name itself doesn't even express what it really does (what is "alone", and in what sense is it "alone"? I know the purpose of this method, because the comments explain that, but I think the name itself should reflect its task too.)

Here are the comments:

  /**
   * Check that form hasn't any other exposed for user taxonomy terms filters.
   * Set object state which may be retrieved late by invoke the
   * function views_hst_filter_handler_filter::is_alone().
   *
   * @param $form_state
   *   The form state array.
   */

Please don't take it personally, I'm not the best English speaker either, but these sentences should be somehow improved. :)
Btw., I didn't find any methods named is_alone(), only the protected $is_alone variable, so the comment is misleading.
Maybe this method should rather just return a boolean, and be named is_alone(), and maybe only set the protected $is_alone; variable just once per request.

But this is only a brainstorming, not complete solutions, so I accept any relevant pros and cons. :) Thanks!

@Sk8erPeter:

I meant that this function shouldn't have the module name as prefix :)

And you're absolutely right: function name is poorly (

Comment is wrong (

Maybe we need different issue special for it?

Function check_is_alone() has moved to out class and renamed to views_hst_filter_is_alone().

I don't know whether it's fixed yet, but there's a strict warning when using the version that yannickoo linked in #1 with kervi's new features (e.g. I got this error message right after creating a taxonomy vocabulary).

Strict warning: Declaration of views_hst_filter_handler_filter::exposed_validate() should be compatible with that of views_handler::exposed_validate() in _registry_check_code() (line 3088 of ...\includes\bootstrap.inc)

http://i.imgur.com/VyMTM.png

Reason:
The header of the original function in views_handler looks like this:
function exposed_validate(&$form, &$form_state);
in class views_hst_filter_handler_filter, it looks like this:
function exposed_validate($form, &$form_state)
So the reference sign SHOULD be used in front of the $form variable.

--------------------

By the way, I think visibility of the class methods should be explicitly given, like for example public function admin_summary() instead of function admin_summary(), even if views_handler_filter doesn't use it - I think they just use it for backwards compatibility reasons (like first appearance of OOP code in PHP).

--------------------

@ser_house:

"I meant that this function shouldn't have the module name as prefix :)"
Yes, I know and I agree. But the method name itself should be more "talkative". :)

"Function check_is_alone() has moved to out class and renamed to views_hst_filter_is_alone()."
I don't understand, why is a global function better than the previous class method?

@Sk8erPeter:

I don't understand, why is a global function better than the previous class method?

Previously, it was unclear what the function does because it was return nothing. In fact it was "procedure", no "function". I think it was wrong.

Now the function is returning boolean flag which then assigned to variable of class (why return value if it is not used nowhere else?).

Since it doesn't work with any of variables of the class more, that is no method.

So, now this function has no relation to the views_hst_filter_handler_filter class.

And views_hst_filter_is_alone is more "talkative", doesn't it? :)

Yes, it is more talkative, but it could be even more talkative. :D Let me explain what I mean: the 'is_' part is OK, because it expresses that this function should return a boolean. BUT if someone looks at the code, he/she doesn't know what is really checked: what could be "alone"? A user? ;) A field? A text? A letter? A filter? Or what? A longer function name could be luckier.
Another approach: is "alone" the best name for that? Wouldn't "single" be more expressive? I think usually "single" and "multiple" words are more often used in this sense. But if it's not better, that's OK. :)

Maybe is_single_exposed_filter() or something like that as a function name would be more expressive, wouldn't it? (Of course, if it's a global function, the module's machine name should also be prepended.)

Btw. if this function is only used in the class, it shouldn't be taken out of that, and declared globally, it could remain a class method. Don't you agree?

Declare function as method just because it used only in the class even though it hasn't relation to the class? Strongly doubt that it is correct...

With regard to the function name maybe then views_hst_filter_is_single_filter will be better? What do you think about it?

Some functions moved in class, at least names became shorter...

views_hst_filter_is_alone is now views_hst_filter_handler_filter::is_single_filter