[mlmmj] Is using the include path for php-admin a good idea?

[mlmmj] Is using the include path for php-admin a good idea?

[Ben Schmidt]

Hi, everybody,

A patch was proposed to have conf/tunables.pl searched for in php's
include path.

http://mlmmj.org/bugs/bug.php?id=4

I'm wondering on people's opinions on this. Is it a good idea?

I actually think it probably isn't. On the one hand, even though
conf/tunables.pl is a somewhat generic name, it's a fairly unlikely one,
so name clashes are unlikely to be a problem. On the other hand, though,
using the include path effectively would require setting the include
path (which is a hassle; more so than just moving the tunables.pl file
around), or installing tunables.pl in a system-wide location where it is
likely to be missed for upgrades and so on.

If anything, perhaps we should just move tunables.pl into the same
directory as the PHP scripts that run the interface, dropping the
../conf? It will always be found then.

Thoughts?

Ben.

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Franky Van Liedekerke]

On Sun, 03 Oct 2010 23:54:55 +1100
Ben Schmidt <mail_ben_schmidt@yahoo.com.au> wrote:

> Hi, everybody,
> 
> A patch was proposed to have conf/tunables.pl searched for in php's
> include path.
> 
> http://mlmmj.org/bugs/bug.php?id=4
> 
> I'm wondering on people's opinions on this. Is it a good idea?
> 
> I actually think it probably isn't. On the one hand, even though
> conf/tunables.pl is a somewhat generic name, it's a fairly unlikely
> one, so name clashes are unlikely to be a problem. On the other hand,
> though, using the include path effectively would require setting the
> include path (which is a hassle; more so than just moving the
> tunables.pl file around), or installing tunables.pl in a system-wide
> location where it is likely to be missed for upgrades and so on.
> 
> If anything, perhaps we should just move tunables.pl into the same
> directory as the PHP scripts that run the interface, dropping the
> ../conf? It will always be found then.
> 
> Thoughts?
> 
> Ben.

As a longtime php-coder, here's my opinion:
- Putting it in the include path is a bad idea
- hardcoding the path also
So best of all: figure out the current filepath, put it in a variable
and start from there (don't put this in a config file, it can be
determined automatically), something like "dirname(__FILE__);" or to
have the parent dir "basename(dirname(__FILE__));"

Franky

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Franky Van Liedekerke]

On Sun, 3 Oct 2010 15:49:35 +0200
Franky Van Liedekerke <liedekef@telenet.be> wrote:

> As a longtime php-coder, here's my opinion:
> - Putting it in the include path is a bad idea
> - hardcoding the path also
> So best of all: figure out the current filepath, put it in a variable
> and start from there (don't put this in a config file, it can be
> determined automatically), something like "dirname(__FILE__);" or to
> have the parent dir "basename(dirname(__FILE__));"
> 

well, please ignore my last statement :-)
Basename strips the parent dir, not returns it. I wish for a mail-recall
feature in mlmmj ;-)

Franky

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Thomas Goirand]

Franky Van Liedekerke wrote:
> On Sun, 3 Oct 2010 15:49:35 +0200
> Franky Van Liedekerke <liedekef@telenet.be> wrote:
> 
>> As a longtime php-coder, here's my opinion:
>> - Putting it in the include path is a bad idea
>> - hardcoding the path also
>> So best of all: figure out the current filepath, put it in a variable
>> and start from there (don't put this in a config file, it can be
>> determined automatically), something like "dirname(__FILE__);" or to
>> have the parent dir "basename(dirname(__FILE__));"
>>
> 
> well, please ignore my last statement :-)
> Basename strips the parent dir, not returns it. I wish for a mail-recall
> feature in mlmmj ;-)
> 
> Franky

It doesn't mater, you got the main principle right anyway. I agree that
here, there's absolutely no point in adding the include path. That one
is to use ... include, not at all for opening files like the php-admin
would need. It makes no sense in this context.

Thomas

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Ben Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

>>> As a longtime php-coder, here's my opinion:
>>> - Putting it in the include path is a bad idea
>>> - hardcoding the path also
>>> So best of all: figure out the current filepath, put it in a variable
>>> and start from there (don't put this in a config file, it can be
>>> determined automatically), something like "dirname(__FILE__);" or to
>>> have the parent dir "basename(dirname(__FILE__));"
>
> I agree that here, there's absolutely no point in adding the include
> path. That one is to use ... include, not at all for opening files
> like the php-admin would need. It makes no sense in this context.

