PHP namespaces for nested messages and enums by bshaffer · Pull Request #4536 · protocolbuffers/protobuf

bshaffer · 2018-04-19T02:54:09Z

The proto

message TestMessage {
  NestedEnum nested_enum = 1;
  NestedMessage nested_message = 2;

  enum NestedEnum {
      ZERO = 0;
  }

  // Test nested messages
  message NestedMessage {
      int32 a = 1;
  }
}

Will result in the files

TestMessage.php
TestMessage/NestedEnum.php
TestMessage/NestedMessage.php

With namespaces corresponding to the classes, e.g.

namespace TestMessage;

class NestedEnum
{
    // ...
}

anandolee · 2018-04-30T21:01:56Z

ping @TeBoring

TeBoring · 2018-05-03T17:54:52Z

ClassNamePrefix?

TeBoring · 2018-05-03T18:04:44Z

Shall we merge the above branch and here?

TeBoring · 2018-05-03T18:05:12Z

Same as above.

TeBoring · 2018-05-03T18:09:06Z

Could you add a test in which containing message has reserved name? For example:

message Empty { message Nested {} }

TeBoring · 2018-05-03T18:10:06Z

Same as above.

TeBoring · 2018-05-03T18:17:20Z

Previously, we didn't take into account reserved name in namespace. Can we also fix it?

TeBoring · 2018-05-03T18:25:50Z

In https://github.com/google/protobuf/blob/master/php/tests/compatibility_test.sh, we need a test to verify the new runtime can work with old generated code.

TeBoring · 2018-05-04T21:25:31Z

Can we do

if (!php_namespace.empty() || (!file->options().has_php_namespace() && !file->package().empty()) || lastindex != string::npos) { ... }

Actually, I think we just need to check lastindex != string::npos

TeBoring · 2018-05-04T21:26:12Z

Same as above

xfxyjwf · 2018-05-14T20:54:03Z

Ping @TeBoring

…cesses to test

TeBoring · 2018-05-21T21:47:02Z

I think in php_generattor.cc, you need to replace FullClassName with LegacyFullClassName in several places in case depended message is still in old generated code.

TeBoring · 2018-05-22T00:05:45Z

-            $this->markTestSkipped('Protobuf extension fails when phpunit runs in a separate process');
-        }
-        $this->legacyEnum(new NestedEnum);
+        $this->legacyEnum(new TestLegacyMessage\NestedEnum);


Why not simply set new TestLegacyMessage_NestedEnum?

I think you just need new TestLegacyMessage_NestedEnum

TeBoring · 2018-05-22T00:06:18Z

-            $this->markTestSkipped('Protobuf extension fails when phpunit runs in a separate process');
-        }
-        $this->legacyMessage(new Sub);
+        $this->legacyMessage(new TestLegacyMessage\NestedMessage);


Use TestLegacyMessage_NestedMessage

TeBoring · 2018-05-22T00:26:28Z

Currently, it still generate GPBMetadata/Proto/Empty/Echo.php for empty/echo.proto

bshaffer · 2018-05-22T17:56:38Z

here is an example of the deprecated class file and message. I would love to get @dwsupplee and @michaelbausor 's thoughts on this!

michaelbausor · 2018-05-22T18:13:25Z

What is the purpose of this line: https://gist.github.com/bshaffer/906dcfbf1e05cd44ac74f0279e185ad3#file-testmessage_sub-php-L7
Is it just to make sure that class gets autoloaded?

bshaffer · 2018-05-22T18:20:48Z

@michaelbausor Yes, it ensures that if someone creates the deprecated class, the new class is loaded and the call to class_alias(new::class, old::class) is executed. So overall the file allows a user's code to continue using the old class name, but it issues a deprecated warning in any custom logger/error handler, and will show up as @deprecated in the IDE

Namespace for nested message/enum in c extension

TeBoring · 2018-05-23T23:46:44Z

@michaelbausor and @dwsupplee, if you have other issues, we can fix them in another PR.

@deprecated

* uses namespaces for nested messages and enums * fixes namespaces for PHP dist * fixes namespace for Descriptors, adds Cardinality and Kind * fixes nested namespaces for reserved words and adds tests * adds tests and generator fix for php class prefixes * fixes escaping of protobuf packages, enum comments, misc others * nice refactor of generated code * adds class files for backwards compatibility * simplifies code with templates * adds compatibility files to makefile * cleanup of generator and fixes nested namespace bug * regenerates proto types * remove internal BC classes * adds deprecated warning, adds methods back * simplifies if statement * fixes dist files * addresses review comments * adds back TYPE_URL_PREFIX constant * adds @deprecated to old nested class files * skips tests which require a separate process when protobuf.so is enabled * Adds tests for legacy nested classes that do not require separate processes to test * uses legacy names for GPBUtil message check * adds block for IDE @deprecated message * Namespace for nested message/enum in c extension * Remove unused code

@deprecated

