ZABBIX 1.8 & Trunk

patch applies and server compiles, but adding a new step in the frontend fails with ;

name: ReferenceError
message: Statement on line 14: Undefined variable: follow

Sorry, I forgot to include the adapted JavaScript function in the first patch.
Attaching the fixed version.

Sigh. Final fix (SQL query failed when 'Follow' was unchecked for a new step)

adding a new step with 3rd patch fails with "Operation cannot be performed due to unauthorized request" - i sort of refreshed all cached stuff, shouldn't impact this

When exactly does it fail? When saving the new step?

The only occurence to the string "Operation cannot be performed due to unauthorized request" is when the session failed, but I'm unable to reproduce it.

Checked twice with the current patch, checked out new trunk (r8670), applied the patched, used that. All working fine.
Have you tried logging out and logging in again?

ok, cleared everything, reapplied the patch, and now operation error is gone. few more things

1. editing a step and unchecking 'follow' results in a failed step saving ;

Syntax error while loading: line 27 of inline script at http://localhost/zabbix-1.8/popup_httpstep.php :
Expected token: ';'
Notice: Undefined index: follow in /usr/local/apache2/htdocs/zabbix-1.8/p

2. it looks like follow is not using 'isset(

)' - which is suppose only saves stuff it has been changed - is that being done on purpose ?

I'm starting to think that a checkbox might be the wrong parameter type for this option.

When the checkbox is not set, the form parameter "follow" will simply be unset (i.e. not submitted at all). This explains the PHP notice "Undefined index". The same goes for isset(

) - by adding this parameter to the variables to check, it makes the option mandatory and ZABBIX GUI will complain about a missing parameter if the checkbox is unset.

I guess, making that parameter a HTML select field ("Follow" -> Value 1, "Don't follow" -> Value 0) instead of the checkbox will work better and make things simpler and less error prone.

I'll get back to the code this evening. Thanks for your input.

oh, usability wise checkbox is the only sane solution - if you proceed, i'd suggest looking for some way to do that - maybe take a look at profile page, for example, where checkboxes are used

OK, now this should be (hopefully) the last and final patch.

Sorted out the issue with undefined variables by using get_request() as opposed to _$REQUEST[], this also eliminated the need for the dirty crotch in include/httptest.php. Also added a validation IN('0,1') to popup_httpstep.php, as how profile.php handles checkboxes.

very nice, no more errors and works as expected. one final nitpick
this affects following redirects only, right ? so maybe it's better to name the option "Follow redirects" to reduce possible future confusion

Hi,

thanks again for the input. You are right, "follow" is pretty generic and could cause some confusion and/or be a source for errors in further development. I modified the patch in the following way:

• Renamed the locale to be S_FOLLOW_REDIRECTS instead of S_FOLLOW
• Renamed the global variables (passed through $_REDIRECT) from 'follow' to 'follow_redirects'

Some of the local variables (e.g. function arguments) have been kept as "follow", as well as database field and C code. I think that is sufficient, as those functions are specific enough to rectify a variable simply named "follow".

Oh, and of course the locale in default en_gb is now "Follow redirects", instead of just "Follow".

very nice. everything patches, compiles and works without any warnings.
this won't go into 1.8 because of the required db schema change, but i hope to see it in trunk soon.

Great

I did not expect this to appear in 1.8, but I'd be glad to see it in trunk, too
I still have some couple of ideas to improve the WEB monitoring module, so expect some more stuff coming.
I'll try to keep the work at your side as minimal as possible

Please also note ZBXNEXT-80 (formerly ZBX-729) that I created and for which I provided a patch in February (and a new v1.8 patch today).

yes, i'm very sorry for not spotting the duplication earlier. i searched the tracker, but older issue did not pup up in my results

I see it practical; we have two patches for the same feature, so chances are that one of the patches will make it in upstream. Personally, I don't care whether it is the patch by Jim in ZBXNEXT-80 or mine that ends up in Trunk. All I care about is that the feature will be available in future versions of Zabbix, since it will make life a little easier.

Anyhow, through the cycles spent with the patch, I learned quite a bit about Zabbix internals (thanks to the input of richlv) and now know how to perform the QA for my next patches on my own

My only concern when comparing this with my patch in ZBXNEXT-80 is that this patch defaults follow to false, whereas the current behavior in Zabbix is to follow redirections. This may cause existing setups to break when upgrading. This needs to be either very clearly mentioned in the release notes, or the default value in the patch should be changed to true so that existing installations continue to work as they do today. I vote for the latter. That way only people who need to NOT follow redirections will have to make changes rather than everyone else (the majority).

