Skip to content
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

Tests fail because of missing serialization #42

Closed
bix0r opened this issue Oct 21, 2022 · 2 comments
Closed

Tests fail because of missing serialization #42

bix0r opened this issue Oct 21, 2022 · 2 comments
Assignees

Comments

@bix0r
Copy link
Collaborator

bix0r commented Oct 21, 2022

In commit abebab9 the Serializable interface was removed.
Turns out that this creates problems because we are using serialization for internal things.

Failing tests:

SolrDocument::hasChildDocuments() - Method test [tests/025.solrdocument_haschilddocuments.phpt]
SolrDocument::getChildDocumentsCount() - Method test [tests/026.solrdocument_getchilddocscount.phpt]
SolrDocument::getInputDocument() - Where document has child docs [tests/027.solrdocument_getinputdocument_children.phpt]
SolrDocument - clone [tests/028.solrdocument_clone.phpt]
SolrDocument - clone [tests/029.solrdocument_serialize.phpt]
SolrDocument::clear - Remove all fields from the document [tests/031.solrdocument_clear.phpt]
SolrDocument::fieldExists [tests/032.solrdocument_fieldexists.phpt]
SolrDocument::getField [tests/037.solrdocument_getfield.phpt]
SolrResponse - ParseMode [tests/101.solrresponse_parseMode.phpt]
SolrDocument - Response parsed as SolrDocument with child documents [tests/106.solrresponse_child_doc_response_solrdoc.phpt]
Solr - Fetch and Update nested documents [tests/160.solr_update_document_block.phpt]
SolrParams::serialize() - serialize params [tests/196.solrparams_serialize.phpt]
SolrParams::unserialize() [tests/197.solrparams_unserialize.phpt]

Example error:

---- EXPECTED OUTPUT
bool(true)
---- ACTUAL OUTPUT
Warning: Class SolrDocument has no unserializer in /tmp/pecl-search_engine-solr/tests/025.solrdocument_haschilddocuments.php on line 7

Warning: Class SolrDocument has no unserializer in /tmp/pecl-search_engine-solr/tests/025.solrdocument_haschilddocuments.php on line 7

Warning: Class SolrDocument has no unserializer in /tmp/pecl-search_engine-solr/tests/025.solrdocument_haschilddocuments.php on line 7

Warning: SolrDocument::hasChildDocuments(): Invalid Document Index 0. HashTable index does not exist. in /tmp/pecl-search_engine-solr/tests/025.solrdocument_haschilddocuments.php on line 8

Warning: SolrDocument::hasChildDocuments(): Internal Error 1008 generated from /tmp/pecl-search_engine-solr/src/php7/solr_functions_helpers.c 214 solr_fetch_document_entry. The observed error is a possible side-effect of an illegal/unsupported operation in userspace. Please check the documentation and try again later. in /tmp/pecl-search_engine-solr/tests/025.solrdocument_haschilddocuments.php on line 8

Fatal error: SolrDocument::hasChildDocuments(): Unable to fetch document entry for current object in /tmp/pecl-search_engine-solr/tests/025.solrdocument_haschilddocuments.php on line 8

I am afraid to make a release with this error even though I've been running this code for a few months on a dev machine.

@bix0r bix0r self-assigned this Oct 21, 2022
@bix0r
Copy link
Collaborator Author

bix0r commented Oct 21, 2022

I think I have a possible fix brewing... I hope I will create PR soon.

I currently think that we should just add the new Serializable methods, but that is a bit problematic since they are not compatible.
See the SolrDocument::serialize() test: for current serialize format. There we see that we have a 'C' format with a string value. Because the new __serialize() requires an array, we have a mismatch.

Note that when unserializing php for the old format, php will pick the old unserialize() method instead of the new one.

Here is the idea for my fix:

  • Add __serialize() and __unserialize() only for PHP-8.1
  • Make __serialize() call the old serialize() and return an array like this ['xml' => $xml]
  • Make __unserialize() require and extract a xml array key with the xml String, and then call the old serialize() with that xml string

By only adding the methods for 8.1+, we avoid possible problems in 7.4 and 8.0.

bix0r added a commit to bix0r/pecl-search_engine-solr that referenced this issue Oct 21, 2022
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
bix0r added a commit to bix0r/pecl-search_engine-solr that referenced this issue Oct 21, 2022
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
@bix0r
Copy link
Collaborator Author

bix0r commented Nov 9, 2022

Fixed in PR #43

@bix0r bix0r closed this as completed Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant