Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 1.2.3

Bug #16339 Static variable $id in addToken() should be a instance variable
Submitted: 2009-06-18 09:23 UTC
From: rodrigosprimo Assigned:
Status: Bogus Package: Text_Wiki (version CVS)
PHP Version: 5.2.9 OS: Linux
Roadmaps: (Not assigned)    

 [2009-06-18 09:23 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
Description: ------------ I'm not sure if this is a bug or a change request. The only reason I'm marking this as a bug is that the actual behavior is the opposite of the behavior described by a code comment. The addToken() method of the class Text_Wiki use a static class variable called $id to store the number of tokens created. The problem is that as it is a static class variable its value is the same for different class instances (although a comment above the variable definition alerts that the value will be the same only for the same class instance) and as it is defined inside a method it is not accessible outside this method. I found this to be a problem while writing tests for the Mediawiki parser. I create a different instance of Text_Wiki for each test but regardless of using a different instance the value of the $id variable is the same for all tests and thus the tests depend on each other. Is this a desirable behavior for some reason? As I wasn't able to found a reason for that I'm attaching a patch that change $id to an instance variable called $tokenId. I'm not sure if I was able to explain the problem due to my limitations with English, please let me know if you are not able to understand this report. Test script: --------------- <?php require_once 'PHPUnit/Framework.php'; require_once 'Text/Wiki.php'; class Text_Wiki_Test extends PHPUnit_Framework_TestCase { function testDifferentTextWikiInstanceShareSameTokenIdVariable() { $obj1 = Text_Wiki::factory(); $obj2 = Text_Wiki::factory(); $this->assertNotEquals(spl_object_hash($obj1), spl_object_hash($obj2)); $options = array('type' => 'someType', 'anotherOption' => 'value'); $id1 = $obj1->addToken('Test', $options, true); $id2 = $obj2->addToken('Test', $options, true); $this->assertEquals($id1, $id2); } } ?> Expected result: ---------------- Both assertions should pass meaning that we are dealing with two different objects (first assertion) and that each of then is using a different variable to store the count of tokens so the value of the first token id for both is equal, is 0 (second assertion). Actual result: -------------- As can be seem below with the output of PHPUnit only the first assertion is passing. We are dealing with two different objects but as the second assertion fails they are using the same $id variable (that is why the value of $id1 is 0 and the value of $id2 is 1). PHPUnit 3.3.17 by Sebastian Bergmann. F There was 1 failure: 1) testDifferentTextWikiInstanceShareSameTokenIdVariable(Text_Wiki_Test) Failed asserting that <integer:1> matches expected value <integer:0>. /home/rodrigo/devel/textwiki/tests/delme.php:13 FAILURES! Tests: 1, Assertions: 2, Failures: 1.


 [2009-06-18 09:31 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
I don't know why there was a problem with the indentation of the test script so I'm sending it again. require_once 'PHPUnit/Framework.php'; require_once 'Text/Wiki.php'; class Text_Wiki_Test extends PHPUnit_Framework_TestCase { function testDifferentTextWikiInstanceShareSameTokenIdVariable() { $obj1 = Text_Wiki::factory(); $obj2 = Text_Wiki::factory(); $this->assertNotEquals(spl_object_hash($obj1), spl_object_hash($obj2)); $options = array('type' => 'someType', 'anotherOption' => 'value'); $id1 = $obj1->addToken('Test', $options, true); $id2 = $obj2->addToken('Test', $options, true); $this->assertEquals($id1, $id2); } }
 [2009-06-18 09:32 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
Also I'm not able to attach the patch so I'm sending it below: Index: Wiki.php =================================================================== RCS file: /repository/pear/Text_Wiki/Text/Wiki.php,v retrieving revision 1.52 diff -u -r1.52 Wiki.php --- Wiki.php 17 Dec 2007 16:03:48 -0000 1.52 +++ Wiki.php 18 Jun 2009 03:24:12 -0000 @@ -371,6 +371,13 @@ var $_blocks; /** + * A numerical identifier for tokens + * + * @param integer + */ + var $tokenId = -1; + + /** * * Constructor. * @@ -1224,18 +1231,13 @@ // increment the token ID number. note that if you parse // multiple times with the same Text_Wiki object, the ID number // will not reset to zero. - static $id; - if (! isset($id)) { - $id = 0; - } else { - $id ++; - } - + $this->tokenId++; + // force the options to be an array settype($options, 'array'); // add the token - $this->tokens[$id] = array( + $this->tokens[$this->tokenId] = array( 0 => $rule, 1 => $options ); @@ -1248,10 +1250,10 @@ // return a value if ($id_only) { // return the last token number - return $id; + return $this->tokenId; } else { // return the token number with delimiters - return $this->delim . $id . $this->delim; + return $this->delim . $this->tokenId . $this->delim; } }
 [2009-06-19 09:08 UTC] justinpatrin (Justin Patrin)
I don't think this is a bug. None of the code that I see assumes that the keys are monotonically increasing or contiguous so multiple copies of Text_Wiki run in "tandem" would still work just fine. Unless you can show that this is causing issues (i.e. a real bug) then I would like to leave this as it already is.
 [2009-06-19 09:09 UTC] justinpatrin (Justin Patrin)
-Status: Open +Status: Bogus
See previous comment.
 [2009-06-20 00:29 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
Hi Justin, thanks for your feedback. Your comment made me realize that indeed this is not a bug (or at least I'm also unable to think of a problematic situation). But anyway I don't see a good reason for keeping $id as a static variable inside addToken(). This might be only because I'm not familiar with this concept (I don't know of other programming language that implement this). From your point of view there is any advantage in using $id as a static variable inside addToken()? My problem is that I'm having a hard time trying to find a way to write tests for the parsers because of this. Please take a look at the last version of the file tests/Text_Wiki_Parse_Mediawiki_Tests.php. The tests in this file will fail without the change I'm proposing. Do you have an idea on how can we test the parsers with the actual design of Text_Wiki (specifically the way $id is defined in addToken())? If we can't think of a way to write tests neither think of advantages of this implementation, it might be a good idea to change it. IMHO, it feels much more natural to have a instance variable called $tokenId to keep the number of tokens than to use a static variable inside a method.
 [2009-06-20 00:32 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
PS: I notice that on the second paragraph of the description I wrongly refer to static variable inside the function as static class variable twice.
 [2009-06-20 05:04 UTC] justinpatrin (Justin Patrin)
I'm not sure why you're trying to test that the entire array is equal. To fix your tests you could simply do an array_values() on the tokens array.
 [2009-07-03 17:08 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
Hi Justin, I will use the solution of testing only array_values() although I don't think this is the better solution. We are only testing part of the side effect of the addToken() function. Maybe in the future when we have solid unit tests for all the Text_Wiki core we can switch without worries as I think that using this static method variable is a strange design solution (and sometimes the beauty of tests is pointing out stuff like these) Rodrigo
 [2009-07-03 22:52 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
Hi again Justin, I realize that I was wrong. It is not possible to test everything using the array_values hack. With your suggestion it is possible to test the side effect of the process method of each parse class (which is setting the tokens array). But it is not possible to test the return value of the process method. For example take a look at Text_Wiki_Parse_Heading. This method return a string with the heading content enclosed by the start and end token. It is not possible to test the return value of the process method without considering the token id number. I'm sorry to keep bothering you with this question but it is really holding back my work and I see no reason for keeping things the way they are. It is a simple change that will improve Text_Wiki design and will make it more test friendly. Don't you think?
 [2009-07-04 02:07 UTC] justinpatrin (Justin Patrin)
It seems to me that changing this could cause issues with doing multiple-parsing. For example, if you parse with one dialect then parse the same text with another dialect (and another parser object) then the numbers would collide. Again, I don't see the issue in testing. You don't need to test that the numbers are exact. An easy way to test the header tokens, for instance, would be to use a regex which checks for the tokens (with any number within them) and the text between them.
 [2009-07-05 00:02 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
The following patch has been added/updated: Patch Name: token_id_change Revision: 1246734129 URL:
 [2009-07-05 00:03 UTC] rodrigosprimo (Rodrigo Sampaio Primo)
Hi Justin, I'm sorry for being so persistent with this issue. I have to agree with you that know you have proposed a workaround. On the other hand I'm really interested about this discussion as it is being a good oportunity for me to learn more about Text_Wiki. So if you are too busy and don't have more time to put on this that is ok, really. I will adopt the regex solution for the id number. But if you have some time I would like to ask you to take a look at the tests I have just commited on tests/Text_Wiki_Tests.php (testTokenIdForMultipleParsingDontCollideWithSameObjects and testTokenIdForMultipleParsingDontCollideWithDifferentObjects) and tell me if they represent correctly your concerns about the change. If not, I did not understood what do you mean. I manage to add the patch with the change I have proposed, just in case you want to try the tests without and with the change. Thanks a lot for all your help, Rodrigo.