Branch tests are failing on all environments with PHP 7.3, due to a notice in unserialize.

https://www.drupal.org/node/2348769/qa
https://www.drupal.org/pift-ci-job/1643959

CommentFileSizeAuthor
#5 3146507-4.patch1.18 KBpfrenssen
#9 3146507-9-facet-serialization.patch534 bytesDeFr

Comments

DeFr created an issue. See original summary.

DeFr’s picture

DeFr commented

I can reproduce those fails locally, but ironically enough only in D8.9 with phpunit 7, they're passing just fine with on D9.0 / phpunit 8.

I'm probably going to look into those on Friday if no one post a patch before that.

DeFr’s picture

DeFr commented

So, I don't have a patch yet, but I'm going to post additional informations I've gathered for now :

  • This notices occurs in PHP 7.3 and PHP 7.4 ; no warning in PHP 7.2. I think we're hitting https://bugs.php.net/bug.php?id=77302 , which means that in previous PHP versions, things are still not really quite right, but they're failing silently
  • The problem can be reproduced outside the testing framework
  • It occurs everytime the widget of the facet is changed and the form is rebuilt by the AJAX callback ; with JS disabled, the problem doesn't happen.
pfrenssen’s picture

pfrenssen
Location Sofia
commented

This seems to be similar to #3049250: PHP 7.3 Session Handling Issue, I checked the place in the serialized data where the error is occurring and it is a Symfony\Component\HttpFoundation\ParameterBag which has been incorrectly serialized.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new 3146507-4.patch1.18 KB

This was not so easy to track down, but it seems indeed to be caused by serializing the Symfony request. Excluding the request from being serialized seems to solve the issue. I'm not sure if the request is ever used after these objects are deserialized again, but the tests are passing and in manual testing all seemed to be fine. It would probably be a good idea though to try to restore the request on wakeup.

DeFr’s picture

DeFr commented

Manually queued a test, but yes I think they're going to pass.

Was the url processor hunk really needed ? I think a cleaner fix would be to add those to Drupal\facets\Entity\Facet ; the facet source is getting serialized because that entity has a facet_source_instance cache ; it can be set to NULL on __sleep and will be restored if needed. If the url processor hunk was needed, I guess we'll need to do that to $processors too.

DeFr’s picture

DeFr commented

(By the way, thanks a lot for picking this up, I dropped the ball on this, sorry about that !)

pfrenssen’s picture

pfrenssen
Location Sofia
commented

I'm not sure if the UrlProcessor change is strictly needed to fix the tests, but I had to drop the request in both to get rid of the errors when using AJAX in the facets form.

DeFr’s picture

DeFr commented
StatusFileSize
new 3146507-9-facet-serialization.patch534 bytes

Attaching a patch that should fix the root cause. Fixes the form for me, haven't run the test yet.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Reviewed & tested by the community

Great, this is the right solution!

  • borisson_ committed 7e7ad17 on 8.x-1.x authored by pfrenssen 
    Issue #3146507 by DeFr, pfrenssen: Tests are failing on PHP 7.3
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Reviewed & tested by the community » Fixed

Thanks so much!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.