completely agreed - default should be to follow redirects - isn't that the case ? original text says :
"The default setting for "Follow" when creating a new step is "On""

additionally, it would need proper updates to zabbix schema (using same field types etc), and upgrade patch should be created as well for mysql, pgsql and oracle.

It is correct that the ALTER TABLE above sets the default to 1. I was more concerned about these:

--- curl-follow-5.patch 2010-02-11 08:01:49.715502741 -0600
+++ curl-follow-5.patch.new     2010-02-11 08:01:35.889358357 -0600
@@ -100,7 +100,7 @@
                foreach($steps as $stepid => $s){
                        if(!isset($s['name']))          $s['name'] = '';
                        if(!isset($s['timeout']))       $s['timeout'] = 15;
-+                      if(!isset($s['follow_redirects']))      $s['follow_redirects'] = 0;
++                      if(!isset($s['follow_redirects']))      $s['follow_redirects'] = 1;
                        if(!isset($s['url']))           $s['url'] = '';
                        if(!isset($s['posts']))         $s['posts'] = '';
                        if(!isset($s['required']))      $s['required'] = '';
@@ -150,7 +150,7 @@
                        foreach($steps as $sid => $s){
                                if(!isset($s['name']))          $s['name'] = '';
                                if(!isset($s['timeout']))       $s['timeout'] = 15;
-+                              if(!isset($s['follow_redirects']))      $s['follow_redirects'] = 0;
++                              if(!isset($s['follow_redirects']))      $s['follow_redirects'] = 1;
                                if(!isset($s['url']))           $s['url'] = '';
                                if(!isset($s['posts']))         $s['posts'] = '';
                                if(!isset($s['required']))      $s['required'] = '';
@@ -195,7 +195,7 @@
                        zbx_jsvalue($_REQUEST['dstfrm']).','.
                        zbx_jsvalue($_REQUEST['name']).','.
                        zbx_jsvalue($_REQUEST['timeout']).','.
-+                      zbx_jsvalue(get_request('follow_redirects', 0)).','.
++                      zbx_jsvalue(get_request('follow_redirects', 1)).','.
                        zbx_jsvalue($_REQUEST['url']).','.
                        zbx_jsvalue($_REQUEST['posts']).','.
                        zbx_jsvalue($_REQUEST['required']).','.
@@ -220,7 +220,7 @@
                        zbx_jsvalue($_REQUEST['stepid']).','.
                        zbx_jsvalue($_REQUEST['name']).','.
                        zbx_jsvalue($_REQUEST['timeout']).','.
-+                      zbx_jsvalue(get_request('follow_redirects', 0)).','.
++                      zbx_jsvalue(get_request('follow_redirects', 1)).','.
                        zbx_jsvalue($_REQUEST['url']).','.
                        zbx_jsvalue($_REQUEST['posts']).','.
                        zbx_jsvalue($_REQUEST['required']).','.

Hello and thanks for your feedback.

The problem with the HTTP POST variables regarding checkboxes is, if a checkbox is not set, the corresponding $_REQUEST variable in PHP will not be set at all. That is why there is a default of '0' for all request – to have a 'real' value instead of no value at all. So all the '0' defaults in the patch just prevent a NULL insert into the database (which would effectively turn into a '1' due to the DEFAULT condition in the SQL statement). Existing tests in the database will inherit the DEFAULT 1 value, so that there is no change in behaviour for those existing WEB tests and they will still follow any redirect encountered.

If the checkbox is set, the defaults for get_request() and all other evaluations of $_REQUEST will not apply at all and take the value '1'. Only if the checbox is unchecked, we insert a '0' into the database.

So I think the "patch to my patch" is not doing what it should and probably would be counter-productive.

Oh, and btw - the default value for the checkbox in the web interface is "checked", so when adding a new step, the value will be '1'. Same goes for editing any previously existing check, because the value in the database will be '1' and thus, the checkbox is "checked".
As for the schema updates and finalizing the patch, I have very little time at the moment I will update the patch when time permits, but I guess it's still quite a long road to ZABBIX 2.0 anyway

Here's a (slightly) updated patch against 1.8.2. (curl-follow-6.patch)

Can we use this patch against 1.8.9

See also ZBX-2072 (redirect ignored when using post variables)

closing as a duplicate of ZBXNEXT-282 - everybody interested, please test that functionality