Fix 8.1 serialization #43

bix0r · 2022-10-21T18:20:50Z

See issue #42 for information.

This fixes tests for all versions after 8.1 compatibility fixes.

This reverts commit abebab9.

Only for php8.1+. Calling the old php functions instead of mimicking the code. This is slower, but the code is simpler, and here we are only talking about when you run (un)serialize() function. The new serialize methods should throw on error. Issue: phpGH-42

Because we changed the output of the serialization in version 8.1

Copy old tests with fixes to output

Shows that the old serialized string works, and it results in the same code as the new format. Also, show that __serialize() method returns an array with 'xml' key

bix0r · 2022-10-25T09:50:09Z

@cmb69 I wonder if you can review this code for me?

cmb69 · 2022-10-25T12:13:05Z

I wonder if you can review this code for me?

I'll try to have a closer look tomorrow, but I strongly suggest to set up some CI. Likely GH actions would be the best solution.

Also consider to introduce stub files. These can be used to build the required arginfo structures automatically.

bix0r · 2022-10-25T13:04:30Z

I'll try to have a closer look tomorrow

Awesome. Thank you!

I strongly suggest to set up some CI. Likely GH actions would be the best solution

It is planned, and is pretty much next on my todo list.

Also consider to introduce stub files.

Sounds good. I will take a look.

cmb69

This looks good to me, but I've left some suggestions for your consideration.

cmb69 · 2022-10-28T16:39:36Z

src/php7/php_solr_document.c

+		RETURN_THROWS();
+	}
+	array_init(return_value);
+	add_assoc_stringl(return_value, "xml", Z_STRVAL(serialized), Z_STRLEN(serialized));


This could be simplified:

Suggested change

add_assoc_stringl(return_value, "xml", Z_STRVAL(serialized), Z_STRLEN(serialized));

add_assoc_str(return_value, "xml", serialized);

Nice. Since serialized is a zval I used add_assoc_zval

cmb69 · 2022-10-28T16:40:22Z

src/php7/php_solr_document.c

+	zend_string *key_str = zend_string_init("xml", sizeof("xml") -1, 1);
+	zend_object *object = NULL;
+	zval *tmp = NULL;
+	/* Process the parameters passed to the default constructor */


"default constructor" sounds wrong. Maybe just drop the comment?

cmb69 · 2022-10-28T16:40:46Z

src/php7/php_solr_params.c

+		RETURN_THROWS();
+	}
+	array_init(return_value);
+	add_assoc_stringl(return_value, "xml", Z_STRVAL(serialized), Z_STRLEN(serialized));


Suggested change

add_assoc_stringl(return_value, "xml", Z_STRVAL(serialized), Z_STRLEN(serialized));

add_assoc_str(return_value, "xml", serialized);

cmb69 · 2022-10-28T16:42:48Z

src/php7/php_solr.c

@@ -180,6 +180,12 @@ ZEND_BEGIN_ARG_INFO(SolrDocument_unserialize_args, 0)
 ZEND_ARG_INFO(0, serialized)
 ZEND_END_ARG_INFO()

+#if PHP_VERSION_ID >= 80100


If you want to preserve full BC with PHP < 8.1, this looks right. If, on the other hand, that would not be necessary, you could do all the new PHP 8.1 stuff unconditionally. Prior to PHP 7.4.0, it would be ignored; as of PHP 7.4.0, Serializable would be ignored.

Well, here I had many problems. I ended up with only breaking BC for 8.1 (I guess I am not really breaking anything since we were not really supporting 8.1).

If both serialize() and __serialize() are available PHP will pick the new one in 7.4+. Those two create totally different outputs, as seen in the tests I duplicated instead of changed because of this difference.

Method signature for zend_call_method() was changed in 8.0. So, I would have had to deal with that mismatch somehow.

This extension uses the unserialization process to create objects from the XML responses, so I don't really dare to change things too much :)

cmb69 · 2022-10-28T16:45:31Z

tests/029.solrdocument_serialize_php81.phpt

@@ -0,0 +1,50 @@
+--TEST--


