As discussed in #2642814: Add tests for servers to EntitySerializationTest, we want to make sure query objects can be safely serialized – and maybe other objects, too, TBD.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

Issue tags: +Release blocker
drunken monkey’s picture

And here they are: now additionally tests query and item serialization, as well as query and field cloning. (I grouped them in the same test class, since cloning is somewhat similar to serialization, and because I re-use some code for which it would be a bit awkward to create a complete trait – but I can do that, too, if we agree it makes more sense.)

And, lo and behold, this even uncovered some bugs:

  • Caching for parse mode plugins was a stupid idea, didn't think this through.
  • For some reason, field serialization skipped the values. Didn't really discuss this in the issue where it was introduced, so no idea what the reasoning was. (For some reason, the test doesn't catch that one, though – no idea why. It did work at one point, but not anymore.)
  • When setting the parse mode on the query, I think it makes sense to automatically re-parse the keys (if they were set before). Not really related, though, admittedly, and is also debatable. I bow to the group's judgement there.

Once again proves how important proper testing is. Damn.

The last submitted patch, 3: 2682615-3--test_serialization_and_cloning--tests_only.patch, failed testing.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Great work! Hurray for tests!

  • drunken monkey committed cf740ca on 8.x-1.x
    Issue #2682615 by drunken monkey: Added tests for object serialization...
drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed.

Status: Fixed » Closed (fixed)

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