In #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize() we're replacing some (soft) deprecated magic methods.

Instead of updating / replacing Query::__wakeup() and ::__sleep() we should deprecate and remove them.

public function __wakeup(): void {
  $this->connection = Database::getConnection($this->connectionTarget, $this->connectionKey);
}

This reattaches an unserialized query to whatever connection currently exists, and there's no guarantee that's going to work / make sense.

It's inconsistent with Connection, which already refuses serialization (see \Drupal\Core\Database\Connection::__sleep), and with PDO, which does not support it.

This would effectively finish off #2463321: Serializing the database connection is dangerous and error-prone, make it unserializable again.

Issue fork drupal-3625895

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mcdruid created an issue. See original summary.

mcdruid’s picture

mcdruid commented
Related issues: +#3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize()

mcdruid opened merge request !17239

mcdruid opened merge request !17240

mcdruid’s picture

mcdruid commented
Status: Active » Needs review

If these land, we should update #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize().

The test suite seems to confirm that nothing in core does the roundtrip except for the one test that specifically does only that.

mcdruid’s picture

mcdruid commented

Just noting that I should have named the branches more carefully:

  • 3625895-11.x-deprecation hopefully speaks for itself
  • 3625895-deprecate-and-remove is the actual removal (hopefully for 12.x)
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. Serializing a query object makes no sense and as the deprecation/removal proves nothing is using this in core anyway.

jesus_md’s picture

jesus_md commented

Reviewed https://git.drupalcode.org/project/drupal/-/merge_requests/17239 on 11.4.5 with PHP 8.5.5, and the deprecation looks right to me

  • both methods are tagged @deprecated in drupal:11.5.0 and is removed from drupal:12.0.0 and trigger E_USER_DEPRECATED pointing to https://www.drupal.org/node/3625908
  • behaviour doesn't change, I serialized and unserialized a Select and an Insert with drush and the select still ran and returned results, both notices were raised
  • SerializeQueryTest passes with the deprecations expected (1 assertion before the patch, 3 after)
ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Question: do magic methods need the deprecation dance? You shouldn't call anything with an underscore anyways and doubleplus shouldn't call a magic method ever.

This shouldn't hold up sending this in, if it's controversial a followup could do the removal already in 11 if such consensus can be reached but I don't think it should be controversial.

I would love if I didn't need to deal with query serializing.

mcdruid’s picture

mcdruid commented

I think it makes sense to do the deprecation in case anyone is relying on this in contrib or custom code.

I won't miss this when it's gone though :)

mondrake’s picture

mondrake
Location 🇮🇹
commented

I'm afraid the 11.5+12 window is over, drats. Isn't this 12.1+13 material now.

catch’s picture

catch
he/him
Primary language English
commented

I think as security hardening we should get this in during the beta, we're very likely to release a beta2 anyway.

  • catch committed c305d653 on main 
    task: #3625895 Deprecate and remove Query::__wakeup() and ::__sleep()...

  • catch committed 5dbc49ae on 12.0.x 
    task: #3625895 Deprecate and remove Query::__wakeup() and ::__sleep()...

  • catch committed a7730e53 on 11.5.x 
    task: #3625895 Deprecate and remove Query::__wakeup() and ::__sleep()...

  • catch committed 3f945c87 on 11.x 
    task: #3625895 Deprecate and remove Query::__wakeup() and ::__sleep()...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.5.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Security improvements

On #9 I would be more tempted to try to do the hard break if it was three months ago at the beginning of the 11.5.x cycle, this close to beta (technically after the window closed) I think people might be more reasonably annoyed if we break something, and also it would require more thought and messaging etc. so committed/pushed to main/12.0.x/11.x/11.5.x with the deprecation version in 11.x

I do think we should do the hard break in 12.0.0 so that this is gone as soon as possible.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Thanks for the thoughtful release managing.