Zend: Add object_init_with_constructor() API #14440

Girgias · 2024-06-02T15:37:00Z

This will instantiate the object and execute its constructor with the given parameters.

As it came up in #14418

Edit: I probably also should add support for a named argument HashTable.

Zend/zend_API.c

arnaud-lb · 2024-06-03T12:54:58Z

Zend/zend_API.c

+		named_params
+	);
+	if (Z_TYPE(retval) == IS_UNDEF) {
+		return FAILURE;


We should set the IS_OBJ_DESTRUCTOR_CALLED flag to match to behavior or new and ReflectionClass::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 when object_init_with_constructor() returns FAILURE.

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.

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.

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 also ZVAL_UNDEF() the zval then, making it safe to unconditionally zval_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.

Yeah this is what I am doing

arnaud-lb · 2024-06-03T13:03:33Z

Zend/zend_API.c

+	zend_object *obj = Z_OBJ_P(arg);
+	zend_function *constructor = obj->handlers->get_constructor(obj);
+	if (UNEXPECTED(constructor == NULL)) {
+		return FAILURE;


Depending on the use-case of this function, I think that returning SUCCESS here when no arguments were passed, would be a better API. Otherwise we can not use this function as an internal equivalent of new without checking the existence of a constructor before that.

ReflectionClass::newInstance() does that, and throws Class %s does not have a constructor, so you cannot pass any constructor arguments when arguments where passed.

This is actually a good point, really wondering if the SPL API that was exposed was not completely broken in reality now...

I am wondering, should we throw an exception, or just allow it to initialize like a new call with too many arguments? As this is not currently an error in PHP: https://3v4l.org/CHps3, and it would be inconsistent with passing too many parameters which would initialize it.

In this case I would prefer to follow the behavior of new, then. Because if this is used by user-facing APIs, users would expect that behavior.

Actually I think we need to handle internal and userland classes differently.

Need to add a test for this when I come back to work on it.

Girgias · 2024-06-04T01:01:10Z

So it turns out, initializing an object by calling the constructor and have the behaviour be identical to new is really fricking hard, because the semantics are all over the place.

I must say, I'm not sure the get_constructor object handler is that useful, as it is only overloaded by opaque objects to throw an Error to indicate that it cannot be initialized and another extension function must be called.

Every other object uses the standard object hook provided by Zend.

I can see a couple of ways to improve the semantics here:

For classes that do not define an explicit constructor, have zend_std_get_constructor return the internal pass function zend_pass_function
Make zend_pass_function the default for the constructor field of CEs
Have opaque objects implement a public constructor that throws the same Error as it currently does in their get_constructor handler
Have NULL for the constructor field of a CE mean the object cannot be initialized traditionally, this would mean foregoing the custom Error exception messages

I must say, it is also slightly surprising that internal classes that don't define a constructor can take arbitrary amount of positional arguments, this feels like it goes against what one would normally expect of internal objects/functions

ndossche · 2024-06-05T16:10:29Z

Zend/zend_API.c

+				/* Throw standard Error */
+				zend_string *arg_name = NULL;
+					zend_hash_get_current_key(named_params, &arg_name, /* num_index */ NULL);
+				ZEND_ASSERT(arg_name != NULL);


I think it's actually allowed to pass an empty hash table to the underlying zend_call_known_function function.
And API users might expect the same to hold for this function.
However, in that case this line will fail.

Indeed, fixed this.

iluuu1994

Can this new function now be used somewhere? That would prove its correctness better than the test.

Zend/zend_API.c

Girgias · 2024-06-06T14:36:36Z

Can this new function now be used somewhere? That would prove its correctness better than the test.

I would use it in the places in #14418 and @TimWolla has told me it would be convenient for the Deprecated Attribute.

TimWolla · 2024-06-06T14:40:38Z

has told me it would be convenient for the Deprecated Attribute.

More specifically for error handling in zend_get_attribute_object, because it currently leaves a broken object in case the constructor fails, instead of setting the zval to UNDEF.

This will instantiate the object and execute its constructor with the given parameters.

Found an existing bug with private constructors

Turns out the logic is really baffling

iluuu1994

LGTM. I find the control flow a bit confusing, I would prefer a common failure path:
https://gist.github.com/iluuu1994/8e2ae4715ef29176c2b97c37443d0e2f

But I'll leave it up to you if that's something you want to do.

Girgias requested review from dstogov and iluuu1994 as code owners June 2, 2024 15:37

github-actions bot added the Category: Engine label Jun 2, 2024

TimWolla reviewed Jun 2, 2024

View reviewed changes

Zend/zend_API.c Outdated Show resolved Hide resolved

TimWolla reviewed Jun 2, 2024

View reviewed changes

Zend/zend_API.c Outdated Show resolved Hide resolved

ndossche reviewed Jun 2, 2024

View reviewed changes

Zend/zend_API.c Outdated Show resolved Hide resolved

Zend/zend_API.c Outdated Show resolved Hide resolved

Girgias force-pushed the object_init_constructor_api branch from f5c8ecb to 05ae5ed Compare June 3, 2024 00:21

Girgias requested a review from kocsismate as a code owner June 3, 2024 00:21

github-actions bot added the Extension: zend_test label Jun 3, 2024

arnaud-lb reviewed Jun 3, 2024

View reviewed changes

Girgias mentioned this pull request Jun 3, 2024

Attempting to initialize class with private constructor calls destructor #14456

Closed

Girgias force-pushed the object_init_constructor_api branch from ab85fe1 to 43e2892 Compare June 3, 2024 16:25

TimWolla approved these changes Jun 5, 2024

View reviewed changes

ndossche reviewed Jun 5, 2024

View reviewed changes

iluuu1994 reviewed Jun 5, 2024

View reviewed changes

Zend/zend_API.c Outdated Show resolved Hide resolved

Zend/zend_API.c Outdated Show resolved Hide resolved

Zend/zend_API.c Outdated Show resolved Hide resolved

Zend/zend_API.c Show resolved Hide resolved

Girgias added 12 commits June 6, 2024 15:52

Zend: Add object_init_with_constructor() API

5aa6d5a

This will instantiate the object and execute its constructor with the given parameters.

Support named arguments and return failure on exception

b702139

Test new API and fix issues encountered in testing

6a2ef11

nit

cebc0a2

Fix memory leak on case of failure

d50f1c3

Fix test description

5b2946d

Test abstract class initialization failure

6d98dce

Add destructors

3f6e96e

Do not run destructors when an exception has been raised

bda84f4

Found an existing bug with private constructors

Deal with NULL constructor properly

3755151

Fix behaviour with new for internal and userland classes

94724be

Turns out the logic is really baffling

Fix test after rebase

0f2a38e

Girgias added 2 commits June 6, 2024 16:03

Fix review nits

610ca73

Allow empty HashTable

9ad870a

Girgias force-pushed the object_init_constructor_api branch from 73c3cb7 to 9ad870a Compare June 6, 2024 15:13

iluuu1994 approved these changes Jun 6, 2024

View reviewed changes

Girgias merged commit 51379d6 into php:master Jun 6, 2024

Girgias deleted the object_init_constructor_api branch June 6, 2024 20:21

Girgias mentioned this pull request Jun 6, 2024

ext/spl: Remove spl_engine.h header #14418

Merged

Zend: Add object_init_with_constructor() API #14440

Zend: Add object_init_with_constructor() API #14440

Uh oh!

Conversation

Girgias commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arnaud-lb Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

arnaud-lb Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias commented Jun 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Girgias commented Jun 6, 2024

Uh oh!

TimWolla commented Jun 6, 2024

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Girgias commented Jun 2, 2024 •

edited

Loading

arnaud-lb Jun 3, 2024 •

edited

Loading

arnaud-lb Jun 3, 2024 •

edited

Loading

iluuu1994 left a comment •

edited

Loading