Have a look at my attached proposed patch. The interface was already
updated since the bug report was filed, but looking for tunables.pl
relative to $templatedir seems senseless to me, so I've added another
variable, $confdir. I've removed hard-coded paths and used paths
relative to __FILE__ instead. Finding config.php in the first place
could still be difficult, but at least the require directive for it is
at the top of each file, so it's easy to find what to change.

What do you think?

Ben.




[-- Attachment #2: php-admin-paths.diff --]
[-- Type: text/plain, Size: 2584 bytes --]

diff -r ca8ddad585ef contrib/web/php-admin/conf/config.php
--- a/contrib/web/php-admin/conf/config.php	Mon Oct 04 10:18:15 2010 +1100
+++ b/contrib/web/php-admin/conf/config.php	Tue Oct 05 22:06:54 2010 +1100
@@ -1,6 +1,7 @@
 <?php
 
 $topdir = "/var/spool/mlmmj";
-$templatedir = "/home/mlmmj/templates";
+$confdir = dirname(__FILE__);
+$templatedir = dirname(dirname(__FILE__))."/templates";
 
 ?>
diff -r ca8ddad585ef contrib/web/php-admin/htdocs/edit.php
--- a/contrib/web/php-admin/htdocs/edit.php	Mon Oct 04 10:18:15 2010 +1100
+++ b/contrib/web/php-admin/htdocs/edit.php	Tue Oct 05 22:06:54 2010 +1100
@@ -26,8 +26,8 @@
  * IN THE SOFTWARE.
  */
 
-require("../conf/config.php");
-require("class.rFastTemplate.php");
+require(dirname(dirname(__FILE__))."/conf/config.php");
+require(dirname(__FILE__)."/class.rFastTemplate.php");
 
 function mlmmj_boolean($name, $nicename, $text) 
 {
@@ -117,10 +117,7 @@
 
 $tpl->assign(array("LIST" =>htmlentities($list)));
 
-$handle = fopen("$templatedir/../conf/tunables.pl", "r");
-$tunables = fread($handle, filesize("$templatedir/../conf/tunables.pl"));
-fclose($handle);
-
+$tunables = file_get_contents($confdir.'/tunables.pl');
 eval($tunables);
 
 $tpl->parse("MAIN","main");
diff -r ca8ddad585ef contrib/web/php-admin/htdocs/index.php
--- a/contrib/web/php-admin/htdocs/index.php	Mon Oct 04 10:18:15 2010 +1100
+++ b/contrib/web/php-admin/htdocs/index.php	Tue Oct 05 22:06:54 2010 +1100
@@ -26,8 +26,8 @@
  * IN THE SOFTWARE.
  */
 
-require("class.rFastTemplate.php");
-require("../conf/config.php");
+require(dirname(dirname(__FILE__))."/conf/config.php");
+require(dirname(__FILE__)."/class.rFastTemplate.php");
 
 $tpl = new rFastTemplate($templatedir);
 
diff -r ca8ddad585ef contrib/web/php-admin/htdocs/save.php
--- a/contrib/web/php-admin/htdocs/save.php	Mon Oct 04 10:18:15 2010 +1100
+++ b/contrib/web/php-admin/htdocs/save.php	Tue Oct 05 22:06:54 2010 +1100
@@ -26,8 +26,8 @@
  * IN THE SOFTWARE.
  */
 
-require("../conf/config.php");
-require("class.rFastTemplate.php");
+require(dirname(dirname(__FILE__))."/conf/config.php");
+require(dirname(__FILE__)."/class.rFastTemplate.php");
 
 function mlmmj_boolean($name, $nicename, $text)
 {
@@ -93,10 +93,7 @@
 $tpl->define(array("main" => "save.html"));
 $tpl->assign(array("LIST" => htmlentities($list)));
 
-$handle = fopen("$templatedir/../conf/tunables.pl", "r");
-$tunables = fread($handle, filesize("$templatedir/../conf/tunables.pl"));
-fclose($handle);
-
+$tunables = file_get_contents($confdir.'/tunables.pl');
 eval($tunables);
 
 $tpl->parse("MAIN","main");

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Thomas Goirand]

Ben Schmidt wrote:
>>>> As a longtime php-coder, here's my opinion:
>>>> - Putting it in the include path is a bad idea
>>>> - hardcoding the path also
>>>> So best of all: figure out the current filepath, put it in a variable
>>>> and start from there (don't put this in a config file, it can be
>>>> determined automatically), something like "dirname(__FILE__);" or to
>>>> have the parent dir "basename(dirname(__FILE__));"
>>
>> I agree that here, there's absolutely no point in adding the include
>> path. That one is to use ... include, not at all for opening files
>> like the php-admin would need. It makes no sense in this context.
> 
> Have a look at my attached proposed patch. The interface was already
> updated since the bug report was filed, but looking for tunables.pl
> relative to $templatedir seems senseless to me, so I've added another
> variable, $confdir. I've removed hard-coded paths and used paths
> relative to __FILE__ instead. Finding config.php in the first place
> could still be difficult, but at least the require directive for it is
> at the top of each file, so it's easy to find what to change.
> 
> What do you think?
> 
> Ben.

That's a much much much better solution, definitively!

Somebody has sent a bug report on the MLMMJ package in Debian, about
this issue, I guess he was reading the list. I guess I'll have to apply
this patch to the SID/Squeeze package before the release.

Thomas

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Ben Schmidt]

> Somebody has sent a bug report on the MLMMJ package in Debian, about
> this issue, I guess he was reading the list. I guess I'll have to apply
> this patch to the SID/Squeeze package before the release.

I would have thought it was close to irrelevant on Debian, since you
distribute binary packages that 'just work', so people would only come
across the problem of the files not being found if they messed with it.
(That's the history behind the bug report, which I didn't bother noting
when I transferred it to the new system.)

BTW, git is probably about 3-5 weeks away, but on the agenda.

Ben.

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Thomas Goirand]

