All serialized values are strings, should be integers/booleans when appropriate

HI. I'm wondering whether this is just an issue with hal_json or also one of json itself?

Good to know!

I guess adapting \Drupal\serialization\Normalizer\ComplexDataNormalizer and \Drupal\hal\Normalizer\FieldNormalizer
using \Drupal\Core\TypedData\PrimitiveInterface::getCastedValue is probably the way to go.

Yeah in general I'm wondering whether you would consider this as breaking change for other users of the api, aka. should this behaviour be opt in/opt out by default

Yeah not sure we have clearly thought about it, but yeah I would vote for an opt IN feature. It doesn't seem to be a break worth risking.

Moving to serialization.module because this affects both json and hal_json, yet 100% of the changes are in the Serialization module. So it makes more sense as the component. It'll need to be RTBC'd by the maintainer of that component, damiankloip.

I've also noticed this while working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

Yes, this makes a lot of sense to me in general. I think we didn't do this a couple of years ago because it was tricky for reasons I forget now. But time has elapsed since then so I don't remember the details now :P

+++ b/core/modules/serialization/src/Normalizer/TypedDataNormalizer.php
@@ -18,7 +18,7 @@ class TypedDataNormalizer extends NormalizerBase {
+    return $object->getCastedValue();

This is potentially a problem in the latest patch. TypedDataInterface does not contain the getCastedValue() method. That comes from the PrimitiveInterface. So we might need a new normalizer to handle this instead. Incidentally, as the IS states it might do but doesn't seem to be :) I think we might need a new normalizer for PrimitiveInterface that comes before TypedData and does what this current patch has done?

This is potentially a problem in the latest patch. TypedDataInterface does not contain the getCastedValue() method. That comes from the PrimitiveInterface.

Oh, darn! We're fortunate to have your eagle eyes!

I think we might need a new normalizer for PrimitiveInterface that comes before TypedData and does what this current patch has done?

That sounds entirely sensible!

I could imagine we added it after serialization module went in.

Yes, right on Daniel. I think that seems pretty likely.

Right :)

#2002102: Move TypedData primitive types to interfaces added getCastedValue(), on June 30, 2016.

The serialization functionality has been in Drupal core since 2012, and was moved into a module of its own in #1903784: Move serialization to own module, on February 9, 2013.

