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.

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

#19 fix appended site path beeing ignored #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tgaertner
Copy link

No description provided.

@mbrodala
Copy link
Member

Nice catch, we did basically drop the site base path (general and languages) here. 👍

@mbrodala
Copy link
Member

Are you able to adjust the CLI tests to haven an error which your change fixes? I tried myself with a custom site base URL but that didn't work reliably ...

@tgaertner
Copy link
Author

A little readme in the project how to setup the test environment would be helpful. I started the docker compose environment and run into a php version mismatch.

@mbrodala
Copy link
Member

Indeed, added now: e23379a

@@ -29,7 +29,9 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
{
$action = new OpcacheAction(OpcacheAction::RESET);

if (strtolower($request->getMethod()) !== $action->getRequestMethod() || $request->getUri()->getPath() !== $action->getUriPath()) {
$resetPathDetected = substr_compare($request->getUri()->getPath(), $action->getUriPath(), -(strlen($action->getUriPath())), strlen($action->getUriPath())) === 0;
Copy link
Member

Choose a reason for hiding this comment

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

This is impossible to read and understand. ;-) Maybe use str_(starts|ends)_with()?

Also currently it sounds like we stop processing if "our" path is detected which is contrary to what actually happens.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for pushing this wip-code to the PR.

Well str_ends_with() makes it incompatible with PHP 7.4 ...
For making it readable I introduces a speaking variable. But I'm happy about a better solution :-)

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind requiring symfony/polyfill-php80 for this.

@tgaertner
Copy link
Author

tgaertner commented Jan 31, 2023

Sadly I was still not able to execute the tests locally . Build is crashing because phpstan is complaining.

Running "docker compose run --rm app composer test" crashes with an connection error. Sorry I'm not too deep into docker compose.

BTW. on my Linux host the command has to be "docker compose" (without the minus)

@mbrodala
Copy link
Member

You can use both the standalone (Python) as well as embedded (Go) version of Docker Compose. Which actually works fine everywhere, see e.g. the CI here. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants