Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[slim3] Run xhgui in subdir #450

Closed
wants to merge 5 commits into from
Closed

[slim3] Run xhgui in subdir #450

wants to merge 5 commits into from

Conversation

luzip665
Copy link
Contributor

@luzip665 luzip665 commented Jan 17, 2022 •
edited
Loading

A little change to be able to run xhgui with slim3 in a subdir.

I need to run xhgui on https://localhost/xhgui/.
The gui loads in general. Links work, but css and js are not found because the links are going to / instead of /xhgui

@luzip665 luzip665 mentioned this pull request Jan 17, 2022
Closed
@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Please commit with sane commit messages, so I could merge the change to intermediate branch

glensc
glensc reviewed Jan 17, 2022
View reviewed changes
src/Twig/TwigExtension.php
// this is copy of \Slim\Slim::urlFor()
// to mix path prefix in \Slim\Slim::urlFor

return rtrim($this->pathPrefix(), '/') . $this->router->urlFor($name) . $query;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just use $this->pathPrefix here directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in PHP8 we have struggle to concatenate null and string

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid null: $this->pathPrefix() ?: ''

glensc
glensc reviewed Jan 17, 2022
View reviewed changes
src/Twig/TwigExtension.php Outdated
@@ -132,7 +135,7 @@ public function formatPercent($value): string