Ben Schmidt wrote:
>>>> As a longtime php-coder, here's my opinion:
>>>> - Putting it in the include path is a bad idea
>>>> - hardcoding the path also
>>>> So best of all: figure out the current filepath, put it in a variable
>>>> and start from there (don't put this in a config file, it can be
>>>> determined automatically), something like "dirname(__FILE__);" or to
>>>> have the parent dir "basename(dirname(__FILE__));"
>>
>> I agree that here, there's absolutely no point in adding the include
>> path. That one is to use ... include, not at all for opening files
>> like the php-admin would need. It makes no sense in this context.
> 
> Have a look at my attached proposed patch. The interface was already
> updated since the bug report was filed, but looking for tunables.pl
> relative to $templatedir seems senseless to me, so I've added another
> variable, $confdir. I've removed hard-coded paths and used paths
> relative to __FILE__ instead. Finding config.php in the first place
> could still be difficult, but at least the require directive for it is
> at the top of each file, so it's easy to find what to change.
> 
> What do you think?
> 
> Ben.

Hi Ben,

Has this patch been applied? I think it would fix this:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bugY9183

As this would almost certainly fix this bug which is "release critical"
in Debian, I would like to know if you have tested it and applied it to
your HG repo. I didn't find it in the log on the web. I'd happily apply
it if I have confirmations that it fixes the issue (if nobody tests it,
I might be able to, but not right now, as I'm busy with other stuff).

Cheers,

Thomas

Re: [mlmmj] Is using the include path for php-admin a good idea?

[Ben Schmidt]

>> Have a look at my attached proposed patch. The interface was already
>> updated since the bug report was filed, but looking for tunables.pl
>> relative to $templatedir seems senseless to me, so I've added another
>> variable, $confdir. I've removed hard-coded paths and used paths
>> relative to __FILE__ instead. Finding config.php in the first place
>> could still be difficult, but at least the require directive for it is
>> at the top of each file, so it's easy to find what to change.
>
> Has this patch been applied?

I've just given it a quick test, merged and pushed it, along with
another little bug fix related to the earlier security fix, that is
particularly for the Mac.

> I think it would fix this:
>
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bugY9183

I think it will probably fix it, too, but haven't explicitly tested
that. Somebody else will need to do that.

Ben.