Conversation
jeking3
left a comment
There was a problem hiding this comment.
Many of the Travis CI build jobs failed. That needs to be investigated and resolved.
|
Hi @jeking3 |
|
Yes, I recommend you:
This will trigger a new build against the tip. |
|
Now all build jobs are success |
jeking3
left a comment
There was a problem hiding this comment.
Thanks! Looks good and passes all tests (it might be useful to follow this up with a new functional test (see test/functional) that tries to use a structure with a required field that is not set, to see which runtimes allow it and which ones do not (such as php after applying this patch)? It would be nice to see a test verifying the new functionality.
|
Yes |
|
I'll open a story in Jira for it. (THRIFT-4142) |
| if (is_required && Z_TYPE_P(prop) == IS_NULL) { | ||
| char errbuf[128]; | ||
| snprintf(errbuf, 128, "Required field %s.%s is unset!", object_class_entry->name, varname); | ||
| throw_tprotocolexception(errbuf, INVALID_DATA); |
There was a problem hiding this comment.
I wonder if folks think it would be useful to add a new TProtocolException cause for this case, MISSING_REQUIRED, across all languages. While technically it is invalid data, I usually think of that as something got garbled.
|
When this does merge into master, it's probably worth updating the README.md indicating this is a breaking change (for the better). I would like to take a look at how some of the other runtime libraries for Thrift handle required fields as well to see what the consensus behavior is. |
jeking3
left a comment
There was a problem hiding this comment.
I did a little more research into this and found an old ticket, THRIFT-455, which led me to discover there's really no standardization on how things are handled in this area across the languages. The one takeaway I got from that is that while your changes are indeed an improvement in making required fields actually required (for the binary protocol only, from what I can tell), it is a breaking change and we should probably add a generic compiler flag that works across all languages to say, "required field validation" that can be turned on optionally (default is OFF, preserving existing behavior). Then at some time in the future when all implementations support validation, we would change the default to ON, and eventually phase out the option. Thoughts?
|
There is a falg "validate" in thrift generator for php (validate: Generate PHP validator methods\n") |
jeking3
left a comment
There was a problem hiding this comment.
Based on last comment, this logic is not engaged unless someone sets the compiler flag "validate" for php generation. This means with default flags there is no breaking change, so I'm approving.
…en validate compiler option is enabled Client: php This closes apache#1215
Hello
It is a pull request with required fields validation for php extension
Here is jira issue https://issues.apache.org/jira/browse/THRIFT-4126