Use zend_std_build_properties() to access zend_object.properties

[arnaud-lb]

The zend_object.properties HashTable needs to be built just in time by calling rebuild_object_properties() on the object before accessing it. Normally this is done automatically in zend_std_get_properties(), but we do it manually in a few places, like this:

php-src/Zend/zend_API.c

Lines 1782 to 1785 in 7d99a9c

	if (!object->properties) {
	rebuild_object_properties(object);
	}
	prop = zend_hash_index_update(object->properties, h, prop);

In this PR I refactor these places to use zend_std_get_properties() instead. I also make rebuild_object_properties() static to enforce usage of zend_std_get_properties().

This refactoring will be useful in the context of the lazy objects RFC, as the zend_std_get_properties() function becomes a single entry point for zend_object.properties where we can initialize a lazy object.

cc @TimWolla

[Girgias]

ACK for SPL and PDO.

This also makes the API simpler and clearer, which is good IMHO

[zeriyoshi]

LGTM ext-random!

[SakiTakamachi]

I looked at everything except zend_vm. Looks good :)

[TimWolla]

We've talked about this before. Thank you, this LGTM.

One question though: I suggested moving zend_std_get_properties() into the header and making it static inline, because the implementation is trivial. This would require making rebuild_object_properties non-static again. Do you still intend to do that?

[staabm]

since something no-longer is ZEND_API which was before, does this need to be noted in some changelog/releasenotes ?

[arnaud-lb]

Agreed. I usually update the UPGRADING files separately, as our merge strategy for those tends to lose the changes.

[arnaud-lb]

@TimWolla

One question though: I suggested moving zend_std_get_properties() into the header and making it static inline, because the implementation is trivial. This would require making rebuild_object_properties non-static again. Do you still intend to do that?

I forgot about this part of your suggestion.

I've now inlined the zend_std_get_properties calls, but in a slightly different way: Introduced zend_std_get_properties_ex(), which is an inlined version of zend_std_get_properties(). This is similar to what we do in zend_atomics (but for different reasons). I didn't inline zend_std_get_properties() directly because we take the address of this function, and there are a few places where we compare its address like here.

An alternative would have been to introduce a non-handler function with a name like zend_get_properties, like we have for zend_get_properties_for, but I feel that code using rebuild_object_properties() want to specifically rely on the behavior of zend_std_get_properties(). Also, zend_get_properties would have used object->handlers->get_properties which defeats inlining.

[TimWolla]

LGTM, maybe add a short note to UPGRADING.INTERNALS.

[arnaud-lb]

I figured that calling rebuild_object_properties(), which is un-exported by this change, from an inline function may be problematic. So I've re-exported it, and renamed it to force external code to notice the change.

[TimWolla]

Still LGTM

[dstogov]

As the API is changed anyway, it makes sense to return HashTable and avoid extra load of zobj->properties in zend_std_get_properties[_ex].

[dstogov]

It's better to use zend_always_inline