php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #40021 assert_options(ASSERT_CALLBACK) does not return current setting
Submitted: 2007-01-04 13:25 UTC Modified: 2007-07-12 16:29 UTC
From: wharmby at uk dot ibm dot com Assigned: iliaa (profile)
Status: Closed Package: *General Issues
PHP Version: 5CVS-2007-07-12(snap) OS: Windows XP
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: wharmby at uk dot ibm dot com
New email:
PHP Version: OS:

 

 [2007-01-04 13:25 UTC] wharmby at uk dot ibm dot com
Description:
------------
This issue previously raised on Internals list but no
response so raising defect to track resolution of the problem. 

A colleague of mine is currently working on creating new
test cases to improve the coverage of the PHP test cases 
and whilst attempting to write new tests for the assert and
assert_options functions hit on a problem when attempting
to query the current setting of  ASSERT_CALLBACK. using the 
assert_options() api. 

With the supplied testcase the following behaviour was 
expected: 

	1) assert_options(ASSERT_CALLBACK, "new value") would return the old setting or NULL if no value set 
	2) assert_options(ASSERT_CALLBACK) would return the current setting or NULL if no value set

In all cases however the return value is "1", or rather a 
bool zval of TRUE.

Looking at the code this is no surprise as the code in 
assert.c which processes these requests unconditionally 
returns with RETURN_TRUE.  For all other assert options 
other than assert_options() function returns the old value 
of the specified option (or FALSE on errors) as documented in the PHP manual. 

Further investigation uncovered that the code actually did 
behave in the way we expected until it was changed to 
accept  the array(&$obj, "methodname") syntax back in July 2001 (revision 1.32 of assert.c). At this time the return was changed to be unconditionally TRUE. 

Unfortunately this makes writing a test case to query the current setting of the ASSERT_CALLBACK option impossible as assert_options() no longer returns any useable information for this option, i.e to code a testcase as below that sets a value and then checks its set as expected. 

If the code is modified as in the attached patch to instead return the zval for the current value for the ASSERT_CALLBACK option then we are able create the new testcase to verify the  assert_options() api. However, I am 
concerned there is a good reason this was not done when the 
code was changed back in 2001 although at the moment I myself cannot see one. 

However, even with the assert_options code modified such 
that assert_options(ASSERT_CALLBACK) returns the current 
setting of the option rather than TRUE if a script issues 
assert_options(ASSERT_CALLBACK) BEFORE the first call to 
assert() then rather than getting any php.ini setting of 
assert_callback NULL is returned instead.

This behaviour is caused by a recent change made under 
defect 39718 ( http://bugs.php.net/bug.php?id=39718). The 
problem is that the value of the global ASSERTG(cb) is not 
copied to ASSERTG(callback) until the first call to assert()
by a request so if the script includes a query on the 
setting BEFORE the first call to assert() then the value returned is the default INI setting, i.e. NULL, rather than 
any value specified in php.ini. The issues is easily 
addressed by moving the code that populates ASSERTG(callback) to a new RINIT function in assert.c. 

The following patches address both issues raised above:

assert.c:  http://pastebin.ca/304563
basic-functions.c: http://pastebin.ca/304566

The patches were built against the latest snap shot build 
(Jan 4 2007, 0730) and the fix has been tested on 
both Windows and Linux using both the supplied testcase and the existing PHPT tests for assert. 

If the attached patch is accepted then my colleague will then drop the improved PHP testcases for assert into CVS.


Andy Wharmby 
IBM United Kingdom Limited 

Reproduce code:
---------------
<?php

function andy() 
{
	echo "andy called\n";
}

function default_callback() 
{
	echo "default_calback called\n";
}
	
  $o = assert_options(ASSERT_CALLBACK);

  echo "Initial setting from php.ini is \"$o\"   ...it should be default_callback!!!\n";
  assert(0);
  
  $o= assert_options(ASSERT_CALLBACK, "andy");
  $n= assert_options(ASSERT_CALLBACK);
  echo "old setting is \"$o\"  new setting \"$n\"\n";
  assert(0);
 
?>

and the following php.ini setting: 

      assert.callback="default_callback" 

Expected result:
----------------
Initial setting from php.ini is "default_callback"   ...it should be default_callback!!!
default_calback called

Warning: assert(): Assertion failed in C:\Eclipse-PHP\workspace\Testcases\assertbug.php on line 16
old setting is "default_callback"  new setting "andy"
andy called

Warning: assert(): Assertion failed in C:\Eclipse-PHP\workspace\Testcases\assertbug.php on line 21

Actual result:
--------------
Initial setting from php.ini is "1"   ...it should be default_callback!!!
default_calback called

Warning: assert(): Assertion failed in C:\Eclipse-PHP\workspace\Testcases\assertbug.php on line 16
old setting is "1"  new setting "1"
andy called

Warning: assert(): Assertion failed in C:\Eclipse-PHP\workspace\Testcases\assertbug.php on line 21

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2007-01-12 15:51 UTC] iliaa@php.net
In this instance addition of RINIT done on every request for 
functionality of dubious use in a non-test environment is IMO 
outweighed by the performance considerations.

