Skip to content

Implement JsonSerializable for SplFixedArray #7117

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Implement JsonSerializable for SplFixedArray #7117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b98e802
Implement JsonSerializable for SplFixedArray
nikic Jun 8, 2021
056fe2b
Register dep on json in config.m4
nikic Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ext/spl/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ PHP_NEW_EXTENSION(spl, php_spl.c spl_functions.c spl_engine.c spl_iterators.c sp
PHP_INSTALL_HEADERS([ext/spl], [php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h])
PHP_ADD_EXTENSION_DEP(spl, pcre, true)
PHP_ADD_EXTENSION_DEP(spl, standard, true)
PHP_ADD_EXTENSION_DEP(spl, json)
10 changes: 7 additions & 3 deletions ext/spl/php_spl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,9 +735,14 @@ PHP_RSHUTDOWN_FUNCTION(spl) /* {{{ */
return SUCCESS;
} /* }}} */

/* {{{ spl_module_entry */
static const zend_module_dep spl_deps[] = {
ZEND_MOD_REQUIRED("json")
ZEND_MOD_END
};

zend_module_entry spl_module_entry = {
STANDARD_MODULE_HEADER,
STANDARD_MODULE_HEADER_EX, NULL,
spl_deps,
"SPL",
ext_functions,
PHP_MINIT(spl),
Expand All @@ -748,4 +753,3 @@ zend_module_entry spl_module_entry = {
PHP_SPL_VERSION,
STANDARD_MODULE_PROPERTIES
};
/* }}} */
16 changes: 15 additions & 1 deletion ext/spl/spl_fixedarray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "spl_fixedarray.h"
#include "spl_exceptions.h"
#include "spl_iterators.h"
#include "ext/json/php_json.h"

zend_object_handlers spl_handler_SplFixedArray;
PHPAPI zend_class_entry *spl_ce_SplFixedArray;
Expand Down Expand Up @@ -758,6 +759,18 @@ PHP_METHOD(SplFixedArray, getIterator)
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
}

PHP_METHOD(SplFixedArray, jsonSerialize)
{
ZEND_PARSE_PARAMETERS_NONE();

spl_fixedarray_object *intern = Z_SPLFIXEDARRAY_P(ZEND_THIS);
array_init_size(return_value, intern->array.size);
for (zend_long i = 0; i < intern->array.size; i++) {
zend_hash_next_index_insert_new(Z_ARR_P(return_value), &intern->array.elements[i]);
Z_TRY_ADDREF(intern->array.elements[i]);
}
}

