The fancy solution:

• Run some static analysis on classes as part of CI which would:
  • detect when the signature of a cached class changes
  • enforce that some component of the cache key must be altered every time the signature is changed.

Imho unserialize() does the right thing. The class property get stored with a visibility and restored exactly the same way. The real trouble is that it should bails out when creating a property having the same name with a different visibility than the class contract.

I would expect unserialize() to return false and emit a warning about class mismatch or something like that.

Well, when you declare variable as "private" it means it - that means, outside of the class it's like it does not exist. Which means when you define property outside of the class, it's completely new one, not the same one. Check out: https://3v4l.org/GLP67

So moving variable to private is essentially the same as renaming it from $foo to $myclass_foo. unserialize() can't do much about it. At least not without generic "class change framework". Java has serialization IDs in classes for this, but PHP doesn't have anything so far.

Some thoughts:

• If a field is removed, that should be ok.
• If a field is added, that could be ok (e.g. a field that's lazy-loaded) or could be problematic (if it's not).
• As already noted, a field with changed access is the same as a field being renamed. And a rename is equivalent to "field is removed" + "field is added".
• Even if the field itself isn't changed, if the type of data stored in the field is changed (e.g. it changes from string to string[], or from a normal array to an associative array with the same values) that could still be a problem.
• A completely generic solution (i.e. BagOStuff does all the work) would need to apply not only to objects directly serialized, but also to objects that are inside an array, or objects included by composition inside another object, etc. That seems like it would be hard to do without rewriting serialize() and unserialize().
• A solution where each class is responsible for checking its own potential unserialization might use __wakeup() to check things (possibly just a version number check like Java's serialization IDs) and throw something on failure that BagOStuff would catch.

Something crazy that seems to actually work in quick testing (PHP 5.6.26, 7.0.15, and HHVM 3.12.11):

class SerializationIdCheckException extends RuntimeException {
    public function __construct( $class, $expected, $found ) {
        parent::__construct( "Serialization ID check failed in $class, $found !== $expected" );
    }
}

trait SerializationIdCheck {
    private $serializationIdCheck = self::SERIALIZATION_ID;

    public function __wakeup() {
        if ( $this->serializationIdCheck !== self::SERIALIZATION_ID ) {
            throw new SerializationIdCheckException( __CLASS__, self::SERIALIZATION_ID, $this->serializationIdCheck );
        }
        if ( is_callable( 'parent::__wakeup' ) ) {
            parent::__wakeup();
        }
    }
}

class Foo {
    use SerializationIdCheck;
    const SERIALIZATION_ID = "foo.1";
}

class Bar extends Foo {
    use SerializationIdCheck;
    const SERIALIZATION_ID = "bar.1";
}

$serialized = serialize( new Bar );

foreach ( [ 'none' => null, 'foo.1' => 'foo.0', 'bar.1' => 'bar.0' ] as $repl => $to ) {
    $test = $serialized;
    if ( $to !== null ) {
        // Fake loading from a different version by changing the ID in the serialized string
        $test = str_replace( serialize( $repl ), serialize( $to ), $test );
    }
    echo "Unserializing $test: ";
    try {
        unserialize( $test );
        echo "No exception thrown.\n";
    } catch ( SerializationIdCheckException $ex ) {
        echo "Caught exception: " . $ex->getMessage() . "\n";
    }
}

Looks reasonable. May slow down serialization a bit so I'd suggest a performance test but otherwise fine. One small tweak would be to add isset($this->serializationIdCheck) check to condition to weed out old versions having no check.

Of course, this would become trickier if the class in question already has its own __wakeup :) We don't have too many of them but Status and Title do have them. Then probably we could do something like:

class Title {
  use SerializationIdCheck { SerializationIdCheck::__wakeup as checkWakeup; }
  public function __wakeup() {
    $this->checkWakeup();
    // ...

  }
}

The isset() isn't useful, because if we're unserializing a version of the class that doesn't include the serializationIdCheck field then it's going to keep its default value. To catch that situation, we'd have to set $this->serializationIdCheck in the constructor instead of using a static initializer.

trait SerializationIdCheck {
    private $serializationIdCheck = 'unset';

    private function initializeSerializationIdCheck() {
        $this->serializationIdCheck = self::SERIALIZATION_ID;
    }

    /* same __wakeup method */
}

class Foo {
    use SerializationIdCheck;
    const SERIALIZATION_ID = "foo.1";

    public function __construct() {
        // Call parent::__construct() and whatever else needs doing
        $this->initializeSerializationIdCheck();
    }
}

I don't think there's any way to get initializeSerializationIdCheck() called automatically.

Change 335101 had a related patch set uploaded (by MaxSem):
[WIP][POC] Automatic serialization guard trait

https://gerrit.wikimedia.org/r/335101

As for things that already have their own __wakeup, we might want to be careful that parent::__wakeup() doesn't get called twice and we'd probably also want to avoid parent::__wakeup() getting magically called by the trait instead of it being done where people can see it...

My above patch does the same thing as Brad's proposal, however it tracks the serialization format automatically, as opposed to manual maintenance.

Not sure why you need "unset" - if it's unset in the data and we expect it to be set, then it's broken.

The "unset" is there so that the exception message (if it isn't caught) is a bit clearer: "Serialization ID check failed in Class, unset !== 1234" rather than "Serialization ID check failed in Class, !== 1234".

Change 335101 abandoned by MaxSem:
[WIP][POC] Automatic serialization guard trait

Reason:
Point was demonstrated, those interested can pick up from that.

https://gerrit.wikimedia.org/r/335101

Not a sub task per T203566#4564404. T203566 isn't required to settle for this task and vice-versa. This task is adding detection for category/source of problems. T203566 is an instance of such problem.

T231542 is a similar case with some AbuseFilter class stored in external storage. Changing the visibility of a property from public to protected cause the stored objects to no more be unserialized properly :-\