private function pathPrefix(): string
{
if ($this->pathPrefix !== null) {
if ($this->pathPrefix !== '') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no no no. this is breaking change.

null means use autodetection, having string value means use value from config.

glensc
glensc reviewed Jan 17, 2022
View reviewed changes
src/Twig/TwigExtension.php Outdated
@@ -78,7 +81,7 @@ public function url(string $name, $queryargs = []): string
// this is copy of \Slim\Slim::urlFor()
// to mix path prefix in \Slim\Slim::urlFor

return rtrim($this->pathPrefix(), '/') . $this->router->urlFor($name) . $query;
return rtrim($this->pathPrefix, '/') . $this->router->urlFor($name) . $query;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using function was correct. revert this line

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

I don't see what problem are you solving here. describe the problem first in PR body!

@luzip665
Copy link
Contributor Author

luzip665 commented Jan 17, 2022 •
edited
Loading

Described in body.

I changed in line 24ff the null handling to set $path_prefix to empty string in case of null.
So in url the pathPrefix can directly be used, because $this->router->urlFor allready resolves the subdir.
May be the function pathPrefix can be removed if it is only used in staticUrl.

The function pathPrefix does autodetection in case of $this->pathPrefix is an empty string.

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

If the detection doesn't work, you can just set path.prefix in config.

also, I do not understand anything written in this comment:

@luzip665
Copy link
Contributor Author

luzip665 commented Jan 17, 2022 •
edited
Loading

If I set path.prefix to xhgui in the config, everything is prefixed with this:
Starting from http://localhost/xhgui/, Links (in the menubar) are going to http://localhost/xhgui/xhgui/. css to http://localhost/css/....
This happens also in case of path.prefix is null.

Setting path.prefix to empty string results in:
Starting from http://localhost/xhgui/, Links (in the menubar) are going to http://localhost/xhgui/xhgui/. css to http://localhost/xhgui/css/....

In the comment above I tried to explain what I did and why in my opinion the autodetection still works.

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Compare the html, show the rendered html tag for css. guess the problem is with leading / making requests invalid. I guess I have to test this locally then to see myself.

what adds the second /xghui after the pathPrefix?

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Tested locally. having path.prefix=/xhgui, the html is:

<link href="/xhgui/css/xhgui.css" rel="stylesheet" media="screen">

Using docker image provided by this project, i.e this nginx config:

@luzip665
Copy link
Contributor Author

I think:

If set path.prefix to null:

  • Twig calls url for non-static links (like home)
    ** url uses $this->pathPrefix() to resolve the path
    ** pathPrefix() returns $this->request->getUri()->getBasePath(), which is in my case /xhgui
    ** url uses urlFor for resolving the url for $name), which results also in /xhgui** So finallyurlreturn/xhgui/xhgui/`
  • For static urls it works because only getbasePath is used

@luzip665
Copy link
Contributor Author

Tested locally. having path.prefix=/xhgui, the html is:

<link href="/xhgui/css/xhgui.css" rel="stylesheet" media="screen">

Using docker image provided by this project, i.e this nginx config:

What about the link to Longest wall time?

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

$this->request->getUri()->getBasePath(), which is in my case /xhgui

this seems correct value, since you have installation at /xhgui.

how can I reproduce your setup?

also, to me it seems you want to use path.prefix='' or path.prefix='/' in your config. (remember path.prefix=null means autodetection!)

@luzip665
Copy link
Contributor Author

luzip665 commented Jan 17, 2022 •
edited
Loading

My setup has nothing special. But it uses #333.

In case of path.prefix=null, I get /xhgui/xhgui for non-static urls and /xhgui for static.
In case of path.prefix='', I get /xhgui/ for non-static urls and `` for static.
In case of path.prefix='/xhgui', I get `/xhgui/xhgui` for non-static urls and `/xhgui` for static.

Imo the main problem is in the url function that does the autodetection twice.

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Special or not. explain how to reproduce this.

From description seems getBasePath() returns xhgui for you and "" for me and it's not symmetric for static URLs. therefore the inconsistency for you.

your current pull request doesn't fix any problem as you just set the path prefix to an empty string, so the autodetection code is never run.

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Added documentation examples how the path prefix must behave:

@luzip665
Copy link
Contributor Author

I use an apache with

DocumentRoot /var/www/
Alias /xhgui/ /var/www/xhgui/webroot/

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

I use an apache with

DocumentRoot /var/www/
Alias /xhgui/ /var/www/xhgui/webroot/

Can you make the reproducer with apache docker image?

@luzip665
Copy link
Contributor Author

Further digging bring up that in /vendor/slim/slim/Slim/App.php in Line 381 the router basePath is set to $request->getUri()->getBasePath()

I try to make it reproducibly

@luzip665
Copy link
Contributor Author

luzip665 commented Jan 17, 2022 •
edited
Loading

Reproducing with docker image at tecart/crm_v51.
This is one image we work with.

xhgui is bind-mounted to /var/www/xhgui

In the container you need to disable tideways using phpdismod tideways

Then you can access xhgui via http://172.17.0.2/xhgui/

The problem relates only to branch slim-upgrade. 0.20.x works

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

slim-upgrade branch is not merged, so 0.19.x and 0.20.x branches are the same

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Here's why the basepath is set on the router. I don't see it helps

Stack trace:
#0 /var/www/xhgui/vendor/slim/slim/Slim/App.php(381): Slim\\Router->setBasePath('/xhgui')
#1 /var/www/xhgui/vendor/slim/slim/Slim/App.php(297): Slim\\App->process(Object(Slim\\Http\\Request), Object(Slim\\Http\\Response))
#2 /var/www/xhgui/src/Application.php(26): Slim\\App->run()
#3 /var/www/xhgui/webroot/index.php(10): XHGui\\Application->run()
#4 {main}
  thrown in /var/www/xhgui/vendor/slim/slim/Slim/Router.php on line 105

@glensc glensc mentioned this pull request Jan 17, 2022
Merged
@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Ok, so the problem is that same basepath added twice, made change that strips from urlFor result:

That replacement PR works in docker image you provided.

@glensc glensc closed this Jan 17, 2022
@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Also, please fill in meaningful commit messages if you want your changes to be merged. repeating the same commit message 5 times, isn't it.

Here's some guides:

@luzip665
Copy link
Contributor Author

Sorry for that. I'll try!
I was still in figuring out where is the difference.

@luzip665 luzip665 deleted the slim-upgrade-run-subdir branch January 17, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glensc glensc glensc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luzip665 @glensc