Consider not to split the tests; it might be okay to use some placeholders for the version-dependent parts (if all else fails, you can use regular expressions inside of %r markers, e.g. %rC:20:"SolrModifiableParams"|O:12:"SolrDocument"%r).

I kinda like having the tests split because I am going with the different api for 8.1+.
Also, the difference in the serialization output is quite different between the old and the new.

C:12:"SolrDocument":172:{<?xml version="1.0" encoding="UTF-8"?>

vs.

O:12:"SolrDocument":1:{s:3:"xml";s:172:"<?xml version="1.0" encoding="UTF-8"?>

And then also the ending.

cmb69 · 2022-10-28T16:46:46Z

tests/029.solrdocument_serialize_php81.phpt

+print_r(unserialize($serializedString)->toArray());
+?>
+--EXPECT--
+O:12:"SolrDocument":1:{s:3:"xml";s:172:"<?xml version="1.0" encoding="UTF-8"?>


Generally, avoid hard-coded string lengths in tests if there are line-breaks; this might not work on Windows (LF vs. CRLF). Instead you can use a placeholder:

Suggested change

O:12:"SolrDocument":1:{s:3:"xml";s:172:"<?xml version="1.0" encoding="UTF-8"?>

O:12:"SolrDocument":1:{s:3:"xml";s:%d:"<?xml version="1.0" encoding="UTF-8"?>

I agree, but I don't want to change these tests, they are old and tried (I hope).

cmb69 · 2022-10-28T16:48:48Z

@remicollet might have some thoughts about this, too, since he noted that #30 breaks BC.

bix0r added 7 commits October 21, 2022 06:45

Revert "Removed serializeable"

2caac38

This reverts commit abebab9.

Skip serialize tests if PHP version < 8.1

b205b82

Because we changed the output of the serialization in version 8.1

Add serialize tests for PHP 8.1+

e33cce1

Copy old tests with fixes to output

Add test for php8.1 SolrDocument serialization

12fe46e

Shows that the old serialized string works, and it results in the same code as the new format. Also, show that __serialize() method returns an array with 'xml' key

Fix indentation of new code

8abbde1

Add missing release after use in __unserialize

5a245ab

bix0r marked this pull request as ready for review October 25, 2022 09:01

cmb69 reviewed Oct 28, 2022

View reviewed changes

bix0r added 2 commits October 31, 2022 12:56

Simplify adding an assoc value when serializing

96220be

Fix wrong add_assoc_ method call

5f502a5

bix0r merged commit d516f85 into php:master Nov 8, 2022

bix0r deleted the fix-8-1-serialization branch November 8, 2022 20:12

bix0r mentioned this pull request Nov 9, 2022

Tests fail because of missing serialization #42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 8.1 serialization #43

Fix 8.1 serialization #43

bix0r commented Oct 21, 2022

bix0r commented Oct 25, 2022

cmb69 commented Oct 25, 2022

bix0r commented Oct 25, 2022 •

edited

Loading

cmb69 left a comment

cmb69 Oct 28, 2022

bix0r Oct 31, 2022

cmb69 Oct 28, 2022

cmb69 Oct 28, 2022

cmb69 Oct 28, 2022

bix0r Oct 31, 2022

cmb69 Oct 28, 2022

bix0r Oct 31, 2022

cmb69 Oct 28, 2022

bix0r Oct 31, 2022

cmb69 commented Oct 28, 2022

	add_assoc_stringl(return_value, "xml", Z_STRVAL(serialized), Z_STRLEN(serialized));
	add_assoc_str(return_value, "xml", serialized);

	O:12:"SolrDocument":1:{s:3:"xml";s:172:"<?xml version="1.0" encoding="UTF-8"?>
	O:12:"SolrDocument":1:{s:3:"xml";s:%d:"<?xml version="1.0" encoding="UTF-8"?>

Fix 8.1 serialization #43

Fix 8.1 serialization #43

Conversation

bix0r commented Oct 21, 2022

bix0r commented Oct 25, 2022

cmb69 commented Oct 25, 2022

bix0r commented Oct 25, 2022 • edited Loading

cmb69 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmb69 commented Oct 28, 2022

bix0r commented Oct 25, 2022 •

edited

Loading