* uses namespaces for nested messages and enums * fixes namespaces for PHP dist * fixes namespace for Descriptors, adds Cardinality and Kind * fixes nested namespaces for reserved words and adds tests * adds tests and generator fix for php class prefixes * fixes escaping of protobuf packages, enum comments, misc others * nice refactor of generated code * adds class files for backwards compatibility * simplifies code with templates * adds compatibility files to makefile * cleanup of generator and fixes nested namespace bug * regenerates proto types * remove internal BC classes * adds deprecated warning, adds methods back * simplifies if statement * fixes dist files * addresses review comments * adds back TYPE_URL_PREFIX constant * adds @deprecated to old nested class files * skips tests which require a separate process when protobuf.so is enabled * Adds tests for legacy nested classes that do not require separate processes to test * uses legacy names for GPBUtil message check * adds block for IDE @deprecated message * Namespace for nested message/enum in c extension * Remove unused code

googlebot added the cla: yes label Apr 19, 2018

xfxyjwf requested a review from TeBoring April 23, 2018 20:51

xfxyjwf assigned TeBoring Apr 23, 2018

TeBoring reviewed May 3, 2018

View reviewed changes

Comment thread src/google/protobuf/compiler/php/php_generator.cc Outdated

Copy link
Copy Markdown

Contributor

TeBoring May 3, 2018

Choose a reason for hiding this comment

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

Shall we merge the above branch and here?

TeBoring reviewed May 3, 2018

View reviewed changes

Comment thread src/google/protobuf/compiler/php/php_generator.cc Outdated

Copy link
Copy Markdown

Contributor

TeBoring May 3, 2018

Choose a reason for hiding this comment

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

Same as above.

TeBoring reviewed May 3, 2018

View reviewed changes

bshaffer force-pushed the php-submessage-namespaces branch 3 times, most recently from 7d15c80 to 3f276bb Compare May 3, 2018 21:47

TeBoring reviewed May 4, 2018

View reviewed changes

Comment thread src/google/protobuf/compiler/php/php_generator.cc Outdated

Copy link
Copy Markdown

Contributor

TeBoring May 4, 2018

Choose a reason for hiding this comment

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

Same as above

TeBoring force-pushed the php-generated-dev branch from f073b35 to a77d3a9 Compare May 15, 2018 16:50

bshaffer added 7 commits May 15, 2018 10:05

uses namespaces for nested messages and enums

81dc441

fixes namespaces for PHP dist

502039b

fixes namespace for Descriptors, adds Cardinality and Kind

04048c4

fixes nested namespaces for reserved words and adds tests

ce27965

adds tests and generator fix for php class prefixes

e677d90

fixes escaping of protobuf packages, enum comments, misc others

c905c50

nice refactor of generated code

56907d3

bshaffer force-pushed the php-submessage-namespaces branch 3 times, most recently from d07aac6 to 54c9685 Compare May 16, 2018 19:01

adds class files for backwards compatibility

46311fc

bshaffer force-pushed the php-submessage-namespaces branch from 54c9685 to 46311fc Compare May 17, 2018 14:42

bshaffer added 5 commits May 18, 2018 17:10

addresses review comments

3644039

adds back TYPE_URL_PREFIX constant

087d09d

adds @deprecated to old nested class files

e6321dc

skips tests which require a separate process when protobuf.so is enabled

48d933e

Adds tests for legacy nested classes that do not require separate pro…

f30899a

…cesses to test

uses legacy names for GPBUtil message check

d9e388c

TeBoring reviewed May 22, 2018

View reviewed changes

bshaffer and others added 4 commits May 22, 2018 11:24

adds block for IDE @deprecated message

2b25b48

Namespace for nested message/enum in c extension

a585375

Remove unused code

44804a4

Merge pull request #1 from TeBoring/php-submessage-namespaces

dccc494

Namespace for nested message/enum in c extension

TeBoring approved these changes May 23, 2018

View reviewed changes

TeBoring merged commit 6ae6e1b into protocolbuffers:php-generated-dev May 23, 2018

bshaffer deleted the php-submessage-namespaces branch May 24, 2018 18:12

xfxyjwf added the php label Jun 22, 2018

TeBoring mentioned this pull request Jun 30, 2018

Split into a sub-namespace instead of using underscores for sub-messages #3369

Closed

google-admin unassigned TeBoring Aug 21, 2018

xfxyjwf assigned TeBoring Aug 24, 2018

haberman mentioned this pull request Feb 5, 2021

Option to disable the generation of deprecated classes for PHP #8260

Closed

bshaffer mentioned this pull request Mar 7, 2022

[PHP] option exclude backwards compatible proto files #9593

Closed

Conversation

bshaffer commented Apr 19, 2018

Uh oh!

anandolee commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TeBoring commented May 3, 2018

Uh oh!

TeBoring commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xfxyjwf commented May 14, 2018

Uh oh!

TeBoring commented May 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TeBoring commented May 22, 2018

Uh oh!

bshaffer commented May 22, 2018

Uh oh!

michaelbausor commented May 22, 2018

Uh oh!

bshaffer commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TeBoring commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bshaffer commented May 22, 2018 •

edited

Loading