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

Add "readonly" as a keyword for PHP and add previous classnames to descriptor pool #10041

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented May 25, 2022

See #10038

This PR adds compatibility for PHP 8.1 by identifying "readonly" as a reserved word, and by adding forwards compatibility for any ReadOnly classes generated with a previous protobuf version (see new tests / new test protos).

It should be noted that in the past when we added a new keyword, forwards compatibility was not preserved. We only realized it now because we have several libraries (Spanner/Datastore/Firestore) which use the keyword readonly for message names.

Summary

BC file for the previously unreserved classnames

In the file ReadOnly.php, where there used to generate class ReadOnly, there is now the following code to create an alias for the new classname:

<?php
# Generated by the protocol buffer compiler.  DO NOT EDIT!
# source: proto/test_reserved_message_lower.proto

namespace Lower;

class_exists(PBReadOnly::class); // autoload the new class, which will also create an alias to the deprecated class
@trigger_error(__NAMESPACE__ . '\ReadOnly is deprecated and will be removed in the next major release. Use PBReadOnly instead', E_USER_DEPRECATED);

This ensures that anyone still using the name Readme on previous versions of PHP will not be broken.

Alias declaration in new class file to the previously unreserved classnames

Likewise, at the bottom of the new PBReadOnly.php file/class, we create an alias to the old one:

/**
 * Generated from protobuf message <code>readonly</code>
 */
class PBReadOnly extends \Google\Protobuf\Internal\Message
{

    /**
     * Constructor.
     *
     * @param array $data {
     *     Optional. Data for populating the Message object.
     *
     * }
     */
    public function __construct($data = NULL) {
        \GPBMetadata\Proto\TestReservedMessageLower::initOnce();
        parent::__construct($data);
    }

}

// Adding a class alias for backwards compatibility with the "readonly" keyword.
class_alias(ReadOnly::class, __NAMESPACE__ . '\ReadOnly');

NOTE: The classname "readonly" is still valid, but the syntax for declaring it (e.g. class ReadOnly {}) is not. That means that creating an alias for it, and instantiating the class directly, is still valid in PHP 8.1.

Previously unreserved classnames

The previously unreserved classnames function the same way as any other type of reserved word, with the exception that they also get added to the descriptor pool. This is to maintain backwards compatibility.

In order for Protobuf to be able to know and handle any messages defined using the previous classname (e.g. ReadOnly) for versions of any library that are still using the older versions, we've added the previous names to the descriptor pool in both the native PHP and the C-extension implementations.

@bshaffer bshaffer force-pushed the 21.x-php-add-previous-classname-to-descriptor-pool branch from 71f2b49 to 7ed2c26 Compare May 31, 2022 22:22
@bshaffer
Copy link
Contributor Author

bshaffer commented Jun 7, 2022

Looks like I need to add some of the new test files to the distribution file! Should be an easy fix.

@bshaffer bshaffer changed the title 21.x php add previous classname to descriptor pool Add "readonly" as a keyword for PHP and add previous classnames to descriptor pool Jun 7, 2022
@bshaffer
Copy link
Contributor Author

bshaffer commented Jun 8, 2022

@haberman This is ready for another test run!

@zeriyoshi
Copy link
Contributor

make: *** [initialize_submodule] Error 1

It seems to be a network problem or something.
Re-run kokoro, maybe it will succeed.

@bshaffer
Copy link
Contributor Author

bshaffer commented Jun 9, 2022

@zeriyoshi I see the following errors:

-- Performing Test HAVE_CXX_FLAG_WD654 - Failed
-- Performing Test HAVE_THREAD_SAFETY_ATTRIBUTES -- failed to compile
-- Performing Test HAVE_GNU_POSIX_REGEX -- failed to compile
CMake Error at CMakeLists.txt:34 (message):
  Did not find Google Test sources! Either pass correct path in
  GOOGLETEST_PATH, or enable BENCHMARK_DOWNLOAD_DEPENDENCIES, or disable
  BENCHMARK_USE_BUNDLED_GTEST, or disable BENCHMARK_ENABLE_GTEST_TESTS /
  BENCHMARK_ENABLE_TESTING.

Could these be what is causing the make error?

@bshaffer bshaffer force-pushed the 21.x-php-add-previous-classname-to-descriptor-pool branch from e496be7 to 5f8e6df Compare June 9, 2022 17:25
@bshaffer
Copy link
Contributor Author

bshaffer commented Jun 9, 2022

@zeriyoshi okay I fixed it, for some reason in my commit there was an update to the third_party/benchmarks submodule. I don't know how that got in there, but I removed the change and that should do the trick!

php/ext/google/protobuf/def.c Show resolved Hide resolved
php/ext/google/protobuf/names.c Outdated Show resolved Hide resolved
php/ext/google/protobuf/names.c Outdated Show resolved Hide resolved
php/ext/google/protobuf/names.c Outdated Show resolved Hide resolved
php/ext/google/protobuf/names.c Outdated Show resolved Hide resolved
php/ext/google/protobuf/protobuf.c Show resolved Hide resolved
src/google/protobuf/compiler/php/php_generator.cc Outdated Show resolved Hide resolved
"new", fullname,
"old", message->name());
LegacyReadOnlyGenerateClassFile(file, message, options, generator_context);
}
Copy link
Member

Choose a reason for hiding this comment

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

this looks to duplicate the code above. Can you add well named helper?

Copy link
Contributor Author

@bshaffer bshaffer Jul 12, 2022

Choose a reason for hiding this comment

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

There are subtle enough differences that I do not think it would be advantageous to add a helper

@bshaffer
Copy link
Contributor Author

@fowles I've addressed all your review comments, PTAL (also @haberman)

@bshaffer bshaffer requested review from haberman and fowles July 13, 2022 18:21
@bshaffer bshaffer force-pushed the 21.x-php-add-previous-classname-to-descriptor-pool branch from c581801 to 4f28c5a Compare July 13, 2022 18:36
@haberman haberman merged commit 7b623fa into protocolbuffers:21.x Jul 18, 2022
@bshaffer bshaffer deleted the 21.x-php-add-previous-classname-to-descriptor-pool branch July 18, 2022 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants