Don't check field counts, instead only test based on the fields that exists (and
likely will never exist). This allows the protos to evolve with almost zero
chance of the tests breaking for those other changes.
This resolves issues with GCC and Clang warning (as error) about
attribute in an unallowed location, in particular when used with a
generated file using a dllexport_decl value of [[gnu::visibility]].
The correct ordering is already used in the compiler in
FileGenerator::ForwardDeclarations() for each of classes_, as well
as for other existing local values in pregenerated files. This
change ensures that all usages can compile without issue.
This will cause fields named exactly these names to get `_p` appended to them.
This is being done since the C89/C99/etc specs say these name should be macros,
so in an ObjC context, trying to access the fields would result in runtime
failures for unknown selectors. The only way the fields were usable was to use
KVC or via the descriptor based apis, but if someone hit that they likely would
have reported the general issue also.
The well-known types generate C code into wkt.inc, and this C code was
not testing isset($msg->submsg_field) like the generated code does:
```php
// PHP generated getter: checks isset().
public function getOptions()
{
return isset($this->options) ? $this->options : null;
}
```
```c
// C generated getter, does not check upb_msg_has()
static PHP_METHOD(google_protobuf_Value, getListValue) {
Message* intern = (Message*)Z_OBJ_P(getThis());
const upb_fielddef *f = upb_msgdef_ntofz(intern->desc->msgdef,
"list_value");
zval ret;
Message_get(intern, f, &ret);
RETURN_COPY_VALUE(&ret);
}
```
This led to an error where we wnuld try to get a sub-message field from upb
when it `upb_msg_has(msg, field) == false`, which is an error according to upb.
There are two possible fixes for this bug. A guiding principle is that we want
the generated C code in wkt.inc to have the same behavior as PHP generated
code. Following this principle, the two possible fixes are:
1. Change the code generator for wkt.inc to check upb_msg_has(f) before
calling Message_get(). This would match the isset() check that the
The PHP generated code does, and we would leave the PHP code unchanged.
2. Change Message_get() to itself perform the upb_msg_has(f) check for
sub-message fields. This means that generated code would no longer need
to perform an isset() check, so we would want to remove this check from
the PHP generated code also to avoid a redundant check.
Both of these are reasonable fixes, and it is not immediately obvious which is
better. (1) has the benefit of resolving this case when we are in more
specialized code (a getter function that already knows this is a sub-message
field), and therefore avoids performing the check later in more generic code
that would have to test the type again. On the other hand, the isset() check is
not needed for the pure PHP implementation, as an unset PHP variable will
return `null` anyway. And for the C extension, we'd rather check upb_msg_has()
at the C level instead of PHP.
So this change implements (2). The generated code in wkt.inc remains unchanged,
and the PHP generated code for sub-message fields is changed to remove the
isset() check.
I noticed that our JavaScript Docker image is a couple years old and
seems to have an old NPM version that may be causing problems on #8610,
so I went ahead and rebuilt the image. To do that successfully, I had to
set some environment variables in the Dockerfile so that it can build
successfully without having to ask for input about timezone
configuration. This seems to be a known issue described here:
https://serverfault.com/questions/949991/how-to-install-tzdata-on-a-ubuntu-docker-image
I also updated it to explicitly install Python since that is required
for part of our JavaScript build.
Deanna Garcia
Paul Yang
Kelvin Jin
Thomas Van Lenten
Yoav Miller
Jan Tattermusch
Jan Tattermusch
Matt Fowles Kulukundis
Joshua Haberman
Joel Johnson
Adam Cozzette
Benjamin Peterson
Petr Jediný
deannagarcia
Yuriy Chernyshov
punmy