Problem/Motivation

The Symfony 3 serializer JsonEncode encoder constructor accepts an int.

The Symfony 4.2 serializer JsonEncode encoder constructor accepts an array and deprecates int.

If we want to support both Symfony 3 and Symfony 4, we can't pass either an int or an array, because either array won't be accepted, or int will be deprecated.

Proposed resolution

Subclass it and accept both, without throwing a deprecation.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#47 3020385-47.patch1.73 KBcatch
#45 3020385-44.patch1.73 KBcatch
#45 3020385-interdiff.txt1.29 KBcatch
#45 3020385-combined.patch149.29 KBcatch
#41 3020385-41.patch1.66 KBcatch
#41 3020385-combined.patch150.06 KBcatch
#41 3020385-interdiff.txt150.06 KBcatch
#36 3020385-36.patch2.09 KBcatch
#36 3020385-34-35-interdiff.txt881 bytescatch
#34 3020385-29-combined.patch187.13 KBcatch
#32 3020385-29.patch2.13 KBcatch
#32 3020385-29.patch2.13 KBcatch
#29 3020385-29.patch2.13 KBcatch
#29 3020385-29-combined.patch187.14 KBcatch
#24 interdiff_21-24.txt6.49 KBvacho
#24 3020385-24.patch8.47 KBvacho
#21 3020385-21.patch7.13 KBcatch
#19 3020385-8.patch6.85 KBcatch
#17 3020385-8.patch6.77 KBcatch
#13 3020385-8.patch6.77 KBcatch
#12 3020385-8.patch6.75 KBcatch
#11 3020385-8.patch5.51 KBcatch
#10 3020385-8.patch5.4 KBcatch
#8 3020385-8.patch4.79 KBcatch
#4 3020385-4.patch2.66 KBcatch
#3 3020385.patch3.19 KBcatch
#2 3020385.patch2.66 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

catch’s picture

Status: Active » Needs review
Parent issue: » #2937984: [META] Symfony 4.0 compatibility
FileSize
2.66 KB

Untested patch but something like this.

catch’s picture

Since the subclass defines the constant we can use that reliably.

catch’s picture

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

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

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
4.79 KB

Can't access private properties in a subclass, changing to protected ought to work.

There's a similar deprecation in JsonDecode so handling that here too.

Also realised that it's actually possible to keep the deprecation message given our own subclass supports both versions, but since we only call this once, will contrib ever interact with it? And they'd only get the deprecation if they use our subclass, so leaving it out for now.

Status: Needs review » Needs work

The last submitted patch, 8: 3020385-8.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
5.4 KB

..

catch’s picture

....

catch’s picture

Declaring as protected isn't enough, need to override more methods.

catch’s picture

Having to adapt some Symfony code for PHP 5.5 compatibility..

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

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

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

catch’s picture

#13 should be a green patch I think, or very close.

One more small change then leaving this for a bit.

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

catch’s picture

Better.

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

catch’s picture

Drupal-ifying some of the duplicated code. I'm not sure here between bringing this up to coding standards with full code comments etc. vs calling it a copied class with modifications and slapping codingstandardsignorefile on it, opinions welcome.

catch’s picture

Issue tags: +Novice

Still lots of code style issues here in the move over from Symfony, tagging Novice in case someone wants to take a look - could also improve the documentation.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Wanted to take a look, but I agree it's ideal for a novice contributor :)

vacho’s picture

Solving codding standars on patch.

vacho’s picture

Issue tags: -Needs reroll
catch’s picture

Status: Needs work » Needs review
catch’s picture

Once extra thing we could do here is to restore the deprecation notice that Symfony provides, since core is already updated for the new usage and Drupal 9 may eventually jump to Symfony 5. However I'm not sure in this case whether it should use the Symfony format verbatim or just Drupal-ify it.

borisson_’s picture

#27 I'd prefer to use our own format, so that we can link to this issue (or a change record), making it easier to do the upgrades?

I don't think the changes to ComposerIntegrationTest are needed here? Especially the coding standards changes introduced in #24.

Some other nitpicks:

  1. +++ b/core/lib/Drupal/Core/Serialization/JsonDecode.php
    @@ -0,0 +1,90 @@
    +   * True to return the result as an associative array, false for a nested
    +   * stdClass hierarchy.
    

    This is not allowed, title of a docblock should be one sentence of 80 cols.

  2. +++ b/core/lib/Drupal/Core/Serialization/JsonDecode.php
    @@ -0,0 +1,90 @@
    +  const OPTIONS = 'json_decode_options';
    

    Doesn't have any documentation.

  3. +++ b/core/lib/Drupal/Core/Serialization/JsonDecode.php
    @@ -0,0 +1,90 @@
    +   * @param \Drupal\Core\Serialization\int $depth
    

    This is not correct, should be just int, this is a scalar typehint.

catch’s picture

Thought about the deprecation. It seems unfortunate to introduce two new classes, two new deprecations, and potentially two new tests for the deprecation to fix this issue. Spoke to alexpott and here's a rewritten patch based off that conversation - instead of creating a bridge, we check (via the existence of class constants) whether we're dealing with Symfony 3 or 4 and instantiate the class the correct way.

Complete rewrite, so no interdiff.

alexpott’s picture