OK, here is a new approach (mentioned by me in #16). It does not rely on ComplexDataInterface anymore, as that doesn't contain the getCastedValue method anyway. The plus side is this also requires no other changes to ComplexDataInterface and anything implementing that. From my testing this works fine. Let's see what the testbot thinks. I didn't do an interdiff as ever line is different.

+++ b/core/modules/serialization/tests/src/Unit/Normalizer/PrimitiveDataNormalizerTest.php
@@ -0,0 +1,75 @@
+  protected $typedData;

Need to fix this docblock and property name.

The varous primitve types added to the test are kind of pointless at the moment too. I will try and change to use the actual classes for real.

Updated tests to use real data objects.

A couple of nitpicks. Overall it looks nice!

1. +++ b/core/modules/serialization/src/Normalizer/PrimitiveDataNormalizer.php
   @@ -0,0 +1,24 @@
   +  protected $supportedInterfaceOrClass = 'Drupal\Core\TypedData\PrimitiveInterface';
   

   Note: We could use PrimitiveInterface::class here, see https://3v4l.org/JuLFp

2. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/PrimitiveDataNormalizerTest.php
   @@ -0,0 +1,80 @@
   +    $data[] = [$string, 'test'];
   ...
   +    $data[] = [$integer, 5];
   ...
   +    $data[] = [$boolean, TRUE];
   

   Note: We could use some string keys here, like 'string', 'integer' and 'boolean'.

Fix for the serialization test.

Do we care taht HHVM doesn't seem to like that approach? Some people at least attempt to use D8 with HHVM? Adding the keys to the data provider is certainly a good plan!

That seems to be a temporary bug for HHVM, because well, it works on either HHVM versions, see

Output for 5.6.0 - 5.6.27, hhvm-3.12.0 - 3.13.2, 7.0.0 - 7.1.0RC5

well, let's do it then.

Yes, this might not work with hal module as it is. I think the only place this is a problem is FieldItemNormalizer::normalize(), as it's doing $values = $field_item->toArray();. Ideally this field item would be iterated instead and passed to the serializer?

I.e. like this. I think this should work?

Those hal tests set up a mock serializer, which seems counter productive in a kernel test like that. It should use the actual service that has been set up correctly...

I agree this looks great!

1. +++ b/core/modules/serialization/serialization.services.yml
   @@ -17,6 +17,10 @@ services:
   +  serializer.normalizer.primitive_data:
   +    class: Drupal\serialization\Normalizer\PrimitiveDataNormalizer
   +    tags:
   +      - { name: normalizer, priority: 5 }
   
   +++ b/core/modules/serialization/src/Normalizer/PrimitiveDataNormalizer.php
   @@ -0,0 +1,26 @@
   +  public function normalize($object, $format = NULL, array $context = array()) {
   +    return $object->getCastedValue();
   +  }
   

   This is essentially what is changing. It looks great!

2. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/PrimitiveDataNormalizerTest.php
   @@ -0,0 +1,80 @@
   + * @coversDefaultClass \Drupal\serialization\Normalizer\TypedDataNormalizer
   

   Wrong class.

3. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/PrimitiveDataNormalizerTest.php
   @@ -0,0 +1,80 @@
   +  protected function setUp() {
   

   Nit: missing inheritdoc.

4. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/PrimitiveDataNormalizerTest.php
   @@ -0,0 +1,80 @@
   +    // Test that an object not implementing TypedDataInterface fails.
   

   Wrong class.

5. The failures are all about the read-only comment fields:
   

   Value '{"message":"Access denied on updating field \\u0027status\\u0027."}' is identical to value '{"message":"Access denied on updating field \\u0027cid\\u0027."}'.
   

   I suspect that what is going on is the original entity having string values for a particular field, and the denormalized entity having int/bool/… values for the same field, and hence it detecting inequality and failing.

I don't see that this trickles up to the HAL module though.

For this reason, I think we need to have HAL+JSON test coverage too.

What kind of hal test coverage do you mean? seems that the hal normalization tests are ok with this? or do you mean tweak the existing tests to type check too?

I mean: we want to have a test specifically asserting that HAL+JSON responses also have ints/bools.

ok, yes. Agree. I think we do the same as we have done for the serialization module tests. Check the type too, not just assertEquals.

+1!

There are some interesting test failures left:

exception: [Notice] Line 385 of core/modules/rest/src/Tests/CreateTest.php:
Undefined index: uuidDrupal\rest\Tests\CreateTest->assertCreateEntityOverRestApi('comment', '{"_links":{"self":{"href":""},"type":

I'm looking into the failures for now

Woot!

I hope that's it :)

1. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -22,8 +22,10 @@ class FieldItemNormalizer extends NormalizerBase {
   +      // @todo Support nullable types on the lowest levels.
   

   s/on the lowest levels/in TypedData/

2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -292,6 +292,7 @@ public function testUpdateComment() {
   +      'cid',
   
   @@ -304,13 +305,14 @@ public function testUpdateComment() {
   +      'cid',
   

   Why?

Why?

Mh, I don't understand it at the moment anymore, I mean its kind of the problems , mhhh, sorry I'm confused right now.

So this needs an opt-in BC flag, much like #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources did in rest.settings. But we need it in serialization.settings, so it's actually more like #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, which is currently in progress.

Hmm, the opt-in would have to be to decide whether the normalizer is added to the container I guess.

#59: This new normalizer is always active, as long as the serialization module is installed. So any existing D8 site with the serialization module installed would need to keep the old behavior (but allow opting in to the new behavior), and any fresh installation of the serialization module would get the new behavior from the start.

But this would essentially render this normalizer useless, we would need to add this in a supportsNormalization implementation or something. I would rather see this just not added to the serializer in that case. Bringing config into normalizers seems questionable, which is kind of what I was trying to convey in #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX.

I don't understand what you mean. Can you explain why this normalizer would be useless? I think you also want to maintain BC for existing sites?

I just read #2768651-36: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX.3, where you wrote:

Related to my comment https://www.drupal.org/node/2751325#comment-11794602 I think we should consider not having config knowledge in a normalizer iteself, I don't think this is the right place for that implementation detail to live. Maybe based on the BC option we add the normalizer (or configure it differently) or not.

Ahhh! I get it now :) Yes, that makes a ton of sense. Keeping the normalizer simpler. Keeping the conditionality out of it. +1!

#64: yes :)

Yes, pretty much. We already have a service provider, so it would probably just be adding this in another compiler pass.

Maybe my plan is flawed a little... using config in a compiler pass might not really work :)

Well, \Drupal\language\LanguageServiceProvider does this too. It uses BootstrapConfigStorageFactory::get().

Inspired by that, I did something similar for the CDN module: it only adds a particular middleware if the CDN module's config has a certain key set to a certain value. And I think that provides the clearest & closest example to what we need here:

class CdnServiceProvider implements ServiceProviderInterface {

  /**
   * {@inheritdoc}
   */
  public function register(ContainerBuilder $container) {
    if ($this->cdnStatusIsEnabled()) {
      $container->register(…)->…
  }

  /**
   * @return bool
   */
  protected function cdnStatusIsEnabled() {
    return BootstrapConfigStorageFactory::get()->read('cdn.settings')['status'] === TRUE;
  }

}

I don't see why that cannot work for this use case as well.

Ah @WimLeers, that's a good idea! Let's do that then!

Here's a reroll with UpdateTest changes missing. Not sure what happened to that test atm.

Can we guarantee that typed data is going to iterate over typed data instances? The docs are kind of unclear but I'm assuming so. However, why don't we move those checks to the actual normalizer that are meant to be getting the values instead? That should work I think? Let's see if we'er in the same position on failures.

Also, just added all the BC stuff too. Some inspiration and stuff yoinked from https://www.drupal.org/node/2768651 with a couple of methods split out in the compiler pass.

Can we guarantee that typed data is going to iterate over typed data instances? The docs are kind of unclear but I'm assuming so. However, why don't we move those checks to the actual normalizer that are meant to be getting the values instead? That should work I think?

I don't know. I don't know Typed Data well enough to be able to answer that.

1. +++ b/core/modules/serialization/config/schema/serialization.schema.yml
   @@ -0,0 +1,10 @@
   +    bc_timestamp_normalizer_unix:
   +      type: boolean
   +      label: 'Whether the pre Drupal 8.3.x behavior of returning non-casted values from typed data (primitive) items.'
   

   This one is for #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, not this issue.

2. +++ b/core/modules/serialization/serialization.install
   @@ -0,0 +1,25 @@
   +  $rest_settings = $config_factory->getEditable('serialization.settings');
   

   s/$rest_settings/$serialization_settings/

3. +++ b/core/modules/serialization/src/RegisterSerializationClassesCompilerPass.php
   @@ -53,6 +58,36 @@ public function process(ContainerBuilder $container) {
   +  protected function normalizerShouldBeSkipped($id) {
   +    switch($id) {
   +      case 'serializer.normalizer.primitive_data':
   +        return $this->normalizerBcSettingIsEnabled('bc_primitive_data_normalizer');
   +      default:
   +        // Default to FALSE as most normalizers will be added.
   +        return FALSE;
   +    }
   +  }
   

   Nice, this sets up the infrastructure for more such cases in the future!

So, regarding the test failures:

-{"message":"Access denied on updating field \u0027changed\u0027."}
+{"message":"Access denied on updating field \u0027cid\u0027."}

This means it was expecting that the first field for which "no access" error response is returned would be the changed field. But with this patch, that's now the cid field.

The reason for that failed assertion (and all others) is simple: some things that are strings in HEAD aren't strings anymore with this patch.

For this particular case, I added this debug output to EntityResource::patch():

         // Unchanged values for entity keys don't need access checking.
+        if ($field_name === 'cid') {
+          var_dump($original_entity->get($field_name)->getValue());
+          var_dump($entity->get($field_name)->getValue());
+        }
         if ($original_entity->get($field_name)->getValue() === $entity->get($field_name)->getValue()) {

which then outputs:

1) Drupal\Tests\hal\Functional\EntityResource\Comment\CommentHalJsonAnonTest::testPatch
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"Access denied on updating field \u0027changed\u0027."}
+array(1) {
+  [0]=>
+  array(1) {
+    ["value"]=>
+    string(1) "1"
+  }
+}
+array(1) {
+  [0]=>
+  array(1) {
+    ["value"]=>
+    int(1)
+  }
+}
+{"message":"Access denied on updating field \u0027cid\u0027."}

As you can see, $original->get($field_name)->getValue() returns a string, but $entity->get($field_name)->getValue() returns an integer.

The real question here is how it's possible that getValue() can return a different value.

Attached reroll fixes it at least for the failing Node tests.

BTW, the test coverage in EntityResourceTestBase is not yet strictly validating types. It's only testing equality, which allows for typecasting. We may need to change that as part of this patch.

i.e. these assertions:

    // Comparing the exact serialization is pointless, because the order of
    // fields does not matter (at least not yet). That's why we only compare the
    // normalized entity with the decoded response: it's comparing PHP arrays
    // instead of strings.
    $this->assertEquals($this->getExpectedNormalizedEntity(), $this->serializer->decode((string) $response->getBody(), static::$format));
    // Not only assert the normalization, also assert deserialization of the
    // response results in the expected object.
    $unserialized = $this->serializer->deserialize((string) $response->getBody(), get_class($this->entity), static::$format);

The real question here is how it's possible that getValue() can return a different value.

I think I can explain this behaviour. When you store data via PDO and retrieve it again, you get back strings. Welcome to the world of non strict languages.
Given that the $original_entity is retrieved from the DB it gets the string values. #2310089: ATTR_EMULATE_PREPARES is not necessary for newer mysql versions anymore. explains you more about that.

Here is a try to cast not only a single value but rather an entire field item list value, so we can compare it. At least the comment test passes with that.
Here is an implementation of that together with some documentation and improved test coverage.

I think I like this approach I think. It could lead to some additional field items getting saved if you had older style stringified data posted to an endpoint using this code but I think that overhead is relatively minor in this case.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -192,6 +227,8 @@ public function post(EntityInterface $entity = NULL) {
   +    /** @var \Drupal\Core\Entity\FieldableEntityInterface|\Drupal\Core\Entity\EntityInterface $original_entity */
   +    /** @var \Drupal\Core\Entity\FieldableEntityInterface|\Drupal\Core\Entity\EntityInterface $entity */
   
   @@ -221,7 +258,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +        if ($this->getCastedValueFromFieldItemList($original_entity->get($field_name)) === $this->getCastedValueFromFieldItemList($entity->get($field_name))) {
   

   It's not really the fault of this patch as it's already happening in some other places in rest module but E.g. patch/post methods are type hinting EntityInterface but really depend on FieldableEntityInterface.

2. +++ b/core/modules/serialization/src/Normalizer/PrimitiveDataNormalizer.php
   @@ -19,7 +19,13 @@ class PrimitiveDataNormalizer extends NormalizerBase {
   +    // Typed data casts NULL objects to their empty variants, so for example
   

   Maybe 'NULL values', we don';t have NULL objects as such. We wish we did ;)

   Maybe replace 'In a better world ...' with 'Ideally ...'.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
@@ -136,7 +136,7 @@ protected function getExpectedNormalizedEntity() {
+          'value' => FALSE,

Not sure this is needed now with this patch, or for parity, we should actually now replace all the values in these getExpectedNormalizedEntity methods with their casted values where appropriate?

we should actually now replace all the values in these getExpectedNormalizedEntity methods with their casted values where appropriate?

This.

Plus what I wrote in #77: we should now strictly compare the expected normalization (strict in terms of types, not in terms of field order).

It's not really the fault of this patch as it's already happening in some other places in rest module but E.g. patch/post methods are type hinting EntityInterface but really depend on FieldableEntityInterface.

Well, we should check for fieldableentity interface, but our code should support config entities in the future without breaking signatures, if possible.

Let's see how many places we assume the wrong values now.

AFAIK assertSame() also requires key order to be the same, which would explain the failed tests.

Yes assertSame() cares about the exact order too. We can try this, sorting the array first. See how we get on. Otherwise we will need a helper assertion method to iterate over the array recursively, compare each scalar value identically and make sure all keys exist.

As far as upgrade path tests to, I think we should just maybe test that when enabling the module you get the normalizer and updating you don't? How easily can we mimic an already installed serialization module? It might be better to just test this on a unit level for the compiler pass?

Missed one.

This is looking awesome. Wanted to RTBC, but yes, upgrade path tests are still missing. (The BC layer and upgrade path itself is present, so removing those issue tags.)

In terms of upgrade path test coverage: \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase always tests the "current defaults" first, and THEN it tests the BC layer for bc_entity_resource_permissions:

public function testPatch() {
…

    // 200 for well-formed request.
    $response = $this->request('PATCH', $url, $request_options);
    $this->assertResourceResponse(200, FALSE, $response);
    $this->assertFalse($response->hasHeader('X-Drupal-Cache'));
    // Ensure that fields do not get deleted if they're not present in the PATCH
    // request. Test this using the configurable field that we added, but which
    // is not sent in the PATCH request.
    $this->assertSame('All the faith he had had had had no effect on the outcome of his life.', $this->entityStorage->loadUnchanged($this->entity->id())->get('field_rest_test')->value);

    // EVERYTHING ABOVE IS TESTING THE CURRENT DEFAULTS

    $this->config('rest.settings')->set('bc_entity_resource_permissions', TRUE)->save(TRUE);
    $request_options[RequestOptions::BODY] = $parseable_valid_request_body_2;
    // @todo Remove this in https://www.drupal.org/node/2815845.
    drupal_flush_all_caches();


    // DX: 403 when unauthorized.
    $response = $this->request('PATCH', $url, $request_options);
    // @todo Update the message in https://www.drupal.org/node/2808233.
    $this->assertResourceErrorResponse(403, '', $response);


    $this->grantPermissionsToTestedRole(['restful patch entity:' . static::$entityTypeId]);


    // 200 for well-formed request.
    $response = $this->request('PATCH', $url, $request_options);
    $this->assertResourceResponse(200, FALSE, $response);
    $this->assertFalse($response->hasHeader('X-Drupal-Cache'));

i.e. we first test current defaults, then we enable a BC flag, and observe the expected different behavior (i.e. 403 because you need to grant this extra permission, then a 200).

I think it'd be best if we do something similar in this case: observe that all integers and booleans are casted to strings. We can check that automatically, by iterating over everything that getExpectedNormalizedEntity() returns and using PHP to cast all integers/booleans to strings, then assert that that is what the server is returning with again a strict equality assertion.

Discussed with damiankloip. I'll be taking on the writing of the test coverage. damiankloip can then review the test coverage. I'm more familiar with that test coverage, so he can work on something else in the mean time, so that should help speed things up :)

This adds BC test coverage to EntityResourceTestBase::testGet().

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -2,6 +2,7 @@
   +use Drupal\Component\Serialization\Json;
   

   Oops. This should be removed.

2. +++ b/core/modules/serialization/config/schema/serialization.schema.yml
   @@ -5,6 +5,3 @@ serialization.settings:
   -    bc_timestamp_normalizer_unix:
   -      type: boolean
   -      label: 'Whether the pre Drupal 8.3.x behavior of returning non-casted values from typed data (primitive) items.'
   

   I fixed this nit while touching the patch anyway.

These changes to the tests look good to me. It's basically what I had locally before you took it over, minus the recursive function :) Just one question really:

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -396,6 +397,28 @@ public function testGet() {
   +    // BC: serialization_update_8301().
   

   Maybe we should consider only running this BC code for content entities? and just wrap this whole hunk in the interface check?

2. +++ b/core/modules/serialization/config/schema/serialization.schema.yml
   @@ -5,6 +5,3 @@ serialization.settings:
   -    bc_timestamp_normalizer_unix:
   

   Sorry, forgot to remove that based on one of your previous comments!

#95.1: hm, that's an interesting idea. OTOH, the current test ensures that nothing changes for config entities, which I thought to be valuable. But you're right, they have a separate ConfigEntityNormalizer so this code naturally doesn't run for them.

I think I prefer what you propose. But I don't have a strong opinion either way. Whichever you prefer is fine by me!

Hm… even though I only touched EntityResourceTestBase (so no code touched, no other tests touched) in #93, #93 is failing in Drupal\Tests\serialization\Kernel\EntitySerializationTest. Does that mean this is a random fail? Retesting…

Here is that change (to only run for fieldable entities). As well as the other test fail. I think..

Just one nit:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -398,25 +398,28 @@ public function testGet() {
+      drupal_flush_all_caches();
 
+      $response = $this->request('GET', $url, $request_options);
...
+    }
 
     // BC: rest_update_8203().

Please ensure two blank lines in these two places, for consistency with the rest of the test file.

Spaces added! :)

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -392,6 +397,34 @@ public function testGet() {
+    }
+
+    // BC: rest_update_8203().

Can you also make sure there's two newlines between these two?

Already RTBC'ing though.

Ha, thanks. Here we are.

Perfect! :)

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -164,7 +164,7 @@ protected function getExpectedNormalizedEntity() {
-          'target_id' => $author->id(),
+          'target_id' => (int) $author->id(),

It's weird you need to case here. I then I remember typed data is not primitively typed :( so

$node->id();
=> "1"

Right. It basically just returns the value as it's stored in the database... it should really return the correct type. I think there might be a reason that never happened. I remember berdir telling me something once. In other sad news, I don't think you can save the value of a Boolean field by setting an actual Boolean value :)

Rerolled

json and hal_json resources will no longer produce just strings, meaning an active compatibility break for systems that assume strings. Most systems already making good use of the existing string values are likely soft-typed enough to minimize the impact.

So we definitely need a CR and we need to think about any breaks that we happen here.

CR created: https://www.drupal.org/node/2837696.

Note that we explicitly don't break BC here. See the CR for details. This was not yet clear in the IS, so updated that too.

1. The BC impacts of this change are tricky to work out because you site could have two third parties that you want to integrate with - one that wants typed primitives and one that doesn't. This change makes that situation possible.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -392,6 +397,35 @@ public function testGet() {
   +      // @todo Remove this in https://www.drupal.org/node/2815845.
   +      drupal_flush_all_caches();
   

   If you do the config listener change below this should not be necessary.

3. +++ b/core/modules/serialization/serialization.install
   @@ -0,0 +1,25 @@
   +function serialization_update_8301() {
   

   8300

4. +++ b/core/modules/serialization/src/RegisterSerializationClassesCompilerPass.php
   @@ -53,6 +58,36 @@ public function process(ContainerBuilder $container) {
   +        return $this->normalizerBcSettingIsEnabled('bc_primitive_data_normalizer');
   ...
   +  /**
   +   * Returns whether a normalizer BC setting is disabled or not.
   +   *
   +   * @param string $key
   +   *
   +   * @return bool
   +   */
   +  protected function normalizerBcSettingIsEnabled($key) {
   +    $settings = BootstrapConfigStorageFactory::get()->read('serialization.settings');
   +    return !empty($settings[$key]);
   +  }
   

   Let's just inline this. Because of this you need a config listener that forces a container rebuild if this setting changes. See \Drupal\language\EventSubscriber\ConfigSubscriber::onConfigSave - basically you need to ensure \Drupal::service('kernel')->invalidateContainer(); is called when this value changes.

5. FILE: ...v/drupal/core/modules/hal/tests/src/Kernel/FileNormalizeTest.php
   ----------------------------------------------------------------------
   FOUND 0 ERRORS AND 7 WARNINGS AFFECTING 7 LINES
   ----------------------------------------------------------------------
   5 | WARNING | [x] Unused use statement
   7 | WARNING | [x] Unused use statement
   8 | WARNING | [x] Unused use statement
   9 | WARNING | [x] Unused use statement
   10 | WARNING | [x] Unused use statement
   11 | WARNING | [x] Unused use statement
   12 | WARNING | [x] Unused use statement
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 7 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
6. FILE: .../drupal/core/modules/hal/tests/src/Kernel/NormalizerTestBase.php
   ----------------------------------------------------------------------
   FOUND 0 ERRORS AND 12 WARNINGS AFFECTING 12 LINES
   ----------------------------------------------------------------------
   5 | WARNING | [x] Unused use statement
   7 | WARNING | [x] Unused use statement
   8 | WARNING | [x] Unused use statement
   9 | WARNING | [x] Unused use statement
   10 | WARNING | [x] Unused use statement
   11 | WARNING | [x] Unused use statement
   13 | WARNING | [x] Unused use statement
   14 | WARNING | [x] Unused use statement
   15 | WARNING | [x] Unused use statement
   16 | WARNING | [x] Unused use statement
   17 | WARNING | [x] Unused use statement
   18 | WARNING | [x] Unused use statement
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 12 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
7. FILE: ...t/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   ----------------------------------------------------------------------
   FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
   ----------------------------------------------------------------------
   5 | WARNING | [x] Unused use statement
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------

Thanks Alex, this is good feedback. I didn;t know the 8300 thing! Silly me. The subscriber has been added. I tested and seems to work as expected. Let's see if the tests like it.

+++ b/core/modules/serialization/src/EventSubscriber/BcConfigSubscriber.php
@@ -0,0 +1,55 @@
+    if ($saved_config->getName() == 'serialization.settings') {

Nit: ===.

No other remarks. Tentatively back to RTBC.

Thanks Wim, I would quickly fix but I don't want to throw anything else at the test runners atm :)

#123: you could cancel the test in #121 and just post a reroll :)

True. That is effort, but why not!

I think I know why this doesn't work in the tests. dfac() makes a call to DrupalKernel::updateModules() which (re) initializes the container. Just relying on the subscriber in tests doesn't work as the container is invalidated but not rebuild straight away.

#129: yep, and similar problem in #2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding.

Just using rebuildContainer() doesn't work... Must be something else that is being relied on in dfac().

It's not about making that the default, it's about keeping responses the same for now. Since Drupal 8.0 all values have been strings. There could be lots of stuff using this, also relying on it being a string for whatever reason. The default behaviour with this patch will be to have typed primitive values, it is only the case where people are upgrading their sites that it will enable the BC flag and not add this new normalizer. People are then welcome to opt out of the BC handling as use the new defaults. Or you are mainly just saying that we should rename the flag we are using? and have this as a generic string casting rather than BC?

Adding back the dfac() call for now.

#134's interdiff pointing to #2815845 to fix it is inappropriate: the scope of that issue is to deal with route rebuilding, nothing else. I think the right solution is to manually reinitialize the container used in the test?

Yeah I think we better should use $this->rebuildContainer();
Also note, the other issue will not solve the problem here to be honest.

#136 and #137, see my comment in #131. However, I think I was not calling the rebuildContainer method on the test class, but on the actual kernel directly.

OK, rebuildContainer() didn't work for me, but rebuildAll() did.

I think this is ready again!

This is blocking #2626924, see #2626924-127: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata.

I'd also argue this is a bug, not a feature.

I realised that I didn't change the variable name in the update hook. This just fixes that (and actually just removes the variable -chaining all the method calls).

Sorry, actual interdiff.

This is a random test failure ...

Needs a reroll.

Rebase FAIL.

Back to RTBC.

OK - me and Alex had a chat on IRC this morning. He is still has BC concerns about API responses changing with this patch. So we settled on always enabling this BC config setting, instead of enabling for modules being updated and disabling it when the module is enabled/installed.

This changes the default config value for 'bc_primitive_data_normalizer' to TRUE, so this matches the update handler.

Wim: This means we need to re-think how we handle string casted vs typed values in EntityResourceTestBase. I have handled this by adding a new getExpectedNormalizedEntityWithCastedStringValues() method, this just wraps the getExpectedNormalizedEntity() value with the string casting. It means we need to check whether the entity is fieldable before doing so. On the plus side, it looks like there are not many occurrences of this to change. If you hate it and think there is a better solution please go for it! :) The alternative seems to be to go back to having all string values in getExpectedNormalizedEntity(), which would mean we'd need to iterate over all the fields for the entity, and cast the values to the appropriate type that way.

OK - me and Alex had a chat on IRC this morning. He is still has BC concerns about API responses changing with this patch. So we settled on always enabling this BC config setting, instead of enabling for modules being updated and disabling it when the module is enabled/installed.

HUH!??!?? So we'll still continue to do the stupid/crazy thing by default? How does that make any sense? :( Then we might as well not do it and say we give up on REST, i.e. we don't maintain it, and all existing bugs will continue to exist.

The whole point is that new sites get sane behavior by default, and existing sites continue to receive the old behavior. Is that not exactly what "backwards compatibility" means? I don't understand what could possibly be "BC concerns about API responses changing".

Of course API responses must change, if the responses' contents don't make change. If we can't change anything about the responses, then we also cannot ever make the REST module's responses remotely sane. Because if this can't change, then none of these can't change either:

1. #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX (this is the one that this issue is blocking)
2. #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field
3. #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs
4. #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata
5. #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration
6. #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias
7. … plus the other ones listed in #2794263: REST: top priorities for Drupal 8.3.x
8. … plus any future "serialization gap" issues

I looked back over the discussion of @damiankloip and @alexpott on IRC.

My understanding of @alexpott's concern is this

1. Company X writes a client/product/service that interacts with Drupal's REST API
2. Company X expects their client to work with Drupal's REST API across multiple sites whether that is many internal sites or their clients' sites if they are offering a service or make Drupal websites for others
3. Company X doesn't want to have to worry about which sites were installed at 8.2.x, 8.3.x , 8.4.x etc and therefore would have different BC settings for this issues or other issue that @wim leers listed in #156
4. Company X wrote their client at 8.0.0(or 8.2.x whatever) and expect it to work until 9.0.0

If we could make the above work it would have the advantage of being able to write clients for Drupal's REST API and not have to worry when a site was installed as far as basic things about the response such as data types. For clients that write a service that deals with multiple Drupal 8 sites this would be a major advantage.

For any individual site that was installed past 8.2.x this in many cases though would be a major disadvantage. Drupal 8's REST module had many problems at 8.0.0 and we are working to improve it, this being just 1 example. These issues have a lot of input and the changes mostly seemed to agreed upon to be just better for clients. If all new sites could not get these changes by default(or very easily) Drupal's REST would still be perceived as broken.

So how to solve this problem and let clients be written that work across multiple drupal sites and also let new sites easily get all the improvements we are making?

Here is my suggestion:

1. Accept a new Header x-drupal-minor-version.
2. This would take values 8.2, 8.3, etc.
3. Instead of checking config settings like bc_primitive_data_normalizer directly use a new service RESTSettingService::checkSetting(bc_primitive_data_normalizer)
4. If no x-drupal-minor-version is provide check bc_primitive_data_normalizer(others) directly.
5. bc_primitive_data_normalizer should be set to false for new sites
6. If the header is provided then this service would be responsible for keeping track of when bc_x was introduced and always return the setting that was in effect when that version of Drupal was released.

One other addition would be to able to set a site wide setting REST Drupal drupal-minor-version. It would be checked by RESTSettingService::checkSetting() if the header was not present.

So why "bc_primitive_data_normalizer should be set to false for new sites"?

I think new sites are going to expect saner defaults. People new to Drupal don't want to have to check through all the bc_X's and make updates and are probably going to start writing their clients before they figure out they exist.

Developers like "Company X" are probably more familiar with Drupal or at least writing their client is more a major part of their practice then someone installing Drupal once and making a couple REST requests.

An important distinction is that this is not API version and maybe x-drupal-minor-version is bad name to avoid confusion.

We had a call/discussion with @damiankloip, @tedbow, @drpal, @prestonso and myself about #155, i.e. about Alex Pott's concerns. For completeness, I've also attached the chat log of damiankloip + Alex Pott for posterity.

Interesting points that came up that hadn't come up fore yet:

1. Alex is concerned about breaking BC by changing normalizers. But there's already no way to guarantee BC, because any module can choose to alter a normalizer. So imagine that this patch would be moved to a better_rest contrib module that would be born out of frustration if issues like this don't land: Alex' sample scenario would still be broken!
2. Alex' concerns about a site integrating with services A and B, and A and B expecting different serializations/normalizations would be easily solved by using a custom format. In fact, that's what 100% of the services that talk to Drupal sites today are using:
   • https://www.drupal.org/project/tag1quo + https://quo.tag1consulting.com/
   • https://www.drupal.org/project/acquia_contenthub + https://www.acquia.com/products-services/acquia-content-hubvid
3. @drpal is a JavaScript developer. He didn't realize this issue was happening, but that he was very happy that it is happening because it drove him, I quote, nuts.
4. Finally, @damiankloip made the observation that we have to weigh the good vs the bad. How many services are actually affected? Versus how many JavaScript developers are actually affected? Because it's still more typical that a D8 site's REST API is consumed only by that site's front end. So bringing this improvement to everyone by default would do far more good than the few (if any) services that would be affected.

I have done quite a lot Android development. Android uses Java, which is a more type strict language. Having to deal with bug makes the client side less efficient.

I do have a question: PHP BC Math extension outputs numbers as String. I am wondering if this patch would automatically converts BCMath outputs to the correct type, or it becomes the module's responsibility to convert BCMath's result when receiving response via REST?

To me, it seems very important to make improvements to REST like this one available out-of-the box. Otherwise, less experienced developers will see core REST's default functionality and think that is all they can expect from it. I get @alexpott's concerns, but it seems unrealistic to expect disparate Drupal systems to all be compatible, unless you put some effort into making them so.

Reuploading the RTBC patch of #153 to be able to mark this RTBC, to be able to get feedback from a core committer to #156 + #157 + #158 + #161. This issue is super important for REST/serialization, and blocks tons of other progress.

Random DateRange test fail. Retested and set to Needs Review

A different random fail? FilterFormatAccessTest

trying again.

Needs a reroll for 8.4.x

For me I would make the bc layer opt-in rather than opt-out.

This is a bug fix that may or may-not break an existing client. If it doesn't break a client, then the site gets updated and will run happily ever after.

If it does get broken, there's a quick fix to enable the bc layer if you don't want to fix/don't have control over the client.

If we do it the current way, then assuming we leave the bc layer in until 9.x and remove it then, it just increases the chances (by the number of clients implemented between now and then that rely on string output that otherwise wouldn't have been) that people run into this problem when they upgrade to 9.x

Re-roll (started before comment #172 so does not take that into account).

Okay so @catch agrees with what we've been proposing… but @alexpott does not. Either catch and Alex Pott need to discuss this among themselves (they're both core committers), or we all need to get on a call together. What do you think?

Yes it does look as though @catch and are coming at this from opposite view points. As this basically a release management question of how we break I’m happy to defer my opinion. I would have expected the release manager to be more conservative than the framework manager - but as catch is both I guess you never can guess :)

The most recent approach #162/#173 is opt in BC so we're inline with @catch's wishes.

#173 is missing the PrimitiveDataNormalizer.php file that was in #162.

+++ b/core/modules/serialization/serialization.install
@@ -12,6 +12,17 @@
+function serialization_update_8300() {

I discussed this with @catch and @alexpott, and we agreed on removing this update function from this patch. And then creating a followup issue in which to discuss whether to add it or not. That way, if this patch lands into 8.3 (which is a big if, since that's only about 12 hours until the beta commit freeze), we have the rest of the beta/rc cycle to decide on what we want the default to be for existing sites in production.

@Jo Fitzgerald thanks for re-rolling. I think all the errors were around \Drupal\serialization\EventSubscriber\BcConfigSubscriber not existing

Re-rolled #162 again. So from reading #162 to onwards I think should be good for RTBC if the tests pass

I diffed the patch in #179 with the one in #162 to make sure all it contains is a reroll plus the removal of serialization_update_8300(). Since that's the extent of the change, back to RTBC.

1. +++ b/core/modules/serialization/serialization.install
   @@ -39,5 +39,16 @@ function serialization_update_8301() {
   +function serialization_update_8302() {
   +  $config_factory = \Drupal::configFactory();
   +
   +  $config_factory->getEditable('serialization.settings')
   +    ->set('bc_primitive_data_normalizer', TRUE)
   +    ->save(TRUE);
   +}
   

   Per #178, we should remove this. Except I don't know if we should remove the entire function, or just make it set the value to FALSE instead. What have we done elsewhere? Do we always need an update function when introducing a new config key, even if we want to set it to the default?

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -464,6 +468,36 @@ public function testGet() {
   +    // BC: serialization_update_8300().
   ...
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -527,6 +561,32 @@ public function testGet() {
   +   * @see serialization_update_8300()
   

   Since in this issue, the update function, if it exists, would only set the new config value to its default, we shouldn't reference it from these places. I think it's fine to just remove these lines, and if we need more clarifying comments, we could punt that to a followup.

3. +++ b/core/modules/serialization/config/install/serialization.settings.yml
   @@ -1,3 +1,12 @@
   +# has been introduced to get the casted value instead. If this module is
   +# installed the new behaviour will be the default. For modules updating this BC
   +# option will be enabled.
   +# @see serialization_update_8300()
   

   With #1, this would no longer be true. Both new sites and existing sites would get the same default. So this comment needs an update. Again, a simple removal of incorrect information would be fine, and non-trivial comment additions could be punted to a followup.

4. +++ b/core/modules/serialization/config/install/serialization.settings.yml
   @@ -1,3 +1,12 @@
   +bc_primitive_data_normalizer: false
   

   This is a confusing name. Because if false, we run the normalizer. And if true, we don't. If we want false to be the value for getting the new behavior and true to be the value for getting the bc behavior, then perhaps rename to bc_primitives_as_strings or similar?

5. +++ b/core/modules/serialization/config/schema/serialization.schema.yml
   @@ -6,3 +6,6 @@ serialization.settings:
   +    bc_primitive_data_normalizer:
   +      type: boolean
   +      label: 'Whether the pre Drupal 8.3.x behavior of returning non-casted values from typed data (primitive) items.'
   

   That label is not a grammatically correct phrase. Maybe something like 'Whether to retain pre Drupal 8.3 behavior of serializing all primitive items as strings.'?

@effulgentsia good points
1. I am removing this. I don't see why we need an update function because when reading the config we would get the default anyways.

2. removed
3 removed. Left the @see to this issue
4. used "bc_primitives_as_strings" that seems good.
5. Replaced with suggestion.

So I asked the other committers earlier but guess it did not get posted back here yet; shouldn't there be an upgrade path regardless of whether it's setting a TRUE or a FALSE? It's still an addition to the schema and so leads to diffs if it's saved explicitly (even as FALSE) any time after a site upgrades to whatever release contains it. If we provide an upgrade path, config changes once as of the release; if we don't, it can change randomly and add stuff confusingly in the future.

We discussed possibly announcing this on g.d.o/core when we do make the change, and adding it to the release notes of whatever release contains it. Given that the bugfix could break client apps that don't even know or care their data is coming from a D8 site, I'd like us to be pretty confident in our approach to handling the BC and upgrade path. For me trying to sort it in order to backport with less than 24h to the commit freeze for the beta makes it difficult to be confident in the solution. So I would prefer to do this for 8.4.x only. It's a DX frustration, but it's also the current behavior, and it can be worked around.

Also in general I think it is a bad idea to defer upgrade paths (or even decisions about them) to followups except in emergency; it has always bit us in the past when we do and it makes the release less shippable which is a no-no for the Drupal 8 release schedule.

+++ b/core/modules/serialization/config/install/serialization.settings.yml
@@ -1,3 +1,9 @@
+# @see https://www.drupal.org/node/2751325

In general, we do not add @see to the issue that introduced a change, only to followup issues not yet done if there is need for an @todo. If people want to see which issue introduced something for background, they can use git blame. But for someone just reading the schema for the documentation, this issue is totally overwhelming and will not give them the information they need. They will open the page because the docs tell them to, and then be at a loss as to what they're supposed to do, more confused than before.

If the existing paragraph is insufficient to explain the setting, then we should add more documentation to the comment to explain anything else they need to know.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -196,6 +198,39 @@ public function post(EntityInterface $entity = NULL) {
   +   * Returns the casted value of a field item list.
   ...
   +   *   The casted value from the field item list.
   ...
   +  protected function getCastedValueFromFieldItemList(FieldItemListInterface $field_item_list) {
   

   "Casted" is not a contemporary English word, at least not American English. The past participle is "cast".

   ...It looks like this misspelling exists in our typed data API already. So I guess that is out of scope.

   However, maybe we can at least make the docs a bit better. A "cast[ed] value" is ambiguous, it could mean cast to string, or cast to the logical type, or anything. The docs for PrimitiveInterface::getCastedValue() say "Gets the primitive data value casted to the correct PHP type."

   So I think the words "to the correct type" explain the purpose a lot better.

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -196,6 +198,39 @@ public function post(EntityInterface $entity = NULL) {
   +   * Currently just primitive data knows how to cast itself to its actual value.
   

   This is really confusing to read. Its value is its value, and casting can change its value. What is actually happening is that the item is being cast to the canonical data type for the data.

There should be an upgrade path to set the config value explicitly, to avoid diffs with config import/export just due to updating core. It's a minor reason for an upgrade path, but still a good one.

@xjm the follow-up isn't about deferring the upgrade path, it's about potentially revisiting the decision on the default for the bc layer if the current approach here turns out to break a lot of real clients. There are arguments for and against each direction.

We have no idea how many clients are written to work with multiple different Drupal sites vs. specifically for one site and the extent to which the person running the client has control over the site in the latter case. The impact of either decision could be different for each of those cases.

One issue with leaving this until 8.4.x is that's an extra six months for people to write clients based on the current behaviour that could break when this is committed. i.e. the earlier this is fixed, the less it will break. It does give a longer window to find out how much it breaks before it gets into a patch release though.

Yet another option is commit it early, but revert it from 8.3.x during beta if we run into unexpected (or bigger than expected) issues.

Discussed this a bit more with @catch this morning. One of the things that makes this issue extremely tricky is that the longer we leave it but want to change it the worse the BC issues will get.

It feels like there are two options with the least worst outcomes:
EITHER: get this in and default to typed values. And suffer the BC consequences - and at least there is a workaround for any site affected.
OR: postpone this issue on REST API versioning - need to search for an existing issue. REST API versioning is likely to be extremely tricky to implement.

It's up to the D8 release managers - @catch, @cilefen and @xjm to make the call here - I don't envy the choice.

Patch attached puts the update function back but changes the default to not use the BC layer and returns a message so users are at least informed. It's important that the change record contains instructions on how to enable the BC mode since you can't do that via the UI and fixes @xjm's comments #186 and #185

EITHER: get this in and default to typed values. And suffer the BC consequences - and at least there is a workaround for any site affected.

Yes.

OR: postpone this issue on REST API versioning - need to search for an existing issue. REST API versioning is likely to be extremely tricky to implement.

Nobody in the world has solved API versioning! Postponing this on that being solved is pretty much equivalent with postponing forever.

I very much like the approach in #188.

I'd lean towards getting this in now. If there turns out to be fallout we have a chance to revert it (or reverse the bc layer decision) before 8.3.0 still.

I don't really like waiting until 8.4.x or starting with the bc layer turned off, since that's likely going to end up with more fallout at a later date.

#190++. We can still revert before 8.3.0.

I'm being bold: RTBC'ing again.

+++ b/core/modules/serialization/serialization.install
@@ -39,5 +39,17 @@ function serialization_update_8301() {
+  return t('The REST API will no longer output all values as strings. Integers/booleans will be used where appropriate. <a href="https://www.drupal.org/node/2837696">If your site depends on these value being strings, read the change record to learn how to enable the BC mode.</a>');

How about a hook_requirements() warning based on the value of the BC flag?

+++ b/core/modules/hal/tests/src/Kernel/FileNormalizeTest.php
@@ -33,20 +24,6 @@ class FileNormalizeTest extends NormalizerTestBase {
-    $entity_manager = \Drupal::entityManager();
-    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend(), \Drupal::moduleHandler(), \Drupal::service('config.factory'), \Drupal::service('request_stack'), \Drupal::service('entity_type.bundle.info')), new RelationLinkManager(new MemoryBackend(), $entity_manager, \Drupal::moduleHandler(), \Drupal::service('config.factory'), \Drupal::service('request_stack')));
-
-    // Set up the mock serializer.
-    $normalizers = array(
-      new FieldItemNormalizer(),
-      new FileEntityNormalizer($entity_manager, \Drupal::httpClient(), $link_manager, \Drupal::moduleHandler()),
-    );
-
-    $encoders = array(
-      new JsonEncoder(),
-    );
-    $this->serializer = new Serializer($normalizers, $encoders);

+++ b/core/modules/hal/tests/src/Kernel/NormalizerTestBase.php
@@ -130,23 +117,7 @@ protected function setUp() {
-    $entity_manager = \Drupal::entityManager();
-    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend(), \Drupal::moduleHandler(), \Drupal::service('config.factory'), \Drupal::service('request_stack'), \Drupal::service('entity_type.bundle.info')), new RelationLinkManager(new MemoryBackend(), $entity_manager, \Drupal::moduleHandler(), \Drupal::service('config.factory'), \Drupal::service('request_stack')));
-
-    $chain_resolver = new ChainEntityResolver(array(new UuidResolver($entity_manager), new TargetIdResolver()));
-
-    // Set up the mock serializer.
-    $normalizers = array(
-      new ContentEntityNormalizer($link_manager, $entity_manager, \Drupal::moduleHandler()),
-      new EntityReferenceItemNormalizer($link_manager, $chain_resolver),
-      new FieldItemNormalizer(),
-      new FieldNormalizer(),
-    );
-
-    $encoders = array(
-      new JsonEncoder(),
-    );
-    $this->serializer = new Serializer($normalizers, $encoders);
+    $this->serializer = $this->container->get('serializer');

At first I saw red in tests and saw red because I couldn't find the replacement, but this is deleting test setup code, presumably because we don't need it anymore with this fix. So that's cool.

Also maybe the flag being TRUE should always go on the status report regardless, to warn people that their API is different from the current expectation.

1. +++ b/core/modules/serialization/config/schema/serialization.schema.yml
   @@ -6,3 +6,6 @@ serialization.settings:
   +      label: 'Whether to retain pre Drupal 8.3 behavior of serializing all primitive items as strings.'
   

   Nit: pre-Drupal 8.3.

2. +++ b/core/modules/serialization/src/EventSubscriber/BcConfigSubscriber.php
   @@ -0,0 +1,55 @@
   +class BcConfigSubscriber implements EventSubscriberInterface {
   

   Does this have its own tests, and should we have a followup to deprecate it for 9.x at some point?

None: this is an opt-in change. Existing sites continue to get the exact same responses, with booleans and integers as strings.

Not true anymore.

Also the CR is still lying and saying it is BC by default; if we are going to commit this we must correct that.

NW for that and then NR for:

1. Set a flag in state in the update hook.
2. Add an info in hook_requirements() for runtime that if the state flag is set you see the same message as from the hook_update_N() on your status report.

We agreed it should not be a warning but I think at least something in hook_requirements() is the best way to inform people.

Also, if we don't already, a followup to discuss the REST API versioning question and a followup to explore whether to let clients request which version of data they prefer.

I expected to find a big issue discussing REST API versioning - only found #2813311: API versioning: decide & document a backwards-compatibility policy for REST resources so far. I related that one to this.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -463,7 +467,35 @@ public function testGet() {
+      $this->rebuildAll();
+
+
+      $response = $this->request('GET', $url, $request_options);
+      $this->assertResourceResponse(200, FALSE, $response);
+
+
+      // Again do an identical comparison, but this time transform the expected

Ultranit: Double blank lines.

So @Wim Leers says the double blank lines are intentional. I disagree but since it's not a tested standard yet it can be overlooked. #197 is what needs to be addressed here.

I've added the hook_requirements to the Rest module since that is how the serialization change is exposed. I couldn't come up with test that made sense if just the serialization module was installed.

Attaching screenshots as test evidence.

This looks good, I was just about to upload a patch with some similar changes too - one question being "Why doesn't this move to REST module", as well as, 'Why shouldn't we just check the config value directly rather than storing an additional value in state'.

The only remark I have is that we are still only assuming that serialization is only used for REST module. I guess in core it is, so this is not a big deal.

Thanks! Two thoughts here:

1. It is not just their sites that might depend on the data type. It is applications that consume the site API. So maybe we should mention something about applications using the data so that less informed users take that into account?
2. For the BC mode case, should we mention that it is no longer the default? What are the consequences in that situation? Do we plan to remove it in Drupal 9? Will it be deprecated? I know that Nat had concerns about enabling the BC mode, so it seems like there is something additional that needs to be conveyed about using it.

Oh, also."BC layer" is kind of an insider label. Should it be called "backwards-compatibility mode"?

And I think we can remove the word "types" from the label. Fewer words that way so it is easier to understand.

I'll try to think of text improvements that might make the text shorter and clearer for site developers.

Also the CR is still not updated, so the link in the hook doesn't actually tell them the right thing or what exactly to do.

The only remark I have is that we are still only assuming that serialization is only used for REST module. I guess in core it is, so this is not a big deal.

E.g. the JSON API contrib module is also affected by this. So I agree that we should move the hook_requirements() implementation out of the rest module, into the serialization module. However, that makes it a lot more abstract. Plus, the JSON API module has no stable releases yet.

So I think from a usability POV, it's best to keep the warning about the REST module. So, +1, and disagreeing with myself one sentence prior :P

#204

1. There's only so much we can do. I think seeing REST API mentioned in both the status report and the update.php output is as much as we can do.
2. Yes, "the old way" will be removed in Drupal 9. We already link to the change record; I don't know that we really need to add more text to the patch; the CR already says so (I just updated it: https://www.drupal.org/node/2837696/revisions/view/10257511/10343125).

@damiankloip, are there any other use cases for the serialization module that we should take into consideration for this issue and the communication about it?

@Wim Leers:

There's only so much we can do. I think seeing REST API mentioned in both the status report and the update.php output is as much as we can do.

I feel like you are answering a question I didn't ask... there are improvements possible to the text, both ones I suggested and others.

The CR is still not accurate. It says:

Any Drupal 8 site that was installed using Drupal 8.0, 8.1 or 8.2 will continue to have the old behavior. New sites (8.3 or newer) will get the new behavior

That's not true, in a dangerous way. This patch does not enable the BC layer by default. People need to turn it on themselves if they want it, which also means we need to tell them how, e.g. with a short list that has an example drush command, a link to the configuration API, and a link to settings override docs. They also need to understand the consequences of making that choice, consequences about maintainability that were bad enough that we are not providing the BC layer by default.

If it's going to be removed for Drupal 9, we should have a followup issue to mark the BC layer deprecated in 8.4.x or whatever. Currently, it is not deprecated in the patch.

Yes, I would say we would certainly want to remove the BC layer in D9 and default to the proper data types that this achieves.

@xjm: Sure, a module that springs to mind that's a good example is default content (currently has indirect dependency through hal).

Ah that's a great example. Will default content need any changes or upgrade path for data that is now properly typed?

Ah that's a great example. Will default content need any changes or upgrade path for data that is now properly typed?

I doubt any JS/PHP client would have to change. Noone is really strict about types in that world. I think its more about JAVA and maybe stuff like elm, poor amitaibu ;) which could potential requite to change.

For me its kinda easy: Don't touch existing sites as much as possible, ship with the better behaviour on new sites. For me breaking site is just unnecessary. If someone cares about it, they are able to change the setting and be done with it.

For me its kinda easy: Don't touch existing sites as much as possible, ship with the better behaviour on new sites. For me breaking site is just unnecessary. If someone cares about it, they are able to change the setting and be done with it.

Hm that's not what the current patch does though. At @catch's request the BC layer is off by default, even for updated sites. But hopefully most apps will not break.

Thanks @dawehner.

The CR is still not accurate. It says:

Indeed, @catch proposed we don't do enable the BC layer at all, and the patch was modified to do exactly that, but the CR was not updated.

EDIT: rewritten for clarity: Indeed, @catch proposed we don't do enable the BC layer BY DEFAULT, and the patch was modified to do exactly that, but the CR was not updated.

CR now updated: https://www.drupal.org/node/2837696/revisions/view/10343125/10343658.

I agree with this.

For me its kinda easy: Don't touch existing sites as much as possible, ship with the better behaviour on new sites. For me breaking site is just unnecessary. If someone cares about it, they are able to change the setting and be done with it.

That's what our patch was originally doing, up to #153.

So:

1. @dawehner and @cilefen prefer #153 (the original approach), where it's enabled by default for new sites and a BC layer is added and opted in to by default for existing sites -> only new sites get the new behavior by default
2. @catch prefers #202, where it's enabled by default for new sites and a BC layer is added and opted out from by default for existing sites (to get as many sites as possible on the same format); with a message during the update process plus the status being shown in hook_requirements(), to make it as easy/obvious as possible for existing sites to opt in to this if they're affected -> all sites get the new behavior by default
3. @alexpott prefers an approach where it's not enabled by default for new sites, and a BC layer is added and opted in to by default for existing sites -> all sites get the old behavior by default

Finally, it must be said that while this is technically a BC break, for many clients this is not going to make any difference.

@alexpott deferred to @catch. But now it seems still not everybody is on board with @catch's proposal

How do we proceed from here?

My two cents as someone making use of REST in D8, I find #218.1 quite unnerving. I have one D8 site consuming another and have had issues with data types, so simultaneously want this fix, but know I will need to update the client site when it changes.

Not sure if this creates more hassle than its worth, but what about something slightly in the middle. We add to the BC layer the ability for clients to request the old format with a GET parameter or similar. 8.3 ships with #218.1, but hook_requirement() and hook_update_N() messages to say that in 8.4 it will serve the new format by default.

Clients can still request the old format, but have 6 months notice of it. Server admins can opt out of the change by setting a variable prior to updating to 8.4?

Even if the client requesting different formats is too much, ensuring there is a variable well documented in the CR/release notes that can prevent #218.1 prior to update may mitigate some danger?

@andrewbelcher Thanks for chiming in — it's super super valuable to get feedback from people actually affected by this change!

First: I'm glad to hear you're confirming you want this fix, because you've had issues because of it.

Second: with all 3 approaches there is a clear variable you can modify to get the behavior you want! All three ship with the same BC layer! They all just have different values set:

In all of those cases, you're still free to modify the value of bc_primitives_as_strings, because it's just configuration. We're just discussing what those default values should be!

Which one would you prefer, and why?

If I were developing something new, I would definitely want the improved behaviour. With my existing system, I would want to manage that transition myself.

Upgrading major version you would expect to have to deal with those kind of changes, I don't know if you would going up a minor version. Also, the window for going up a major version (if I understand correctly) would be longer as security fixes would still be available for the previous major (LTS), whereas that's not the case for minor releases.

So I guess that would probably put me on #218.1. I suppose my suggestion previously was to do somewhere between #218.1 and #218.2 by making the behaviour of existing sites change in the next minor release, which gives 6 months of visibility for systems to be able to mange it themselves. If I hadn't come across this issue/seen a CR, that would mean I'm not caught out unless I also don't look at my site when I update it.

I think the danger with #218.3 is that we end up with a lot of systems unable to make the move to 9, whereas if we change the behavior now, it will only be systems built in the last year or so.

Interesting, so if I update the table to show that option, it looks like this:

That could also work. Indeed a middle ground between #218.1 and #218.2.

I would be in favor of #221 if we couldn't get #218.1. But it seems like it would be important that in 8.4 there should warning on the status report page that says:

1. There has been this change(improvement?) in the Serialization behaviour and the site is not enabled that change
2. If no action is taken the the site will be upgraded to this new functionality in the next minor update.

Also in the update to 8.5 there should be message for sites that are having this change implemented for them to let them know.

I think even friendlier to developers would be if there could be some way after 8.5(and beyond?) to keep a message about the change on the status report page.

I am thinking of the situation where a developer does nothing in the upgrade to 8.4. Everything works fine in their REST integrations. Then they update to 8.5 they see the message in the upgrade but don't fully realize how it might affect their REST integrations. Then a week a so later they start to get weird behaviour in 1 of their clients(maybe not all because they might handle casting better). Are they going to make the connection to the message they saw during the update but is now gone?

I think an important question is are we deciding on a general policy for this kind of change to REST responses/serialization? It would beneficial for developers to know that this is how these types of changes will happen in the future. It also would allow these types of issues to be resolved faster in the future.

For instance probably if we had already had an issue like this and had a policy to follow then this change would have made it into 8.3.

8.3.x hasn't shipped yet, and we have to wait until 8.4 to fix this bug? No production in the world is running on 8.3.x yet, and what about 8.2.x? I thought the idea of having a BC layer is to let someone (who needs to fix this bug) to fix this bug in the production today.

#223:

I think an important question is are we deciding on a general policy for this kind of change to REST responses/serialization?

I'd like that. But it's likely that there will be examples of encoding and normalization changes that are less disruptive, e.g. pure additions to GET responses, which are not required to be sent on POST/PATCH requests. But I think we'll extract a basic policy from this issue, for cases like this one.

It would beneficial for developers to know that this is how these types of changes will happen in the future.

Indeed!

It also would allow these types of issues to be resolved faster in the future.

Indeed…

#224: It might still ship in 8.3.x. It will depend on when we reach consensus ;) Also, even the act of adding a BC layer can be disruptive. We want to ensure that people can brainlessly update to patch versions. Only minor versions can be slightly disruptive. Patch releases must be zero disruptive.
Also, the point of a beta is that we now only add bugfixes to 8.3.0-beta.

Then a week a so later they start to get weird behaviour in 1 of their clients(maybe not all because they might handle casting better).

Doesn't this mean the point from #219, to allow clients to somehow control this (GET-parameter?), needs to be entertained more? The concern is, after all, that some clients might run into trouble because of this change, so it seems not particular logical (if maybe easier to implement) to make this a central configuration on the server side.

To me, the best option seems to be #221, with a message on the status report page indicating that the BC support is temporary. Existing sites won't simply break upon update to the next minor version, and will have a hopefully clear notice regarding the changes that will need to be made by 6 months time. BC is important, but holding BC behavior over until 9.0.0 seems like it could lead to problems later. Plenty of methods and classes have been deprecated and will be removed before 9.0.0; I think this type of BC holdover should be treated similarly, encouraging developers to move forward.

New sites should use the new behavior, so developers creating a site don't mistakenly build something on top of the old behavior, and need to fix it later. Hopefully developers would see the status page notice even if #218.3 was chosen, but I can certainly imagine some people not realizing the ramifications of that message, and finding themselves in a very discouraging bind when they eventually do.

It seems vital to get this into 8.3.0, as leaving it another 6 months to be fixed will mean many more applications will be built using the old behavior, and need to be changed. Unfortunately no one option can possibly make everyone happy--each of us is in a different situation and has our own priorities and preferences--so let's pick a less disruptive option that still makes timely progress.

It seems vital to get this into 8.3.0, as leaving it another 6 months to be fixed will mean many more applications will be built using the old behavior, and need to be changed.

The deadline for disruptive changes in 8.3.x has already passed. The release and framework managers have agreed to consider a policy exception for this issue in light of the BC and maintainability concerns, but it is discretionary and will be a release management decision. The goal should still to be to come up with the best solution to the BC problem.

The problem with the proposal in #221 (as represented in #222 at least) is the classic Configuration update problem: We have no way of knowing whether the site owner got the value from the default at the time of their 8.4.x update, or whether they set it on purpose because they want their client apps to love strings.

So for me, the question is still about whether the update hook sets the BC layer to on or off. @catch strongly prefers it be off. I think @alexpott and I are both ambivalent; there are strong cases for and against each. Many others have raised that it is actually a disruptive break for the BC layer to be off by default, e.g. @dawehner, @cilefen, @andrewbelcher, @Grayside.

I also agree with #226. I keep thinking that the application should treat this the way we would treat a required BC break for a PHP API: provide a fallback to the old behavior, and let the caller choose if it needs to. If this were a method that needed a signature change, we would either add a new optional parameter, or add a new method and deprecate the old one as a wrapper. Can we provide similar sensible DX and BC for a third-party app somehow for this change?

Also, let's go forward with the hook_requirements() messaging regardless; the final version may need to be updated based on our final decision but it does help site owners' awareness at least.

For the config issues, perhaps a 3 way value would work (on, off or default/temporary)? Or two config keys?

For clients requesting the BC behaviour, I'd thought either a parameter (eg _legacy=1) or possibly encapsulating it in the format (eg _format=json_string)?

One other thought that may be worth considering - what about requiring a value to be explicitly set before the update? Could throw an update exception if it's not set. Would need a really clear helpful message and an obvious way to set it through the UI, but that would avoid unexpected changes whilst maintaining high visibility that the the change is deemed important?

I've had a big long think about this issue and the recent feedback. How about we flip this all around?

1. Backport an opt-in only API addition of the new, properly typed output (and that backport is to 8.3.x if we can), since everyone's concerns about how this disruption gets more disruptive over time are true. But...
2. Make the old behavior the default for not only existing sites updated from 8.2.x, but also new sites installed with 8.3.x. This addresses @catch concern about the maintenance nightmare of having it both ways now.
3. Still start communicating now about the upcoming change with CRs, g.d.o/core, release notes, etc.
4. Make it possible for apps to request the new typed format if they want to be future-compatible.
5. Flip the switch for the next minor (hopefully 8.4.x) as @andrewbelcher and others have suggested, make it the default for new installs in that minor rather than this one, and add the site BC configuration setting and update path for it then rather than now.

#230.2: What is the advantage of new installs using the old behaviour only to be changed in 6 months time? That feels like more of a maintenance nightmare - the BC layer is still going to exist, so any maintenance/support issues are still going to need clarity of whether the BC layer is on/off, regardless of the default.

#230.4: I'm not sure future proofing is really the way an app developer will think. I would expect most app developers would use the default unless there is a problem. App developers may not even know that it's a Drupal 8 API...

Make it possible for apps to request the new typed format if they want to be future-compatible.

This is the huge problem in this proposal.

That requires us to solve REST API versioning, which nobody in the world has ever solved.

Plus, it requires some extra flag to be sent by the client: a request header or a URL query argument. Which is making it Drupal-specific. Which is bad: we don't want to add Drupalisms. As @andrewbelcher said in #231:

App developers may not even know that it's a Drupal 8 API...

Right at the moment I think we should commit this without the bc layer because we have zero reports of this breaking any clients. I'm hoping the people who posted the patch to prevent workarounds in their REST clients tested it with them.

If we get such a report, we still have some time to commit the bc layer (which is already written) as a follow-up. The REST API is not PHP code and we don't have the same bc promise - we'd also potentially fix a int-as-string return from a PHP method without a bc layer if it was seen as a clear bug. The most likely fallout from this seems to me to be that either clients do unnecessary casting, or they continue to work.

If we get such a report, we still have some time to commit the bc layer (which is already written) as a follow-up.

I like this.

The REST API is not PHP code and we don't have the same bc promise

This times a thousand!

we'd also potentially fix a int-as-string return from a PHP method without a bc layer if it was seen as a clear bug. The most likely fallout from this seems to me to be that either clients do unnecessary casting, or they continue to work.

YES!

In this particular case, disruption really is very low, so I find myself agreeing with #233, to my own surprise.

@alexpott, @damiankloip and myself discussed that. We kinda believe that we actually fix way more problems (problems which are maybe not known yet by people) by pushing this change out, than what we prevent by having a BC layer in the first place.

The nervousness for me is really around security releases. Say a significant security bug is found a week or two after 8.3 is released, it gets fixed in 8.3.1. I have an 8.2 site and I now have to decide whether to ignore the security release or potentially break things for clients of my service.

In my case, I'm also the client of my service, so it essentially means I have to rush through testing and fixing any issues.

The mitigation is that I think most of my issues with types was related to tests, not live functionality, which I wouldn't be surprised if that is the norm.

@catch:

Right at the moment I think we should commit this without the bc layer because we have zero reports of this breaking any clients.

Well, @andrewbelcher said in #219 that they would have to make changes in response to it.

@Wim Leers:

That requires us to solve REST API versioning, which nobody in the world has ever solved.

A complete solution would involve API versioning, but sometimes we need one-off workaround rather than complete solutions. A one-off solution would be a single one-off metadata flag, something like #229.

@dawehner:

@alexpott, @damiankloip and myself discussed that. We kinda believe that we actually fix way more problems (problems which are maybe not known yet by people) by pushing this change out, than what we prevent by having a BC layer in the first place.

So just to confirm, you have changed your recommendation from #213?

Oh and re: #233 / #234:

The REST API is not PHP code and we don't have the same bc promise

That really seems like a stretch to me. I would not ever imagine that Drupal's public REST API was not covered by Drupal's public API BC policy.

I'm not necessarily against backporting this (edit: actually I would also really prefer it). I just am concerned that the one contributor who actually has apps has said it is a disruption, but the discussion keeps downplaying that and prioritizing maintenance burden over managing such disruptions to users. See #236.

From app developer perspective, I have to do one of two things below in the world without API versioning:

1. Push out a client side fix to support both Integer and String today/now even when the server end is not updated yet. This fix prepares the app in the transition period. Then, in the future, remove the String handling.

2. Let the app automatically detect the server API changes, if there is any, force self to update to a new app version before opens up.

The work won't change, if the bug fix releases in 8.3.x or 8.4.x. Given that this bug and discussion has been around for a while, I am already prepared to deal with it, and hope to fix it sooner than later.

On the other hand, from Drupal 8 web services SDK/library type of project perspective, fixing this primitive problem now is always better than 6 months later.

So just to confirm, you have changed your recommendation from #213?

Yes, actually I think I totally flipped my opinion.

Oh and re: #233 / #234:

The REST API is not PHP code and we don't have the same bc promise
That really seems like a stretch to me. I would not ever imagine that Drupal's public REST API was not covered by Drupal's public API BC policy.

More as a general statement: If we want to be API first, our public REST API has to treated conceptually similar than our PHP code.

Yes, fair point. I think at this point (and after discussing with Daniel and Alex yesterday) my preference would be to ship this with the BC code but default this to being DISABLED. So we have the 'BC break' as default, but people can easily opt-in to the string behaviour. That would also appease @andrewbelcher and his comment in #236 too I think.

Otherwise, can we just get on with this? The comments are getting way out of hand and it is not being picked apart and theorised far too much.