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

Simplify Server registration #205

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

antonkomarev
Copy link

@antonkomarev antonkomarev commented Jan 9, 2025

Overview

Simplify registration of server, because PATH_PREFIX constant declares:

Should be used with caution, it only matches routes with the default "/twirp" prefix and default CamelCase service and method names. Use EmployeeServiceServer::getPathPrefix instead.

What this PR does / why we need it

Instead of this:

$this->router->registerHandler(
    EmployeeServiceServer::PATH_PREFIX,
    new EmployeeServiceServer(svc: $this->employeeService),
);

Or this:

$employeeServiceServer = new EmployeeServiceServer(
    svc: $this->employeeService,
);

$this->router->registerHandler(
    $employeeServiceServer->getPathPrefix(),
    $employeeServiceServer,
);

We can do:

$this->router->registerServer(
    new EmployeeServiceServer(
        svc: $this->employeeService,
    )
);

Special notes for your reviewer

New ServerInterface may have new method getMethodPath described here:

Does this PR introduce a user-facing change?

New ability to register server without manually define server path.

@antonkomarev antonkomarev marked this pull request as draft January 9, 2025 11:00
@antonkomarev
Copy link
Author

antonkomarev commented Jan 9, 2025

@sagikazarmark marked as Draft because no tests included and documentation should be updated too. If this proposal is applicable, will invest more time for it.


/**
* @see {{ .Service | phpServiceName .File }}
*
* Generated from protobuf service <code>{{ .Service.Desc.FullName }}</code>
*/
final class {{ .Service | phpServiceName .File }}Server implements RequestHandlerInterface
final class {{ .Service | phpServiceName .File }}Server implements ServerInterface
Copy link
Author

Choose a reason for hiding this comment

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

Swapped it to the new interface, because it is extending RequestHandlerInterface

@sagikazarmark
Copy link
Member

Thanks @antonkomarev !

I think this makes sense, although in the past I avoided adding additional dependency on the shared library on purpose, mostly to avoid incompatibility between the generated code and the shared library.

Let me sleep on it before accepting.

@sagikazarmark
Copy link
Member

Looks like I described that exact same solution in #89 though, but never got to implementing it...probably because it meant adding an interface to the runtime library.

@antonkomarev
Copy link
Author

antonkomarev commented Jan 13, 2025

@sagikazarmark so, you didn't changed your mind from then? )

I think it's redundant to define prefix manually if we already have it in a service object.

@sagikazarmark
Copy link
Member

I haven't really given it much thought. I can see how the current UX is not ideal. I'm leaning towards accepting this, I just want to give it some thought to see if we can find an alternate solution.

@antonkomarev
Copy link
Author

antonkomarev commented Jan 19, 2025

@sagikazarmark what's wrong with the external interface? Code is generated and doesn't mean to be committed to the application repository. Maybe only in tests.

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

Successfully merging this pull request may close these issues.

2 participants