Page MenuHomePhabricator

Handle deprecation of Serializable interface in Wikibase
Closed, ResolvedPublic

Description

Wikibase  (30 usages found)
    lib  (30 usages found)
        packages  (30 usages found)
            wikibase  (30 usages found)
                changes  (8 usages found)
                    src  (8 usages found)
                        EntityDiffChangedAspects.php  (4 usages found)
                            6 use Serializable;
                            18 class EntityDiffChangedAspects implements Serializable {
                            139 * @see Serializable::serialize
                            148 * @see Serializable::unserialize
                        RepoRevisionIdentifier.php  (4 usages found)
                            7 use Serializable;
                            17 class RepoRevisionIdentifier implements Serializable {
                            79 * @see Serializable::serialize
                            88 * @see Serializable::unserialize
                data-model  (22 usages found)
                    src  (20 usages found)
                        Entity  (8 usages found)
                            EntityId.php  (2 usages found)
                                5 use Serializable;
                                10 interface EntityId extends Serializable {
                            EntityIdValue.php  (2 usages found)
                                27 * @see Serializable::serialize
                                40 * @see Serializable::unserialize
                            ItemId.php  (2 usages found)
                                63 * @see Serializable::serialize
                                74 * @see Serializable::unserialize
                            NumericPropertyId.php  (2 usages found)
                                59 * @see Serializable::serialize
                                68 * @see Serializable::unserialize
                        Snak  (8 usages found)
                            PropertyValueSnak.php  (2 usages found)
                                50 * @see Serializable::serialize
                                61 * @see Serializable::unserialize
                            Snak.php  (2 usages found)
                                5 use Serializable;
                                17 interface Snak extends Serializable, PropertyIdProvider {
                            SnakList.php  (2 usages found)
                                268 * @see Serializable::serialize
                                277 * @see Serializable::unserialize
                            SnakObject.php  (2 usages found)
                                96 * @see Serializable::serialize
                                107 * @see Serializable::unserialize
                        ReferenceList.php  (4 usages found)
                            9 use Serializable;
                            27 class ReferenceList implements Countable, IteratorAggregate, Serializable {
                            225 * @see Serializable::serialize
                            256 * @see Serializable::unserialize
                    tests  (2 usages found)
                        fixtures  (2 usages found)
                            CustomEntityId.php  (2 usages found)
                                16 * @see Serializable::serialize
                                25 * @see Serializable::unserialize

Details

SubjectRepoBranchLines +/-
mediawiki/extensions/WikibaseQualityConstraintsREL1_39+6 -0
mediawiki/extensions/WikibaseREL1_39+8 -0
mediawiki/extensions/WikibaseREL1_39+21 -0
mediawiki/extensions/WikibaseREL1_39+21 -13
mediawiki/extensions/WikibaseREL1_39+146 -10
mediawiki/extensions/Wikibasemaster+21 -0
mediawiki/extensions/Wikibasemaster+8 -8
mediawiki/extensions/Wikibasemaster+2 -2
mediawiki/extensions/Wikibasemaster+15 -40
mediawiki/extensions/Wikibasemaster+8 -0
mediawiki/vendormaster+426 -213
mediawiki/extensions/Wikibasemaster+49 -16
mediawiki/extensions/WikibaseQualityConstraintsmaster+1 -1
mediawiki/extensions/Wikibasemaster+25 -6
mediawiki/extensions/Wikibasemaster+5 -1
mediawiki/extensions/WikibaseMediaInfomaster+2 -2
mediawiki/extensions/WikibaseQualityConstraintsmaster+2 -2
mediawiki/extensions/Wikibasemaster+146 -10
mediawiki/extensions/Wikibasemaster+30 -18
Show related patches Customize query in gerrit

Related Objects

Event Timeline

I’d like to think we don’t rely on serialize() of Wikibase entity IDs anywhere, but that’s probably a fool’s hope. (E.g., I believe the caching layer uses PHP serialization, and we probably put entity ID objects into and out of caches somewhere, even if strings would be cleaner.)

The good news that this should be reasonably straightforward, the serialization is just a string and the deserialization just needs to reconstruct a small bit of internal state afterwards. (I believe this is true for all the entity ID types we have, so including T301248 and T301223.)

…And I should learn to read the full task description, this one isn’t only about entity IDs. 🤦

But from a quick look through them, it seems like the other affected classes also don’t have super complicated serialization methods. In fact the new magic method interface should be more easy to implement, since we can directly return arrays in some cases instead of wrapping them in JSON.

…And I should learn to read the full task description, this one isn’t only about entity IDs. 🤦

But from a quick look through them, it seems like the other affected classes also don’t have super complicated serialization methods. In fact the new magic method interface should be more easy to implement, since we can directly return arrays in some cases instead of wrapping them in JSON.

Sounds pretty much like what I did for the couple of patches to numerous classes in MW Core.

Make the new __ methods handle the array version, keep the old non __ prefixed versions with any special handling (turning to strings, or whatever else) and make them call the __ versions

https://gerrit.wikimedia.org/r/c/756964
https://gerrit.wikimedia.org/r/c/758800

And if the tests are about right, you should know whether your changes are correct ;D

And if the tests are about right, you should know whether your changes are correct ;D

And thankfully we already run php74 in gate-and-submit, so at least one job should cover the new methods 😌

Change 762094 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/Wikibase@master] Implement __serialize/__unserialize for PHP 8.1 support

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

Change 762094 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Implement __serialize/__unserialize for PHP 8.1 support

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

Change 773521 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/Wikibase@master] DataModel\Entity: Add __serialize/__unserialize for EntityIds

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

I think the Diff package, used by Wikibase, could also be included in this task; somebody already uploaded a pull request for it: https://github.com/wmde/Diff/pull/128

(Edit: See also the more narrowly scoped PR https://github.com/wmde/Diff/pull/129)

As far as I can see, all remaining issues can only be addressed once T305785 is fixed.

Change 773521 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] DataModel\Entity: Add __serialize/__unserialize for EntityIds

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

Moving back to Doing, AFAICT there’s nothing to review here at the moment. I think the remaining open part is the DataValues library.

Change 842427 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/Wikibase@master] DM: Add abstract __(un)serialize to SerializableEntityId

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

Change 842428 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/Wikibase@master] DM: Add __(un)serialize to Snak (and implementations)

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

Change 842434 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/WikibaseMediaInfo@master] MediaInfoId: Add (return) type hints to __(un)serialize

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

