require_once(geshi.php) in svnlook.php

[danielmarschall]

The following problem has been found in my PHP error log

include/svnlook.php

PHP Warning: require_once(geshi.php): failed to open stream: No such file or directory in .../websvn/include/svnlook.php on line 758

(I will try to find an instance in my access.log)

[michael-o]

I consider the issue as invalid because it is only happens when you enable Geshi highlighting, don't use composer, but are expected to manually drop geshi or use pear to install it.

Can you double check that Geshi highlighting is enabled, but Geshi not installed?

[danielmarschall]

This PHP warning was last thrown 2 months ago, and currently I have php-geshi installed. I think it wasn't installed back then.

My include/config.php contains:

# aptitude install php-geshi
set_include_path(get_include_path() . PATH_SEPARATOR . '/usr/share/php-geshi/'); // added by DM 18.12.2022 (what is the correct way?!)
$config->useGeshi();

I have one question and one improvement suggestion:

Question: As I commented in my config file, is set_include_path() the correct way? I remember geshi.php won't be found if I don't do that, but this is also not documented.

Improvement suggestion: Instead of throwing the PHP error require_once(geshi.php), you should catch this case and either show a human-readable warning and/or disable the syntax-highlighting silently.

[michael-o]

This PHP warning was last thrown 2 months ago, and currently I have php-geshi installed. I think it wasn't installed back then.

My include/config.php contains:

# aptitude install php-geshi
set_include_path(get_include_path() . PATH_SEPARATOR . '/usr/share/php-geshi/'); // added by DM 18.12.2022 (what is the correct way?!)
$config->useGeshi();

I have one question and one improvement suggestion:

Question: As I commented in my config file, is set_include_path() the correct way? I remember geshi.php won't be found if I don't do that, but this is also not documented.

I believe that the call is not required. We expect that geshi through pear is on the default include path. Check it with phpinfo(). At least here when I install the pear package it is instantly available. One should not modify the source for that. (for me: .:/usr/local/share/pear).

Improvement suggestion: Instead of throwing the PHP error require_once(geshi.php), you should catch this case and either show a human-readable warning and/or disable the syntax-highlighting silently.

I am against silently disabled it, then the admin will not know about this issue. As for the warning, I am a bit split because I expect that it is installed by php means. Otherwise we'd need to do these catches for all optional components which we don't.

[danielmarschall]

For my environment (installed via Debian package) geshi.php is in /usr/share/php-geshi/. It is not in the default PHP include path. So, Geshi won't run out-of-the-box with WebSVN if Geshi is installed via Debian package.

[michael-o]

For my environment (installed via Debian package) geshi.php is in /usr/share/php-geshi/. It is not in the default PHP include path. So, Geshi won't run out-of-the-box with WebSVN if Geshi is installed via Debian package.

I see, this makes it quite unportable und need to explicitly reference that. Have you considered using composer? We have removed geshi from WebSVN because there is pear and composer.

[danielmarschall]

Ideas:

• In the config you could let the user choose a custom path where to find the geshi.php
• Or, you can do file_exists('/usr/share/php-geshi/geshi.php') require_once '/usr/share/php-geshi/geshi.php'); else require_once "geshi.php" because I think some other distros might use that path for Non-Pear installations

In regards a warning: The problem is that an user installing WebSVN might not notice that Geshi is not installed, because they might not look in a source file.

[danielmarschall]

We have removed geshi from WebSVN because there is pear and composer.

I have not considered composer. I think it is good to use the Debian package, because it is automatically updated with aptitude

[michael-o]

I double checked: #153 (comment). Geshi form pear is too old, it does not work with PHP 8. Either manual or composer.

[michael-o]

Ideas:

* In the config you could let the user choose a custom path where to find the geshi.php

* Or, you can do `file_exists('/usr/share/php-geshi/geshi.php') require_once '/usr/share/php-geshi/geshi.php'); else require_once "geshi.php"` because I think some other distros might use that path for Non-Pear installations

In regards a warning: The problem is that an user installing WebSVN might not notice that Geshi is not installed, because they might not look in a source file.

We have dropped some code years ago:
b4294eb and a4bda61.

Have you considered to add this to config.php? This is the only file you are supposed to touch. I don't want to add any OS-specific paths to search for any external components. It will create a never ending mess.

[michael-o]

Have you also considered to modify your php.ini? include_path and done.

[danielmarschall]

I have now changed my php.ini file to:

include_path = .:/usr/share/php:/usr/share/php-geshi/

