Clean up \Drupal\hal\Normalizer\NormalizerBase: duplicate less from the parent class \Drupal\serialization\Normalizer\NormalizerBase

Added test to demonstrate that supportsDenormalization() method override was redundant and it's removal will not break anything.

Also noticed that $supportedInterfaceOrClass property can be an array for example:
Drupal\serialization\NormalizerContentEntityNormalizer

protected $supportedInterfaceOrClass = ['Drupal\Core\Entity\ContentEntityInterface'];

In this case implementation of \Drupal\hal\Normalizer\NormalizerBase::supportsDenormalization() will throw \RuntimeException Array to string conversion on this line

$supported = new \ReflectionClass($this->supportedInterfaceOrClass);

Uploading a patch and let's wait until tests will pass.

This has been bothering me for a long time too, but it was never a big enough deal to create this issue. Thanks!

SupportDenormalizationUnitTest is kind of a strange test. Possibly we already have enough functional test coverage that we don't need this unit test that is comparing "old code" with "new code" and asserting the results are the same. This test makes sense right now, but it won't anymore after this is committed.
I'll let a core committer decide.

Just two nits:

+++ b/core/modules/hal/tests/src/Unit/SupportDenormalizationUnitTest.php
@@ -0,0 +1,104 @@
+class SupportDenormalizationUnitTest extends UnitTestCase {
+  /**

+++ b/core/modules/serialization/src/Normalizer/NormalizerBase.php
@@ -16,6 +16,12 @@
   protected $supportedInterfaceOrClass;
+  /**

Needs \n in between. After that's fixed, this is RTBC.

Ahhhh, right! Great :)

Can you then please repulsed #2? Core committers expect the last patch to be the patch to commit.

1. @BR0kEN - it's community convention that you don't rtbc your own patches. Also it's great when the RTBC patch is the most recent patch on the issue so that it is the one being re-tested when the issue is at rtbc.

2. +++ b/core/modules/serialization/src/Normalizer/NormalizerBase.php
   @@ -16,6 +16,12 @@
      protected $supportedInterfaceOrClass;
   +  /**
   

   Need a new line here.

3. +++ b/core/modules/serialization/src/Normalizer/NormalizerBase.php
   @@ -67,7 +73,7 @@ public function supportsDenormalization($data, $type, $format = NULL) {
   -    if (!isset($format) || !isset($this->format)) {
   +    if (!isset($format) || empty($this->format)) {
          return TRUE;
        }
   

   Is this change covered by a test anywhere?

4. Also looking at \Drupal\serialization\Normalizer\NormalizerBase the use of in_array with specifying the 3rd parameter as TRUE makes me afraid of really odd bugs.

@BR0kEN also - just so you know - hiding the files doesn't affect the RTBC retest.

@BR0kEN - it's the lack of the third argument and it not being TRUE that scares me. Pretty much people always want in_array($needle, $haystack, TRUE) and not in_array($needle, $haystack) - see https://3v4l.org/sr29Y

@BR0kEN not in scope because as far as we know it is not causing issues but it'd be great if someone could open an issue about it.

+++ b/core/modules/hal/src/Normalizer/NormalizerBase.php
@@ -11,35 +11,8 @@
   /**
-   * The formats that the Normalizer can handle.
-   *
-   * @var array
-   */
-  protected $formats = ['hal_json'];
...
+  protected $format = ['hal_json'];

Sorry I just noticed this rename. We shouldn't rename a property on a base class. This is API. Things are meant to inherit from this class - it's abstract! Or at least it we do this we need to implement a magic __get and __set for the property "formats" - which gets complex because the original scope of the property is protected.

Maybe the thing to do is make the base class use either $format or $formats? And deprecate $formats. Tricky. BC is hard.

Maybe the thing to do is make the base class use either $format or $formats? And deprecate $formats.

That seems right here. Use $formats if it's set, but trigger_error() with a deprecation notice, use $format otherwise with no deprecation notice. Then a subclass with no changes continues to work the same way but gets the notice.

I thought about doing it the other way, but since the property is already set there doesn't seem a way around that.

For reference, here is the handbook page on how to add the deprecation: https://www.drupal.org/core/deprecation

And in general, yes, we do have to at least consider the impact on custom implementations.

Note that the next steps are clearly described in #24.

This blocks #2856110: [PP-1] Expose entity validation errors in a machine readable REST API: .

There we go.

Thanks!

1. +++ b/core/modules/hal/src/Normalizer/NormalizerBase.php
   @@ -15,4 +15,19 @@
   +    if (!isset($format) || !isset($this->format) || !isset($this->formats)) {
   

   Using either $this->format or $this->formats is allowed.

   Therefore this should be

   if (!isset($format)
    || (!isset($this->format) && !isset($This->formats))) {

2. +++ b/core/modules/hal/src/Normalizer/NormalizerBase.php
   @@ -15,4 +15,19 @@
   +    return in_array($format, (array) $this->format);
   

   And the $this->format here should be either $this->format or $this->formats, right now this is still breaking BC.

   P.S.: also should use strict comparison.

P.S.: also should use strict comparison.

Well, the parent is using the non strict comparison already, but yeah meh.

Using either $this->format or $this->formats is allowed.

Therefore this should be

Oh njce point! I improved the readability of the logic a bit.

I don't understand any of the changes in #33.

Also, yay, #31 failed! Seems like we have some test coverage for this that is now failing due to the bug I pointed out in #32.

I don't understand any of the changes in #33.

Can you clarify what you don't understand? The only logic I applied on top of your comment is: (!$ && !b) == !($a || b)

Oh, hah, I misread! That part now makes sense. But this part still does not:

1. +++ b/core/modules/hal/src/Normalizer/NormalizerBase.php
   @@ -11,35 +11,23 @@
   +    return in_array($format, (array) $this->format, TRUE);
   

   How is this supposed to work, if you used class::$format?

2. +++ b/core/modules/hal/src/Normalizer/NormalizerBase.php
   @@ -11,35 +11,23 @@
   -  protected $formats = ['hal_json'];
   ...
   +  protected $format = ['hal_json'];
   

   Oh… you kept the rename. We shouldn't do that, see #23. That's what I was getting at in #32.2.

I'm not sure why we have to take back the rename ... in case we use something like this ... which I actually wanted to include :)

Now that makes sense :)

Cool :)

Sure why not.

I like this even better. Even clearer that this method is decorating the parent method, but only for BC, and it'll be removed in 9.0.0.

Thanks, BR0kEN!

Committed/pushed to 8.4.x, thanks!

Yay, improved maintainability!

publish the change record