Change 842436 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/WikibaseQualityConstraints@master] Use MockBuilder::getMockForAbstractClass for SerializableEntityId

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

Change 842436 merged by jenkins-bot:

[mediawiki/extensions/WikibaseQualityConstraints@master] Use MockBuilder::getMockForAbstractClass for SerializableEntityId

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

Change 842434 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@master] MediaInfoId: Add (return) type hints to __(un)serialize

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

Change 842427 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] DM: Add abstract __(un)serialize to SerializableEntityId

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

Change 842428 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] DM: Add __(un)serialize to Snak (and implementations)

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

Change 845589 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Declare compatibility with data-values/number ^0.12.0

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

Change 845590 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseQualityConstraints@master] Declare compatibility with data-values/number ^0.12.0

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

Change 848265 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] WIP: Drop compatibility with older data-values versions

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

Change 848346 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/vendor@master] Update data-values/*

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

Change 845589 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Update data-values versions in composer.json

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

Change 845590 merged by jenkins-bot:

[mediawiki/extensions/WikibaseQualityConstraints@master] Declare compatibility with data-values/number ^0.12.3

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

Change 848346 merged by jenkins-bot:

[mediawiki/vendor@master] Update data-values/*

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

Change 850189 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] FederatedPropertyId: Add __serialize()/__unserialize()

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

Change 850189 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] FederatedPropertyId: Add __serialize()/__unserialize()

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

Change 848265 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Drop compatibility with older data-values versions

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

Moving back to doing; this PHP 8.1 CI log has two Serializable-related deprecation warnings left:

24) Wikibase\Repo\Tests\Normalization\CommonsMediaValueNormalizerTest::testNormalize_badType
Mock_DataValue_5fa3bfea implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary)

Deprecated: Mock_EntityId_c95bc3e8 implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary) in /workspace/src/vendor/phpunit/phpunit/src/Framework/MockObject/MockClass.php(51) : eval()'d code on line 3

That second one isn’t reported as belonging to any particular test case, so I guess we just need to grep for calls like $this->createMock( EntityId::class ). Shouldn’t require any more library updates, though – I think all the DataValues stuff is done at last.

Correction: there might be more Serializable-related warnings, because the above job failed before it ran the DB tests. (But it at least finished running the non-DB tests and reported all the errors in there, instead of crashing partway through as it used to.)

git grep 'createMock.*EntityId:' finds 16 occurrences. I think a convenient way to make all of them work would be to add __serialize() / __unserialize() methods to the EntityId interface, so that PHPUnit will mock them automatically. (This would be a breaking change, but the data model RELEASE-NOTES.md already list the in-progress version as a major version update (10.0.0) with breaking changes.) I wonder if this would be acceptable?

Change 851651 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Use BooleanValue in CommonsMediaValueNormalizerTest

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

Change 851651 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use BooleanValue in CommonsMediaValueNormalizerTest

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

Change 852162 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Wikibase@master] Make a few tests more compatible with PHP 8.1

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

Change 852162 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Make a few tests more compatible with PHP 8.1

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

Change 853286 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Add __serialize() and __unserialize() to EntityId

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

git grep 'createMock.*EntityId:' finds 16 occurrences. I think a convenient way to make all of them work would be to add __serialize() / __unserialize() methods to the EntityId interface, so that PHPUnit will mock them automatically. (This would be a breaking change, but the data model RELEASE-NOTES.md already list the in-progress version as a major version update (10.0.0) with breaking changes.) I wonder if this would be acceptable?

Did that in the above change, since I haven’t heard any complaints about it yet. Let’s see if CI is happy with it.

Change 853286 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Add __serialize() and __unserialize() to EntityId

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

PHP 8.1 is voting in gate-and-submit now, so I think we can be confident that this is done.

Change 880548 had a related patch set uploaded (by Addshore; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_39] FederatedPropertyId: Add __serialize()/__unserialize()

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

Change 880549 had a related patch set uploaded (by Addshore; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_39] Add __serialize() and __unserialize() to EntityId

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

Change 880550 had a related patch set uploaded (by Addshore; author: Hoo man):

[mediawiki/extensions/Wikibase@REL1_39] DataModel\Entity: Add __serialize/__unserialize for EntityIds

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

Change 880551 had a related patch set uploaded (by Addshore; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_39] Drop compatibility with older data-values versions

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

Change 880923 had a related patch set uploaded (by Addshore; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseQualityConstraints@REL1_39] Declare compatibility with data-values/number ^0.12.3

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

Change 880550 abandoned by Addshore:

[mediawiki/extensions/Wikibase@REL1_39] DataModel\Entity: Add __serialize/__unserialize for EntityIds

Reason:

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

Change 880551 abandoned by Addshore:

[mediawiki/extensions/Wikibase@REL1_39] Drop compatibility with older data-values versions

Reason:

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

Change 880549 abandoned by Addshore:

[mediawiki/extensions/Wikibase@REL1_39] Add __serialize() and __unserialize() to EntityId

Reason:

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

Change 880548 abandoned by Addshore:

[mediawiki/extensions/Wikibase@REL1_39] FederatedPropertyId: Add __serialize()/__unserialize()

Reason:

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

Change 880923 abandoned by Addshore:

[mediawiki/extensions/WikibaseQualityConstraints@REL1_39] Declare compatibility with data-values/number ^0.12.3

Reason:

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