Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

FileSize
11.29 KB

This removes all BC layers from core/modules/serialization as of July 11, 2017, commit e1e216138f5311a5dd6b96b5789ab2db9ac5fd5f.

 .../config/install/serialization.settings.yml      | 11 -----
 .../config/schema/serialization.schema.yml         | 10 ----
 core/modules/serialization/serialization.install   | 57 ----------------------
 .../serialization/serialization.services.yml       |  9 +---
 .../src/EventSubscriber/BcConfigSubscriber.php     | 55 ---------------------
 .../RegisterSerializationClassesCompilerPass.php   | 18 -------
 .../tests/src/Kernel/EntitySerializationTest.php   | 13 +++--
 7 files changed, 10 insertions(+), 163 deletions(-)
Wim Leers’s picture

Status: Active » Postponed
Wim Leers’s picture

FileSize
13.45 KB

1.5 year later, time for an update. Commit c8dc57eedea3fd34de067098ca2ee36e6c459245.

 core/modules/serialization/config/install/serialization.settings.yml        | 11 -----------
 core/modules/serialization/config/schema/serialization.schema.yml           | 10 ----------
 core/modules/serialization/serialization.install                            | 58 ----------------------------------------------------------
 core/modules/serialization/serialization.services.yml                       | 11 +++--------
 core/modules/serialization/src/EventSubscriber/BcConfigSubscriber.php       | 55 -------------------------------------------------------
 core/modules/serialization/src/Normalizer/DateTimeIso8601Normalizer.php     | 15 +--------------
 core/modules/serialization/src/RegisterSerializationClassesCompilerPass.php | 18 ------------------
 core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php     | 12 ++++++++----
 8 files changed, 12 insertions(+), 178 deletions(-)
Wim Leers’s picture

Issue tags: +API-First Initiative

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.

Wim Leers’s picture

Issue tags: +Deprecation Removal
Wim Leers’s picture

Title: Remove serialization.module BC layers » [PP-1] Remove serialization.module BC layers
kim.pepper’s picture

This is issue is postponed on #2893804: Remove rest.module BC layers that one is postponed on this?!

There seems to be a circular dependency here! 🙃

longwave’s picture

Wim Leers’s picture

#11: #10 noticed the super outdated and super wrong issue summary at #2893804: Remove rest.module BC layers 😅 Fixed that!

Wim Leers’s picture

Reroll now that #2893804: Remove rest.module BC layers is getting close.

Stefdewa’s picture

Status: Postponed » Needs review
FileSize
12.9 KB
20.36 KB

