Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Zend: Add object_init_with_constructor() API #14440
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
Zend: Add object_init_with_constructor() API #14440
Changes from all commits
5aa6d5a
b702139
6a2ef11
cebc0a2
d50f1c3
5b2946d
6d98dce
3f6e96e
bda84f4
3755151
94724be
0f2a38e
610ca73
9ad870a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should set the
IS_OBJ_DESTRUCTOR_CALLED
flag to match to behavior ornew
andReflectionClass::newInstance()
.Should we also release the object, or let the caller release it?
Currently the caller can not tell if
arg
is initialized or not whenobject_init_with_constructor()
returns FAILURE.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about
zend_object_store_ctor_failed
, I think it indeed makes sense to call it.I was thinking I had a bug in not releasing the object, but I couldn't produce it with a test so I left it, will try and see if I can come up with something and add some destructors to the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The object is useless because it's incompletely initialized, so the caller can't do much with it. I think I'd prefer it it were released and not returned at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This discussion reminded me of my PR #14161 (“Add zend_get_attribute_object()”), which also automatically destructs the object on constructor failure.
For the
#[\Deprecated]
PR, I've made a follow-up to alsoZVAL_UNDEF()
the zval then, making it safe to unconditionallyzval_ptr_dtor()
the zval no matter if the constructor failed or not:4211f4d
I would suggest the same here: If the constructor fails, the object is destroyed and the zval UNDEF'd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is what I am doing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.