static void spl_fixedarray_it_dtor(zend_object_iterator *iter)
{
zval_ptr_dtor(&iter->data);
Expand Down Expand Up @@ -838,7 +851,8 @@ zend_object_iterator *spl_fixedarray_get_iterator(zend_class_entry *ce, zval *ob

PHP_MINIT_FUNCTION(spl_fixedarray)
{
spl_ce_SplFixedArray = register_class_SplFixedArray(zend_ce_aggregate, zend_ce_arrayaccess, zend_ce_countable);
spl_ce_SplFixedArray = register_class_SplFixedArray(
zend_ce_aggregate, zend_ce_arrayaccess, zend_ce_countable, php_json_serializable_ce);
spl_ce_SplFixedArray->create_object = spl_fixedarray_new;
spl_ce_SplFixedArray->get_iterator = spl_fixedarray_get_iterator;
spl_ce_SplFixedArray->ce_flags |= ZEND_ACC_REUSE_GET_ITERATOR;
Expand Down
4 changes: 3 additions & 1 deletion ext/spl/spl_fixedarray.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

/** @generate-class-entries */

class SplFixedArray implements IteratorAggregate, ArrayAccess, Countable
class SplFixedArray implements IteratorAggregate, ArrayAccess, Countable, JsonSerializable
{
public function __construct(int $size = 0) {}

Expand Down Expand Up @@ -49,4 +49,6 @@ public function offsetSet($index, mixed $value) {}
public function offsetUnset($index) {}

public function getIterator(): Iterator {}

public function jsonSerialize(): array {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is I think more a question of convention. The JsonSerializable interface defines jsonSerialize() as having a mixed return type. Even though this implementation does only ever return an array, will it cause issues if someone wanted to extend SplFixedArray and override jsonSerialize() with a different return type like JsonSerializable?

I can't think of many situations where there would be a reason someone would have to do this, but I think if they were to be using this stub for intelisense it might complain that the child's method is incompatible with SplFixedArrays even though it should technically be allowed by the interface.

This is the deepest I've ever seen into PHPs internals so I don't know how else the stubs are used and whether running that hypothetical example would actually generate any errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a child class will not be allowed to widen the type. I guess if we really want we can declare this as a tentative type instead, so that any code hypothetically extending SplFixedArray with a different return type would not be immediately broken.

cc @kocsismate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting problem.. Personally, I don't have many too much concern about the array return type (I'm not sure how common is to extend SplFixedArray + implement JsonSerializable), but if we want to be extra safe, a tentative return type seems like a good idea.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it is worth, I am not overtly concerned either. I just saw it and wasn't sure if it was an overt decision to deviate from the interface.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll go with the explicit type here and change it to a tentative one if anyone complains that it broke their code :) It seems pretty unlikely to me.

}
11 changes: 8 additions & 3 deletions ext/spl/spl_fixedarray_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: aeac254f38638c19a11f7d79ac2e5c2d40924e58 */
* Stub hash: 115b2d974b18287654be925c4fdc2236674423eb */

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_SplFixedArray___construct, 0, 0, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, size, IS_LONG, 0, "0")
Expand Down Expand Up @@ -39,6 +39,9 @@ ZEND_END_ARG_INFO()
ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_class_SplFixedArray_getIterator, 0, 0, Iterator, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_SplFixedArray_jsonSerialize, 0, 0, IS_ARRAY, 0)
ZEND_END_ARG_INFO()


ZEND_METHOD(SplFixedArray, __construct);
ZEND_METHOD(SplFixedArray, __wakeup);
Expand All @@ -52,6 +55,7 @@ ZEND_METHOD(SplFixedArray, offsetGet);
ZEND_METHOD(SplFixedArray, offsetSet);
ZEND_METHOD(SplFixedArray, offsetUnset);
ZEND_METHOD(SplFixedArray, getIterator);
ZEND_METHOD(SplFixedArray, jsonSerialize);


static const zend_function_entry class_SplFixedArray_methods[] = {
Expand All @@ -67,16 +71,17 @@ static const zend_function_entry class_SplFixedArray_methods[] = {
ZEND_ME(SplFixedArray, offsetSet, arginfo_class_SplFixedArray_offsetSet, ZEND_ACC_PUBLIC)
ZEND_ME(SplFixedArray, offsetUnset, arginfo_class_SplFixedArray_offsetUnset, ZEND_ACC_PUBLIC)
ZEND_ME(SplFixedArray, getIterator, arginfo_class_SplFixedArray_getIterator, ZEND_ACC_PUBLIC)
ZEND_ME(SplFixedArray, jsonSerialize, arginfo_class_SplFixedArray_jsonSerialize, ZEND_ACC_PUBLIC)
ZEND_FE_END
};

static zend_class_entry *register_class_SplFixedArray(zend_class_entry *class_entry_IteratorAggregate, zend_class_entry *class_entry_ArrayAccess, zend_class_entry *class_entry_Countable)
static zend_class_entry *register_class_SplFixedArray(zend_class_entry *class_entry_IteratorAggregate, zend_class_entry *class_entry_ArrayAccess, zend_class_entry *class_entry_Countable, zend_class_entry *class_entry_JsonSerializable)
{
zend_class_entry ce, *class_entry;

INIT_CLASS_ENTRY(ce, "SplFixedArray", class_SplFixedArray_methods);
class_entry = zend_register_internal_class_ex(&ce, NULL);
zend_class_implements(class_entry, 3, class_entry_IteratorAggregate, class_entry_ArrayAccess, class_entry_Countable);
zend_class_implements(class_entry, 4, class_entry_IteratorAggregate, class_entry_ArrayAccess, class_entry_Countable, class_entry_JsonSerializable);

return class_entry;
}
18 changes: 18 additions & 0 deletions ext/spl/tests/splfixedarray_json_encode.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
--TEST--
json_encode() on SplFixedArray
--FILE--
<?php

echo json_encode(new SplFixedArray()) . "\n";
echo json_encode(new SplFixedArray(1)) . "\n";

$a = new SplFixedArray(3);
$a[0] = 0;
$a[2] = 2;
echo json_encode($a) . "\n";

?>
--EXPECT--
[]
[null]
[0,null,2]