The downside of the include_path setting is that you cannot extend it. So you need to repeat the value it had previously (in my case the previous setting was .:/usr/share/php

I still think it would be nice if there would be a config setting that lets the user choose a path to a custom geshi installation.

How about an optional argument to useGeshi?

// uses "default" geshi according to composer/PEAR/include_path.
$config->useGeshi();

// alternatively, explicit path.
$config->useGeshi('/usr/share/php-geshi/geshi.php');

[michael-o]

I have now changed my php.ini file to:

include_path = .:/usr/share/php:/usr/share/php-geshi/

The downside of the include_path setting is that you cannot extend it. So you need to repeat the value it had previously (in my case the previous setting was .:/usr/share/php

I agree, but that is a PHP problem we cannot solve and the change happens only once.

I still think it would be nice if there would be a config setting that lets the user choose a path to a custom geshi installation.

How about an optional argument to useGeshi?

// uses "default" geshi according to composer/PEAR/include_path.
$config->useGeshi();

// alternatively, explicit path.
$config->useGeshi('/usr/share/php-geshi/geshi.php');

Did you try a hack? How does Geshi find its other files using require/include?

[danielmarschall]

What do you mean with hack?
$config->useGeshi('/usr/share/php-geshi/geshi.php'); was a improvement suggestion. I don't have that in my config file

[michael-o]

What do you mean with hack? $config->useGeshi('/usr/share/php-geshi/geshi.php'); was a improvement suggestion. I don't have that in my config file

Hack the code locally to see you fully qualified path loading works without changing php.ini.

[danielmarschall]

Ok, I removed the include_path (and verified that it fails then), and then altered include/svnlook.inc.php to change the two require(geshi.php) to require(/usr/share/php-geshi/geshi.php). It then works.

Geshi achieves this by using define('GESHI_ROOT', dirname(__FILE__) . DIRECTORY_SEPARATOR)

[michael-o]

Ok, I removed the include_path (and verified that it fails then), and then altered include/svnlook.inc.php to change the two require(geshi.php) to require(/usr/share/php-geshi/geshi.php). It then works.

Geshi achieves this by using define('GESHI_ROOT', dirname(__FILE__) . DIRECTORY_SEPARATOR)

Very good! I think we can reuse the semantics in WebSvnConfig by introducing $geshi = 'geshi.php, setGeshiPath($path) and getGeshiScript(). WDYT?

[danielmarschall]

setGeshiPath($path) sounds very good!

Would it also be possible to do this for Parsedown.php? I just noticed this feature, and I am using the Debian package php-parsedown . With the Debian package, the location of Parsedown.php is /usr/share/php/Parsedown/Parsedown.php . Like Geshi, it works if I change require_once Parsedown.php to require_once '/usr/share/php/Parsedown/Parsedown.php'

Thank you very much

[michael-o]

setGeshiPath($path) sounds very good!

Would it also be possible to do this for Parsedown.php? I just noticed this feature, and I am using the Debian package php-parsedown . With the Debian package, the location of Parsedown.php is /usr/share/php/Parsedown/Parsedown.php . Like Geshi, it works if I change require_once Parsedown.php to require_once '/usr/share/php/Parsedown/Parsedown.php'

Thank you very much

Technically, yes, but I consider this feature partially broken, please see: #174

Do you want to work on a PR for Geshi first?

[Gwindalmir]

I hit this same problem setting up websvn in php:8.2-apache docker container.
I was confused, because the documentation still says it's included.

https://websvnphp.github.io/docs/install.html

GeSHi (Generic Syntax Highlighter) is bundled with WebSVN. To use it, just make sure config.php has this line uncommented

I'll figure it out, now that I know it was actually removed.

[danielmarschall]

I never have read this documentation.

I think large parts of it needs to be extended or replaced.
I still dislike this approach:

apt-get install php-pear
pear channel-discover pear.geshi.org
pear install Archive_Tar
pear install geshi/geshi
pear install Text_Diff

because I rather just install apt-get install php-geshi and then simply change the path to it.

[Gwindalmir]

I never have read this documentation.

I think large parts of it needs to be extended or replaced. I still dislike this approach:

apt-get install php-pear
pear channel-discover pear.geshi.org
pear install Archive_Tar
pear install geshi/geshi
pear install Text_Diff

because I rather just install apt-get install php-geshi and then simply change the path to it.

I had to switch to enscript because I couldn't find any packages available for geshi. php-pear is blocked in the official php docker image so I couldn't use the above steps either.

[michael-o]

I never have read this documentation.

I think large parts of it needs to be extended or replaced. I still dislike this approach:

apt-get install php-pear
pear channel-discover pear.geshi.org
pear install Archive_Tar
pear install geshi/geshi
pear install Text_Diff

because I rather just install apt-get install php-geshi and then simply change the path to it.

That is actually a conceptual flaw of your distro, IMHO. Here on FreeBSD, I install the channel and the pear-packages by pkg together. It is logically the same as pip, if you install pip/pear through OS means then the packages should do as well. You still can resort to composer.

[michael-o]

I never have read this documentation.
I think large parts of it needs to be extended or replaced. I still dislike this approach:

apt-get install php-pear
pear channel-discover pear.geshi.org
pear install Archive_Tar
pear install geshi/geshi
pear install Text_Diff

because I rather just install apt-get install php-geshi and then simply change the path to it.

I had to switch to enscript because I couldn't find any packages available for geshi. php-pear is blocked in the official php docker image so I couldn't use the above steps either.

Have you tried composer? It works for me.