Rerolled patch. Added approximate interdiff (not all changes from #13 would apply.
Setting 'Needs review' but should after review be put back to 'Postponed' because tests will fail until #2893804: Remove rest.module BC layers is merged.

Wim Leers’s picture

Status: Needs review » Postponed

Obviously you found quite a few things that still needed to be removed, wonderful!

+++ b/core/modules/serialization/serialization.install
@@ -6,33 +6,15 @@
+ * The serialization.settings configuration is irrelevant in Drupal 9.

Let's use the same wording as rest_update_9001() in the latest patch iteration (comment 91).


 13 files changed, 13 insertions(+), 315 deletions(-)

Marking postponed for now, but as soon as #2893804 lands, this should be re-tested and marked RTBC (and that one nit should be fixed) :) 🚢🚢

Wim Leers’s picture

Title: [PP-1] Remove serialization.module BC layers » Remove serialization.module BC layers
Status: Postponed » Needs review

#2893804: Remove rest.module BC layers landed 5 mins ago! Testing #14

Status: Needs review » Needs work

The last submitted patch, 14: 2893795-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

+++ b/core/modules/serialization/serialization.install
@@ -6,33 +6,15 @@
 /**
- * Implements hook_update_last_removed().
+ * The serialization.settings configuration is irrelevant in Drupal 9.
  */
-function serialization_update_last_removed() {
-  return 8401;
+function serialization_update_9001() {
+  \Drupal::configFactory()->getEditable('serialization.settings')->delete();

I guess we also need an update path test for that, just like rest?

Wim Leers’s picture

Issue tags: +Needs reroll

Just some simple updates needed and then this will be RTBC!

DamienMcKenna’s picture

Tagging as a requirement for Drupal 9.0-beta1.

catch’s picture

  1. diff --git a/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
    index 6f04ba3c68..054ce7c8ae 100644
    
    index 6f04ba3c68..054ce7c8ae 100644
    --- a/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
    
    --- a/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
    +++ b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
    
    +++ b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
    @@ -12,25 +12,15 @@
    

    Is this trait still needed?

  2. +++ b/core/modules/serialization/serialization.install
    @@ -6,33 +6,15 @@
    -  return 8401;
    +function serialization_update_9001() {
    +  \Drupal::configFactory()->getEditable('serialization.settings')->delete();
     }
    

    As with the REST issue, this should be a post update.

Wim Leers’s picture

#21:

  1. No, but @Berdir argued in #2893804 to keep it to minimize disruption for contrib. See the rationale for this approach/consensus amongst the two of us in #2893804-73: Remove rest.module BC layers.
  2. Yep, that should definitely happen here too :)
sokru’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.66 KB

Reroll from #14. Moved hook_update_N to post_update to resolve #21.2

Wim Leers’s picture

@sokru Thanks! Could you also post the corresponding interdiff? 🙏🙂

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/core/modules/serialization/serialization.post_update.php
    @@ -0,0 +1,13 @@
    + * The serialization.settings configuration is irrelevant in Drupal 9.
    

    This comments needs to be updated as described in #15.

  2. And this still needs the update path that @Berdir pointed out in #18, just like \Drupal\Tests\rest\Functional\Update\RestSettingsDeletionUpdateTest.
sokru’s picture

Status: Needs work » Needs review
FileSize
23.89 KB
3.66 KB

This should resolve #25

A bit unsure about data values provided by core/modules/serialization/tests/fixtures/serialization-module-installed.php.

Edit: or should we extend the fixture from rest module to cover this like @Berdir mentioned in #3034062-14: Remove hal.module BC layers?

Status: Needs review » Needs work

The last submitted patch, 26: 2893795-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

+++ b/core/modules/serialization/tests/src/Functional/Update/SerializationSettingsDeletionUpdateTest.php
@@ -0,0 +1,40 @@
+    // @todo Remove this in https://www.drupal.org/project/drupal/issues/3095333.
+    \Drupal::entityDefinitionUpdateManager()->installEntityType(\Drupal::entityTypeManager()->getDefinition('serialization_resource_config'));
+

These lines should be removed; that was only relevant for the REST module :)

EntityTestDatetimeTest and EntityTestDateRangeTest also still need to be updated.

sokru’s picture

Status: Needs work » Needs review
FileSize
23.91 KB
3.67 KB

Correct path for fixture.

sokru’s picture

Still misses EntityTestDatetimeTest and EntityTestDateRangeTest.

Wim Leers’s picture

Issue tags: -Needs tests

Tests were added in #26, so removing that issue tag. Awaiting test results.

The last submitted patch, 29: 2893795-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 30: 2893795-30.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs work » Needs review
FileSize
25.81 KB
4.98 KB

Same as in the hal issue, simplifying the update test by using the filled dump that already has the module enabled.

FWIW, bc_primitives_as_strings is really confusing as it uses TRUE to disable the BC module and then the change record talks about how to "enable" it o.0

Also fix the remaining test fail. Took me a while to figure out out (insert usual rant about generic rest test fails being hard to understand & locate), but turned out to be pretty easy, just had to remove the altered patch entity normalization that tested BC.

Status: Needs review » Needs work

The last submitted patch, 34: 2893795-34.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Down to 1 failure! Nearly there :)

FWIW, bc_primitives_as_strings is really confusing as it uses TRUE to disable the BC module and then the change record talks about how to "enable" it o.0

I think it was the very first BC flag ever in core. Getting that done involved a string of confusions … (pun intended!)

Took me a while to figure out out (insert usual rant about generic rest test fails being hard to understand & locate)

🤗

Berdir’s picture

that remaining should be easy, we just need to change that test in the same way as I did the other date test already (remove the message abut the BC thing in the expected error message).

Berdir’s picture

Assigned: Unassigned » Berdir
Berdir’s picture

Assigned: Berdir » Unassigned
Status: Needs work » Needs review
FileSize
27.06 KB
1.9 KB

Fixed that last test fail.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/serialization/serialization.post_update.php
@@ -0,0 +1,13 @@
+ * Remove obsolete serilization.settings configuration.

🙈 s/serilization/serialization/

Can be fixed on commit. Or … just be left in there.

  • catch committed 1d1f222 on 9.0.x
    Issue #2893795 by sokru, Wim Leers, Berdir, Stefdewa: Remove...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1d1f222 and pushed to 9.0.x. Thanks!

Status: Fixed » Closed (fixed)

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