As such I am marking this issue as won't fix.
 [2007-01-15 15:28 UTC] wharmby at uk dot ibm dot com
Thanks for taking at look at this one Ilia. 

Having taken onboard your concerns regarding the performance impact of my initial patch for a function 
which I agree has little value outside of a testing environment could I ask you to consider the following 
alternative fix as without the ability to query the 
current setting of "callback" writing tests to verify 
this aspect of the API's behaviour is more difficult.

New patch: http://pastebin.ca/317353

This new fix has no impact on RINIT, whilst at the same time fixing the assert_options API to perform as documented in PHP manual allowing new tests to be written to verify the assert_options API. These new tests will be dropped into CVS by my colleague if the new fix is acceptable.
 [2007-01-15 15:45 UTC] iliaa@php.net
The patch won't work I'm afraid. The reason it won't is 
because a callback can be an array method in which case it is 
an array, not a string.
 [2007-01-15 16:42 UTC] wharmby at uk dot ibm dot com
When you say "callback can be an array method in which
case it is an array, not a string" are you referring to 
the fact that the callback routine can be defined as follows

  $newcb = array("foo","cccc");
  $o = assert_options(ASSERT_CALLBACK,$newcb);

where "cccc" is a method of class "foo". 

If so then for sure the returned value is no longer always
a string and the testcase has to deal with this using is_string(), is_array etc to determine just what has been returned.  Is this not acceptable ? The manual already defines the return value from assert_options() as "mixed" 
(some options result in a INT) although an extra line or two to explain that assert_options(ASSERT_CALLBACK) can return either a string or array would be required should this fix be accepted.

Here is the simple testcase I put together to test out the 
fix which I believe covers all possible use cases for this 
API and shows how I envisaged users dealing with the returned value: 
	http://pastebin.ca/317405

The output with fix applied is as follows: 
	http://pastebin.ca/317408
 [2007-01-16 09:36 UTC] wharmby at uk dot ibm dot com
One use case I did miss in my testcase and one that
highlights the need to return the current callback routine 
in non-testing environments. By modifying the  assert_options API to return the current setting of 
"callback" it is possible to save the old value so that it
can be restored at a later date, i.e if a routine wants  
to temporarily override the current callback routine. e.g

/* override current callback routine */
$o = assert_options(ASSERT_CALLBACK, "new-callback");

<do whatever>

/*restore callback */
assert_options(ASSERT_CALLBACK, $o);

This is not possible with current code.

My updated testcase: http://www.pastebin.ca/318252
 [2007-03-24 17:08 UTC] wharmby@php.net
Re-opening in order to get a response to by last 2 comments on this bug.
Bug was changed to WNF after my first suggested fix which I admit had a performance hit but my 2nd patch avoids these issues.

If this function is not to be fixed then I need to raise a defect against the menual as it incorrectly suggests that the current setting of the callback is returned.
 [2007-07-11 12:57 UTC] jani@php.net
Does this happen with PHP 5.2 (latest snapshot preferrably) ??
If so, update the version to "5CVS, 4CVS (yyyy-mm-dd)"
 [2007-07-12 13:42 UTC] wharmby at uk dot ibm dot com
Problem fixed in latest 5.2 snap shot (200707121230). Version updated. Thanks
 [2007-07-12 16:29 UTC] jani@php.net
Closed then.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Oct 31 23:01:28 2024 UTC