Skip to content

PHPC-2248: Remove Serializable implementations #1663

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

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Sep 18, 2024

PHPC-2248

This will conflict with other pull request, but I'll rebase before merging and resolve conflicts.

@alcaeus alcaeus requested a review from jmikola September 18, 2024 12:42
@alcaeus alcaeus force-pushed the phpc-2248-remove-serializable branch from 0e933fb to 52b93ed Compare September 18, 2024 12:52
UPGRADE-2.0.md Outdated
@@ -4,3 +4,6 @@ UPGRADE FROM 1.x to 2.0
* The `getServer()` method has been removed from the CommandFailedEvent,
CommandStartedEvent, and CommandSucceededEvent event classes. The `getHost()`
and `getPort()` methods have been added in its place.
* All classes that previously implemented the `Serializable` interface no
longer implement this. This changes the serialization format, but the
ability to serialize and unserialize these classes remains as-is.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to say "this changes the serialization format" as the Serializable interface hasn't been used in some time.

In PHP 7.4+, the __serialize() and __unserialize() magic methods are preferred when available (see: docs). We also noted this in PHPC-2230 when dropping support for PHP 7.2 and 7.3 in 1.17.0.


Presumably, the only loss in functionality here is that users would no longer be able to unserialize a string in the legacy "C format" since that would depend on Serializable::unserialize(). All of the tests for handling that format were removed in 2ab1e4b.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I removed the second sentence and only highlight that the interface is no longer implemented.

@alcaeus alcaeus force-pushed the phpc-2248-remove-serializable branch from 52b93ed to 286b4e9 Compare September 19, 2024 06:25
@alcaeus alcaeus requested a review from jmikola September 19, 2024 06:25
@alcaeus alcaeus force-pushed the phpc-2248-remove-serializable branch 2 times, most recently from e8bea11 to 79bbf96 Compare September 19, 2024 07:03
@alcaeus alcaeus force-pushed the phpc-2248-remove-serializable branch from 79bbf96 to 1728b3f Compare September 19, 2024 11:25
@alcaeus alcaeus enabled auto-merge (squash) September 19, 2024 11:25
@jmikola jmikola disabled auto-merge September 19, 2024 16:19
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion but LGTM after that. I've disabled auto-merge so we can address that first.

@@ -6,15 +6,15 @@ MongoDB\Driver\ServerApi unserialization errors (__serialize and __unserialize)
require_once __DIR__ . '/../utils/basic.inc';

echo throws(function() {
unserialize('C:24:"MongoDB\Driver\ServerApi":24:{a:1:{s:7:"version";i:0;}}');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was wondering if we had any examples that still relied on Serializable::unserialize(). Missed this earlier, but it's what I had in mind when writing about the possible loss in functionality in #1663 (comment).

@jmikola jmikola merged commit 46c6055 into mongodb:v2.x Sep 19, 2024
62 checks passed
@alcaeus alcaeus deleted the phpc-2248-remove-serializable branch September 20, 2024 06:24
alcaeus added a commit that referenced this pull request Oct 29, 2024
* v2.x: (22 commits)
  PHPC-2441: Remove deprecated Manager constructor options (#1719)
  PHPC-990: Strict type validation for boolean URI options (#1713)
  PHPC-2440: Remove deprecated Query constructor options (#1707)
  PHPC-2459: Remove support for float arg in UTCDateTime ctor (#1709)
  Remove obsolete test
  PHPC-2344 Remove SSLConnectionException (#1696)
  PHPC-2144 Throw a LogicException when getting info from unacknowledged write result (#1687)
  PHPC-2454: Remove --enable-system-ciphers configure option (#1681)
  PHPC-2348 Remove `WriteException` and move `getWriteResult` to `BulkWriteException` (#1685)
  PHPC-2417 Add UTCDateTimeInterface::toDateTimeImmutable() (#1684)
  PHPC-2309: Remove --with-openssl-dir configure option (#1676)
  PHPC-2444: Remove support for string arguments in UTCDateTime constructor (#1662)
  PHPC-2248: Remove Serializable implementations (#1663)
  Update version for 2.x branch (#1672)
  PHPC-1021: Remove support for ReadPreference integer modes (#1666)
  PHPC-2342: Remove --with-libbson and --with-libmongoc configure options (#1667)
  PHPC-2351: Remove CursorId class (#1664)
  PHPC-2140: Make tentative return types definitive (#1658)
  PHPC-2402: Remove range_preview constants (#1665)
  PHPC-2346: Remove deprecated BSON functions (#1653)
  ...
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

Successfully merging this pull request may close these issues.

2 participants