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

Path prefix doesn't work #126

Open
1 of 2 tasks
ilzrv opened this issue Jun 24, 2023 · 3 comments
Open
1 of 2 tasks

Path prefix doesn't work #126

ilzrv opened this issue Jun 24, 2023 · 3 comments

Comments

@ilzrv
Copy link

ilzrv commented Jun 24, 2023

Preflight Checklist

  • I have searched the issue tracker for an issue that matches the one I want to file, without success.
  • I am not looking for support or already pursued the available support channels without success.

Version

v.0.9.1

PHP version

7.4

Go version

No response

Expected Behavior

When the path prefix is changed, the library continues to work.

Actual Behavior

When changing the path prefix, the library does not work.

Steps To Reproduce

  1. Add to the .proto file package apis.inter.services;
  2. Register a handler
$this->router->registerHandler(
    MyServer::PATH_PREFIX,
    new MyServer($this->service)
);
  1. After generating the MyServer class in the PATH_PREFIX constant:
final class MyServer implements RequestHandlerInterface
{
    const PATH_PREFIX = '/twirp/apis.inter.services.My/';
//...
  1. But below the code (MyServer) we have a switch:
switch ($req->getUri()->getPath()) {
    case '/twirp/apis.inter.services.My/Create': // <-- full path?
        return $this->handleCreate($ctx, $req);

    default:
        return $this->writeError($ctx, $this->noRouteError($req));
}

This switch does not use path prefix from constructor in ANY way, which we specify when registering the handler. Looks like a serious mistake. Why pass a prefix if it's always the same?

Additional Information

No response

@sagikazarmark
Copy link
Member

This is probably a regression we introduced when upgrading from v6 spec to v7. v7 made the prefix optional...I guess we did not change all necessary code to reflect that.

Let me look into it.

@sagikazarmark
Copy link
Member

Actually, @ilzrv are you sure you are using 0.9.1?

As far as I can tell, the prefix change was merged way before that version.

Also, I'm looking at code generated by that version (downloaded from the releases page) and the routing mechanism looks completely different.

Can you please confirm your plugin version by running protoc-gen-twirp_php --version?

Thank you!

@ilzrv
Copy link
Author

ilzrv commented Aug 8, 2023

The result of executing the ./proto/bin/protoc-gen-twirp_php --version command is: 0xc0000296b0

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

No branches or pull requests

2 participants