+++ b/core/modules/serialization/src/Encoder/JsonEncoder.php
@@ -28,14 +28,49 @@ class JsonEncoder extends BaseJsonEncoder implements EncoderInterface, DecoderIn
+  protected function getJsonEncode() {
...
+  protected function getJsonDecode() {

I'd be seriously tempted to make these final since this workaround is a very very internal detail. Nothing else should be calling these. They should be using the properties set up in the constructor.

Also I like how now we don't have new public API in Drupal-land. +1 to the approach.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
2.13 KB

Did you mean private rather than final? Went with private for now. Agreed these are 100% not an extension point, people can override the constructor if they really wanted to.

Also reversing the conditions which should fix tests - that was leftover debug from figuring out define() doesn't invoke the autoloader.

alexpott’s picture

Yes I meant private. Thanks for reading my mind :)

I think one of the patches on #32 was meant to be a combined patch.

catch’s picture

Managed to upload the same file twice.

sjerdo’s picture

  1. +++ b/core/modules/serialization/src/Encoder/JsonEncoder.php
    @@ -28,14 +28,49 @@ class JsonEncoder extends BaseJsonEncoder implements EncoderInterface, DecoderIn
    +    else {
    +      return new JsonEncode($json_encoding_options);
    +    }
    

    Doesn't really need to be in an else block. This can just be return new JsonEncode($json_encoding_options);

    Or you could change the code to:

    if (defined('JsonEncode::OPTIONS')) {
      $json_encoding_options = [JsonEncode::OPTIONS => $json_encoding_options];
    }
    return new JsonEncode($json_encoding_options);
    
  2. +++ b/core/modules/serialization/src/Encoder/JsonEncoder.php
    @@ -28,14 +28,49 @@ class JsonEncoder extends BaseJsonEncoder implements EncoderInterface, DecoderIn
    +    else {
    +      return new JsonDecode(TRUE);
    +    }
    

    can be replaced by return new JsonDecode(TRUE);

catch’s picture

Updated for #35.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

This looks great. We've got no new API and we can use both Symfony 3 and Symfony 4 JsonEncode/JsonDecode.

larowlan’s picture

review credits

  • larowlan committed 7977626 on 8.7.x
    Issue #3020385 by catch, vacho, alexpott, sjerdo, borisson_: Symfony 3...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Fixed on commit

diff --git a/core/modules/serialization/src/Encoder/JsonEncoder.php b/core/modules/serialization/src/Encoder/JsonEncoder.php
index 3f7d58c54e..4e47e635b2 100644
--- a/core/modules/serialization/src/Encoder/JsonEncoder.php
+++ b/core/modules/serialization/src/Encoder/JsonEncoder.php
@@ -66,7 +66,6 @@ class_exists('JsonDecode');
     return new JsonDecode(TRUE);
   }

-
   /**
    * {@inheritdoc}
    */

Committed as 7977626 and pushed to 8.7.x. Thanks!

catch’s picture

OK so class_exists() doesn't work unless you pass the fully qualified classname. However once you do that, defined() doesn't pick up the constant, unless you also use the full namespaced classname there. This all starts to get a bit ugly and rely on side-effects, so here's a new approach using reflection.

Re-opening because while the original patch does not cause a regression, it also doesn't fix the original bug at all.

edit: messed up the interdiff but scroll to the bottom and it's in there.

alexpott’s picture

+++ b/core/modules/serialization/src/Encoder/JsonEncoder.php
@@ -39,13 +39,12 @@ public function __construct(JsonEncode $encodingImpl = NULL, JsonDecode $decodin
-    // defined() does not trigger the autoloader, so do that first.
-    class_exists('JsonEncode');
...
-    if (defined('JsonEncode::OPTIONS')) {

How about if both are fully defined... or does even having a FQCN in defined without the class exists work?

It'd be nice to avoid reflection.

alexpott’s picture

Also how did #34 pass then?

The last submitted patch, 41: 3020385-combined.patch, failed testing. View results

catch’s picture

If both are fully defined it works, but I don't see a way to do that with defined() without literally spelling out the FQDN as a string. I don't mind doing that to avoid reflection, but it just feels increasingly brittle.

I don't yet know why the test passed, maybe the class is loaded by another test or something? Again makes me wary about using defined() at all.

Now that the code is actually getting executed, it's flushed out a bug with the decode case, new patch to address that for now.

alexpott’s picture

Now that the code is actually getting executed, it's flushed out a bug with the decode case, new patch to address that for now.

Yeah that convinces me. Reflection it is. Predictability and not wondering why something is a FQCN is better.

+++ b/core/modules/serialization/src/Encoder/JsonEncoder.php
@@ -39,13 +39,12 @@ public function __construct(JsonEncode $encodingImpl = NULL, JsonDecode $decodin
+    $reflection = new  \ReflectionClass(JsonEncode::class);

@@ -58,10 +57,9 @@ class_exists('JsonEncode');
+    $reflection = new  \ReflectionClass(JsonDecode::class);

More space than necessary

catch’s picture

Removing the extra space noted in #46.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks great and makes sense - it is a shame we have to use reflection but it won't be around for ever and can be removed in Drupal 9.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 67ed5a5 and pushed to 8.7.x. Thanks!

  • larowlan committed 67ed5a5 on 8.7.x
    Issue #3020385 by catch, vacho, alexpott, larowlan, borisson_, sjerdo:...

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +Symfony 4