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