Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: use of fully qualified names in place of imports #30

Merged
merged 1 commit into from
May 25, 2022

Conversation

ryanjcohen
Copy link
Contributor

@ryanjcohen ryanjcohen commented May 25, 2022

Reason for This PR

An issue with conflicting imports was found while using the protoc-gen-php-grpc plugin. For example, the plugin can potentially create a php file with both of the following imports.

use A\Proto;
use B\Proto; 

This can become problematic, for example, when the return type for a function in the generated php file is specified as Proto\Message

As a solution to these issues, this commit uses fully qualified names in generated php files in place of imports.

Description of Changes

  • Removed use of imports in generated php files and replaced with use of fully qualified names
  • Made corresponding changes to files used for testing

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

@rustatian rustatian changed the title Added use of fully qualified names in place of imports feat: use of fully qualified names in place of imports May 25, 2022
@rustatian rustatian self-requested a review May 25, 2022 18:03
@rustatian rustatian added the enhancement New feature or request label May 25, 2022
@rustatian
Copy link
Member

Hey @ryanjcohen 👋🏻 . Thanks for your contribution. Could you please also send a PR to the docs: https://github.com/spiral/roadrunner-docs/blob/2.x/docs/en/plugins/grpc.md ?

@ryanjcohen
Copy link
Contributor Author

👋🏻 I've opened this PR for grpc.md. Could you please clarify any additional changes to the PHP Client subsection that are needed?

@rustatian
Copy link
Member

As far as I understand, there are no changes in the worker sample (from the gRCP section):

<?php

/**
 * Sample GRPC PHP server.
 */

use Service\EchoInterface;
use Spiral\RoadRunner\GRPC\Server;
use Spiral\RoadRunner\Worker;

require __DIR__ . '/vendor/autoload.php';

$server = new Server(null, [
    'debug' => false, // optional (default: false)
]);

$server->registerService(EchoInterface::class, new EchoService());

$server->serve(Worker::create());

Am I right?

@ryanjcohen
Copy link
Contributor Author

ryanjcohen commented May 25, 2022

That is also my understanding. The changes are intended specifically for service generation by the protoc-gen-php-grpc plugin

@rustatian
Copy link
Member

@ryanjcohen Ok, thanks. I'm asking because I'm not a PHP dev 😄
All tests are green, so thanks, approved. We release patches every Thursday, so your patch was on time and will be released tomorrow.

Copy link
Member

@rustatian rustatian left a comment

Choose a reason for hiding this comment

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

lgtm

@rustatian rustatian merged commit 99374dc into roadrunner-server:master May 25, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants