YunoHost-Apps / garradin_ynh

Logiciel libre de gestion associative pour YunoHost
https://garradin.eu/
GNU General Public License v3.0
7 stars 8 forks source link

url path issue when saving new accounting entry #37

Closed slowphil closed 3 years ago

slowphil commented 3 years ago

I installed garradin_ynh in the subpath myserver.tld//garradin of our root domain, following the English README. Turns out that we are now having an issue when trying to save a new accounting entry: we get a "this page does not exist" error trying to access https://myserver.tld/garradin/garradin/admin/acc/transaction/new.php?ok=134 where the subpath garradin is doubled. If I remove manually the doubled /garradin in the path, there is no longer an error and the entry is saved.

I notice that the French version of the README_fr, warns one should use a subdomain of the root domain (i.e. unlike the English version, which mentions one should use a subpath). N'installez pas cette apllication à la racine du domaine par défaut sans ajouter un sous-domaine. Sinon la page de connexion SSO sera remplacée par la page d'accueil de Garradin. but the wording is ambiguous and the error we observe is not the issue that is described in the README.

Is the cause of the trouble not using a dedicated subdomain (e.g. garradin.myserver.tld) ? If so the English documentation should be fixed, and I guess the manifest should be changed so that it is made clear it just won't work in a subpath.

Otherwise, if the issue is not about using a subpath, is there something I can do to fix the issue?

rodinux commented 3 years ago

Hello, I think My explanation for the README is bad. You don't have to add a slash, it must be only installed for example in myserver.tld/garradin. And you can install it without a dedicated subdomain also. I just wanted prevent installing it on myserver.tld/ (on the root of the principal domain). I can't reproduce your issue, but if you have installed the app on myserver.tld//garradin, this is wrong because you use 2 slash...

slowphil commented 3 years ago

Actually, I simply used the default location myserver.tld/garradin, so there is no double slash, indeed. My bad typing, sorry for the confusion. In any case, from what you write, our garradin setup (the default one) should not be the cause of our problem.

Maybe the issue we have is that I first installed yunohost and garradin on a testing subdomain, and then changed garradin's URL to the main domain from yunohost's admin interface. So perhaps our issue would be due to a small bug in the change_url script... I'll deinstall and reinstall and let you know.

slowphil commented 3 years ago

So, I deleted and recreated Garradin using all default settings for the app (except public visibility, which we deselected) and the issue is still present for us with a newly created database: Whenever we try to record an expense (an income too and probably whatever), we get an error message for an url as above (only the last number changes, reflecting the entry number in the accounting book, it seems): https://myserver.tld/garradin/garradin/admin/acc/transactions/new.php?ok=3 However, in spite of the error message, the item appears correctly recorded in the DB without any further action.

Our yunohost is up-to-date, running on a raspberry, and everything else is default from install : Garradin 1.0.3 [release] Informations système Version PHP : 7.3.19-1~deb10u1 Version SQLite : 3.27.2

I can also reproduce the issue on a fresh install on a x86_64 virtual machine... It has nothing to do with the history of our raspberry pi. I find it strange that you cannot reproduce it.

rodinux commented 3 years ago

Hi, ok I see and can reproduce your issue... And like your situation, if I manually remove the double subpath, it works...

So I think (surely) the problem is caused by the nginx.conf file. It was hard to found how access at the pages created on the public site and we have changed few times this file with contributors because updating the version gives troubles and mismatch urls... The last change with these lines must be the problem (but also the solution to repair broken urls on the public site):

index index.php /_route.php;
  try_files $uri $uri/ __PATH__/__PATH__/_route.php?$query_string;

  location ~ \.php$ {
        if (!-e $request_filename) {
            rewrite ^__PATH__/?(.*)$ __PATH__/_route.php?/$1 last;
            break;
        }
       fastcgi_pass unix:/var/run/php/php__PHPVERSION__-fpm-__NAME__.sock;
        fastcgi_index index.php;
        include fastcgi_params;
        fastcgi_param REMOTE_USER $remote_user;
        fastcgi_param PATH_INFO $fastcgi_path_info;
        fastcgi_param SCRIPT_FILENAME $request_filename;
  }

before it was like this:

 index index.php index.html;
  try_files $uri $uri/ /_route.php;

  location ~ [^/]\.php(/|$) {
    try_files $uri $uri/ /_route.php;
    fastcgi_split_path_info ^(.+?\.php)(/.*)$;
    fastcgi_pass unix:/var/run/php/php7.0-fpm-__NAME__.sock;
    fastcgi_pass unix:/var/run/php/php__PHPVERSION__-fpm-__NAME__.sock;
    fastcgi_index index.php;
    include fastcgi_params;
    fastcgi_param REMOTE_USER $remote_user;
}
slowphil commented 3 years ago

Thanks for the answer. Our conf file was containing

index index.php /_route.php;
  try_files $uri $uri/ /garradin//garradin/_route.php?$query_string;

  location ~ \.php$ {
        if (!-e $request_filename) {
            rewrite ^/garradin/?(.*)$ /garradin/_route.php?/$1 last;
            break;
        }
        fastcgi_pass unix:/var/run/php/php7.3-fpm-garradin.sock;
        fastcgi_index index.php;
        include fastcgi_params;
        fastcgi_param REMOTE_USER $remote_user;
        fastcgi_param PATH_INFO $fastcgi_path_info;
        fastcgi_param SCRIPT_FILENAME $request_filename;
  }

