<div><br></div><div><div><span style="color: rgb(160, 160, 168); ">On Wednesday, 27 March 2013 at 7:31 PM, Dieter Adriaenssens wrote:</span></div></div>
<blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;">
<span><div><div><div>2013/3/27 Ayush Chaudhary <<a href="mailto:ayushchd@gmail.com">ayushchd@gmail.com</a>>:</div><blockquote type="cite"><div><div><br></div><div>On Wednesday, 27 March 2013 at 5:29 PM, Dieter Adriaenssens wrote:</div><div><br></div><div>2013/3/26 Dieter Adriaenssens <<a href="mailto:dieter.adriaenssens@gmail.com">dieter.adriaenssens@gmail.com</a>>:</div><div><br></div><div>2013/3/24 Marc Delisle <<a href="mailto:marc@infomarc.info">marc@infomarc.info</a>>:</div><div><br></div><div>Le 2013-03-23 11:07, Ayush Chaudhary a écrit :</div><div><br></div><div>Hi</div><div><br></div><div><br></div><div>On Saturday, 23 March 2013 at 8:24 PM, Marc Delisle wrote:</div><div><br></div><div>Le 2013-03-23 08:47, Ayush Chaudhary a écrit :</div><div><br></div><div>Hi,</div><div><br></div><div><br></div><div><br></div><div>Hello,</div><div><br></div><div><br></div><div>I was going through the existing tests. The test 'testIsHttps'</div><div>under test/classes/PMA_Config_test.php looks wrong as isHttps()</div><div>should be returning true for the second test case. However, the</div><div>function isHttps() uses a local static variable, which is why the</div><div>test is passing as in the construct, the PmaAbsoluteUri is</div><div>initialised with null and isHttps() is hence returning false in</div><div>all subsequent calls.</div><div><br></div><div>I am not sure why a local static variable was used here.</div><div><br></div><div><br></div><div>To avoid retesting the condition for nothing?</div><div><br></div><div>A better test for this method would be if we could reset the</div><div>static variable before each assertion. Since its a local static</div><div>variable, I couldn't find a proper way to reset the static</div><div>variable. Should we not move the static variable in the scope of</div><div>the class? In fact, even if we don't use a static variable, we</div><div>could just make is_https a class member and initialise it to NULL</div><div>in the constructor. This would eventually achieve the same result</div><div>in a neater way.</div><div><br></div><div><br></div><div><br></div><div>Makes sense, but watch out: isHttps() and detectHttps() are both</div><div>using $is_https, for different goals.</div><div><br></div><div><br></div><div>Also, what approach do we take for resetting static variables if</div><div>needed for unit testing? One thing I came across was using</div><div>Reflection_Helper::set_static_variable($class, $name,</div><div>$new_value)?</div><div><br></div><div><br></div><div><br></div><div>Yes, this approach seems interesting :</div><div><a href="http://tech.blog.box.com/2012/07/unit-testing-with-static-variables/">http://tech.blog.box.com/2012/07/unit-testing-with-static-variables/</a></div><div><br></div><div><br></div><div>I think the purpose of the static variable is to only let isHttps()</div><div>execute once in the lifecycle of a PMA_Config object. Regarding, the</div><div>use of $is_https in detectHttps(), the copy of $is_https in</div><div>detectHttps() is completely independent of the one in isHttps()</div><div>because isHttps() creates a local copy of the variable. So</div><div>detectHttps() would not need to be changed at all.</div><div><br></div><div><br></div><div>Yes, but on a maintenance perspective, it's confusing to be using the</div><div>same variable in one file, with two different purposes.</div><div><br></div><div><br></div><div>Making $is_https a member variable and using the member variable in</div><div>isHttps() will help to make tests more exhaustive as we will be able</div><div>to test for 1. non-https input 2. https input 3. input when is_https</div><div>is already set (this is the only case that is being tested right</div><div>now)</div><div><br></div><div>Also, regarding that approach, the issue here is that the static copy</div><div>has scope only within the function isHttps(), so it does not seem</div><div>possible to reset it using Reflection_Helper.</div><div><br></div><div>Please let me know, if this would be fine. I can then write a quick</div><div>patch to fix this, and also improve testIsHttps()</div><div><br></div><div><br></div><div>Please do; I am not the OOP expert of the team ;)</div><div><br></div><div><br></div><div>I'm trying to figure out what is going on with $is_https in the</div><div>PMA_Config class. I'll have another look at the code later and do some</div><div>tests. I'll get back to you.</div><div><br></div><div><br></div><div>is_https isn't a member variable of the PMA_Config class, and I'm</div><div>wondering if it should be.</div><div><br></div><div>In detectHttps() $is_https is a local variable. (only defined in the</div><div>scope of this method) and this works.</div><div>In isHttps() $is_https is made static, forcing it to be remembered,</div><div>probably to avoid the behaviour as described in [0].</div><div><br></div><div>As I can see now, there are three things to be considered</div><div><br></div><div>1) use of static $is_https in isHttps() is not necessary -> it can be</div><div>replace by something else that makes more sense in an instanciated</div><div>class (like a class member variable, but because</div><div>$this->settings["is_https"] already exists, it makes sense to use this</div><div>one)</div><div><br></div><div>The issue with using $this->settings['is_https'] would be that</div><div>$this->settings['is_https'] is set by checkIsHttps() which in turn calls</div><div>isHttps() So if in any case, isHttps() is called independently, it might</div><div>cause issues. Since the purpose of static $is_https in isHttps() is</div><div>precisely to make sure that isHttps() only sets $is_https to true or false</div><div>only once in the life cycle of an object, we can have a class member like</div><div>'check_for_https' which is set to true in the constructor and then isHttps()</div><div>checks if its true or false and if its true, isHttps() executes further and</div><div>sets it to false.</div></div></blockquote><div><br></div><div>Adding the check_for_https will overcomplicate things.</div><div>If $this->settings['is_https'] would be used by isHttps() there would</div><div>be no use for checkIsHttps().</div></div></div></span></blockquote><div>That would be ideal if removing checkIsHttps isn't a problem. And I just checked, checkIsHttps is only used on line Line 94 of Config.class.php </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div><br></div><blockquote type="cite"><div><div>2) the reason why $is_https is cached/stored in isHttps()?</div><div><br></div><div>3) the reason why isHttps() doesn't parse PmaAbsoluteUri when it is</div><div>called a second time. (this is why the test returns a false when the</div><div>method is called a second time with a url that contains https, if it</div><div>would be a https url first and a http url second, it would return true</div><div>two times)</div><div><br></div><div><br></div><div>In case 2) and 3) I'm wondering why it is done this way.</div><div><br></div><div>When it runs a second time, since $is_https already has a boolean value</div><div>(true or false) and $is_https is a static variable in the scope of isHttps()</div><div>and isHttps() returns $is_https if it already has a boolean value, the tests</div><div>are returning the same results every time.</div></div></blockquote><div><br></div><div>I understand the code in isHttps() and understand why it is behaving</div><div>that way. ;)</div><div>What I was wondering about, is why it was done that way. (see reasoning below)</div><div><br></div></div></div></span></blockquote><div>Oh, sorry I misunderstood :) </div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;"><span><div><div><div></div><blockquote type="cite"><div><div><br></div><div>Michal, do you have any idea why this is done this way. It would make</div><div>sense that isHttps() would not cache $is_https and would parse</div><div>PmaAbsoluteUri every time, the execution is not expensive. That said,</div><div>PmAbsoluteUri is not supposed to change because it is set in the</div><div>configfile, so in a way caching does make sense.</div><div><br></div><div>[0] <a href="http://sourceforge.net/p/phpmyadmin/bugs/2965/">http://sourceforge.net/p/phpmyadmin/bugs/2965/</a></div><div><br></div><div><br></div><div>--</div><div>Kind regards,</div><div><br></div><div>Dieter Adriaenssens</div></div></blockquote><div><br></div><div>-- </div><div>Kind regards,</div><div><br></div><div>Dieter Adriaenssens</div><div><br></div><div>------------------------------------------------------------------------------</div><div>Own the Future-Intel® Level Up Game Demo Contest 2013</div><div>Rise to greatness in Intel's independent game demo contest.</div><div>Compete for recognition, cash, and the chance to get your game </div><div>on Steam. $5K grand prize plus 10 genre and skill prizes. </div><div>Submit your demo by 6/6/13. <a href="http://p.sf.net/sfu/intel_levelupd2d">http://p.sf.net/sfu/intel_levelupd2d</a></div><div>_______________________________________________</div><div>Phpmyadmin-devel mailing list</div><div><a href="mailto:Phpmyadmin-devel@lists.sourceforge.net">Phpmyadmin-devel@lists.sourceforge.net</a></div><div><a href="https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel">https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel</a></div></div></div></span>
</blockquote>
<div>
<br>
</div>