Problem/Motivation

This has a @todo to remove once 8.6.x is no longer supported, which is very much the case with 9.1.x.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

jungle’s picture

Status: Active » Needs review
FileSize
3.2 KB
longwave’s picture

  1. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -196,51 +196,6 @@ protected function handleAccess(Request $request) {
       public function loadLegacyIncludes() {
         parent::loadLegacyIncludes();
    

    This now only calls the parent, so the entire method can be removed?

  2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -196,51 +196,6 @@ protected function handleAccess(Request $request) {
    -  public static function fixSerializedExtensionObjects(ContainerInterface $container) {
    

    This was public static, it seems unlikely anyone would call it - but in theory they could. Do we have to go through the deprecation process?

    One call in contrib suggests we might: http://grep.xnddx.ru/search?text=fixSerializedExtensionObjects&filename=

jungle’s picture

Thanks @longwave!

  1. Re #3.1 Good point, done
  2. -   * @internal
    -   *   This function is only to be called by the Drupal core update process.
    -   *   Additionally, this function will be removed in minor release of Drupal.
    ...
    -  public static function fixSerializedExtensionObjects(ContainerInterface $container) {
    

    Re #3.2, It's an internal method, not sure if a BC layer is necessary.

  3. Should #3031322: Remove \Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects() once 8.6.x is not supported be closed? no patch but filed earlier than this.
jungle’s picture

-use Symfony\Component\DependencyInjection\ContainerInterface;

Removing unused use statement.

catch’s picture

Issue tags: +Needs change record

http://grep.xnddx.ru/search?text=fixSerializedExtensionObjects&filename= is test-only, and explicitly references it's a workaround because the method hasn't been removed. That seems quite unique so I think we can go ahead and remove in a minor, but we should file an issue against lightning_media to be helpful.

jungle’s picture

Issue filed to lightning_media [#3130552[ and CR created

longwave’s picture

Status: Needs review » Reviewed & tested by the community

#5 looks good.

hchonov’s picture

This has a @todo to remove once 8.6.x is no longer supported, which is very much the case with 9.1.x.

I would welcome such decision, however I have to ask whether this is documented officially? Do we already remove updates introduced in branches we don't support with the new version?

catch’s picture

All updates prior to Drupal 8.8 were removed in 9.0.x, change record is here: https://www.drupal.org/node/3098327 and links to various issues. Short version is that sites have to update to 8.8 or 8.9 prior to going to 9.0.

There is an open issue to discuss the cut-off point for Drupal 10 #2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later).

At one point we were also discussing removing very old updates (< 8.6 era) from 8.9.x, but then realised it would be a lot more work than it saved after having done this for 9.0.x and run into various issues (like spotty hook_update_last_removed() support).

  • xjm committed 2a38906 on 9.1.x
    Issue #3130341 by jungle, catch, longwave: Remove Drupal\Core\Update\...
xjm’s picture

Title: Remove Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects() » [backport] Remove Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects()
Version: 9.1.x-dev » 9.0.x-dev

This was added for #3031128: Update from 8.6.7 to 8.6.8 warnings - Drupal\Core\Extension\Extension has no unserializer, which in turn was a fix for the update path for #2701829: Extension objects should not implement \Serializable, and both are indeed no longer part of the supported upgrade path. You have to update to 8.8.x in order to update to D9, so the functionality is no longer needed.

I confirmed that the only other non-test caller for this in 8.8.x was update_fix_compatibility(), which has also been killed with fire. And that use in Lightning Media 8.x-3.x (the supported branch for 8.8.x) is only in a test anyway. I pinged the Lightning maintainers a heads-up as well.

Probably we should have deprecated this properly, but nonetheless, I think we should go ahead with this change in 9.1.x and 9.0.x as well.

Committed to 9.1.x and fixed up the CR a little. Leaving RTBC against 9.0.x to discuss the backport.

jungle’s picture

# File in 8.9.x and 9.x
core/modules/system/tests/src/Functional/UpdateSystem/BrokenCacheUpdateTest.php:
# File in 8.8.x
core/modules/system/tests/src/Functional/Update/BrokenCacheUpdateTest.php

Rerolled a patch for 8.8.x, in case it's needed. The patch in #5 should apply to 8.9.x but does not apply to 8.8.x because of the difference of file path.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3130341-8.8.x-13.patch, failed testing. View results

hchonov’s picture

The patch to be committed should be the last one on the issue. When providing re-rolls for earlier releases please also provide the patch for the target of the issue, which is now 9.0.x.

jungle’s picture

Re #15, the patch in #5 was valid for 9.1.x and is still vaild for 9.0.x, that's why no 9.0.x patch uploaded explicitly.

longwave’s picture

Status: Needs work » Reviewed & tested by the community

#5 is still OK for 9.0.x - it is dead code that does nothing now so I think we are OK to remove it there.

This cannot go back to 8.9.x or 8.8.x as update_fix_compatibility() still need it, as the test fails show - we can't remove any of this for BC reasons in D8.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3130341-8.8.x-13.patch, failed testing. View results

jungle’s picture

Status: Needs work » Reviewed & tested by the community

@longwave thank you!

This cannot go back to 8.9.x or 8.8.x

Setting back to RTBC. Failure of 8.8.x patch is irrelevant.

xjm’s picture

Right, the backport under discussion is 9.0.x only. Thanks!

longwave’s picture

Reuploading #5 for clarity, the RTBC auto test is looking at #13 which means it will fail in 2 days when it gets retested again.

xjm’s picture

Issue tags: +rc deadline

If we do backport this to 9.0.x, we'll want to do so before RC.

  • catch committed 7e93a68 on 9.0.x
    Issue #3130341 by jungle, longwave, catch, xjm: [backport] Remove Drupal...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Agreed with backporting this, zero disruption and one less thing in our most-critical-bug-prone system.

Committed 7e93a68 and pushed to 9.0.x. Thanks!

xjm’s picture

Title: [backport] Remove Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects() » Remove Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects()

Status: Fixed » Closed (fixed)

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