I followed your indications (adjusting the fastcgi_pass directive to make it as it was above) and changed that section of the conf file to:

  index index.php index.html;
  try_files $uri $uri/ /_route.php;

  location ~ [^/]\.php(/|$) {
    try_files $uri $uri/ /_route.php;
    fastcgi_split_path_info ^(.+?\.php)(/.*)$;
    fastcgi_pass unix:/var/run/php/php7.3-fpm-garradin.sock;
    fastcgi_index index.php;
    include fastcgi_params;
    fastcgi_param REMOTE_USER $remote_user;
}

and I restarted nginx and php-fpm.

However, now garradin does not load at all (the browser shows a blank page). There is no longer any apparent error, though :).

Opening the web console in firefox does not give any obvious clue. I'm not knowledgeable enough about web servers to understand or even to debug what is wrong...

rodinux commented 3 years ago

Oh, I think we need some time to found how correct this issue, and I can not found it alone (the correction was given by a contributor). But perhaps the correct change (for your case only) could be like this:

 index index.php index.html;
  try_files $uri $uri/ /_route.php;

  location ~ [^/]\.php(/|$) {
    try_files $uri $uri/ /_route.php;
    fastcgi_split_path_info ^(.+?\.php)(/.*)$;
    fastcgi_pass unix:/var/run/php/php7.0-fpm-__NAME__.sock;
    fastcgi_pass unix:/var/run/php/php__PHPVERSION__-fpm-__NAME__.sock;
    fastcgi_index index.php;
    include fastcgi_params;
    fastcgi_param REMOTE_USER $remote_user;

  # Increase size limit
  client_max_body_size 2M;

  # PHP configuration end

  # Include SSOWAT user panel.
  include conf.d/yunohost_panel.conf.inc;
}

But, anyway, this do iisues with links on the public site (not for you if it is a private site) I try this, but there is still an error, when you do an entry it return to the SSO page... so is not a solution at once.

rodinux commented 3 years ago

We need found how to resolve this issue, please be patient, we need try found the configuration to add in the nginx.conf...

BenoitCier commented 3 years ago

I test to reproduce the bug, i think it could be a bug in the source code like when we update to 1.0.2

I test, it's only for acc/transaction/new.php form. I don't think it's a problem with nginx conf.

rodinux commented 3 years ago

It means perhaps we need to introduce again the file Utils.php ? I try replacing Utils.php but with no success...

rodinux commented 3 years ago

It is really every time we need saving a new accounting or delete one where comes the error to get the confirmation or view page, but the action is considred...

BenoitCier commented 3 years ago

not the util.php fil but other (acc/new.php) delete also?

the form call the same page, and the this page call itself after saving the form. It's on this second call we have a bug, i work on this.

BenoitCier commented 3 years ago

I found a bug in garradin source file, i send a mail I don't reproduce the problem when i delete an accounting...

BenoitCier commented 3 years ago

A new version of Utils.php files fix this issue. https://fossil.kd2.org/garradin/raw/9771f61ae98cb42c91c9cbda7ce7fac3ccdf5903a868fd7fe97501ef4a1302c6?at=9771f61ae98cb42c Thanks to @bohwaz. We need to wait next release 1.0.5 (only work whitout subpath for the moment!)

rodinux commented 3 years ago

@slowphil, you can upgrade now your garradin app with the testing branch, it will resolve your issue.

sudo yunohost app upgrade garradin -u https://github.com/YunoHost-Apps/garradin_ynh/tree/testing --debug

rodinux commented 3 years ago

A new version of Utils.php files fix this issue. https://fossil.kd2.org/garradin/raw/9771f61ae98cb42c91c9cbda7ce7fac3ccdf5903a868fd7fe97501ef4a1302c6?at=9771f61ae98cb42c Thanks to @bohwaz. We need to wait next release 1.0.5 (only work whitout subpath for the moment!)

Thanks. Perhaps it is better wait this next release to push on the master branch ? But I put already a PR with these changes because this issue is already in the version 1.0.3...

BenoitCier commented 3 years ago

Perhaps it is better wait this next release to push on the master branch ?

Yes, it's better

rodinux commented 3 years ago

Perhaps it is better wait this next release to push on the master branch ?

Yes, it's better

but there is a this issue also in the master branch with the 1.0.3 version, so I doubt...

slowphil commented 3 years ago

I have reverted to 1.0.2.~ynh4 and that issue is gone. I'll wait for the next version, not a problem. Merci Benoît et Rodolphe pour votre réactivité!

rodinux commented 3 years ago

I have reverted to 1.0.2.~ynh4 and that issue is gone. I'll wait for the next version, not a problem. Merci Benoît et Rodolphe pour votre réactivité!

Ok, in the version 1.0.2~ynh4, you already have a temporaly file Utils.php edited from the conf directory. But do as you want... I hope there will be ok with the next update...

bohwaz commented 3 years ago

A new 1.0.5 release is available, fixing that issue, thanks for the feedback.

rodinux commented 3 years ago

A new 1.0.5 release is available, fixing that issue, thanks for the feedback.

Thanks for your work, nice.

rodinux commented 3 years ago

I have reverted to 1.0.2.~ynh4 and that issue is gone. I'll wait for the next version, not a problem. Merci Benoît et Rodolphe pour votre réactivité!

@slowphil can you now upgrade the app ? The version 1.0.5 is up to date and fix the issue. If is right, you can also close this issue

slowphil commented 3 years ago

I upgraded. So far so good. The issue is resolved, I close it. Thanks to all!