LogicException in Drupal\hal\Normalizer\FieldItemNormalizer does not exist

We need to do two things for this issue.

One is super simple: Roll a patch changing LogicException to \LogicException -- not the preceding slash.

Two is a little more tricky. We need to create a test that will trigger the two exceptions noted in the issue summary. These two exceptions are currently untested.

Attached is a patch with tests only and another patch with tests and fix.

1. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldItemNormalizer.php
   @@ -45,10 +45,10 @@ public function normalize($field_item, $format = NULL, array $context = array())
   -      throw new LogicException('$context[\'target_instance\'] must be set to denormalize with the FieldItemNormalizer');
   +      throw new \LogicException('$context[\'target_instance\'] must be set to denormalize with the FieldItemNormalizer');
   ...
   -      throw new LogicException('The field item passed in via $context[\'target_instance\'] must have a parent set.');
   +      throw new \LogicException('The field item passed in via $context[\'target_instance\'] must have a parent set.');
   

   Note: There is a logicexception as part of the serializer namespace, see Symfony\Component\Serializer\Exception\LogicException. The original code intended to use that, as it was probably copied from another place. On top of that I really wonder why we don't use an InvalidArgumentException given that seems to have a better semantic

2. +++ b/core/modules/hal/lib/Drupal/hal/Tests/DenormalizeTest.php
   @@ -23,6 +25,33 @@ public static function getInfo() {
   +    catch (\LogicException $e) {
   ...
   +    catch (\LogicException $e) {
   

   These bits probably have to be adapted based on the previous point.

Firstly, @alexpott asked at #drupaldevdays to convert this mocking mess to proper getMock() stuff, so I implemented that one instead.

Secondly, I have changed from LogicException to InvalidArgumentException as @dawehner asked.

Attached is a patch with tests only and another patch with tests and the fix.

The bot threw this issue back to NW because of the patch which contains only the tests, so it must have failed. The other patch contains tests AND the fix and passed, so setting the issue to NR, as @ianthomas_uk suggested on IRC.

Read through the code; it looks succinct and well written. The fix changes the incorrect LogicException to an \InvalidArgumentException.

Here we go. Changes:

1. Using the phpunit annotation @expectedException.
2. Fixing and testing Drupal\hal\Normalizer\FieldNormalizer too.
3. Using a common data provider for the two.

Do we also want to unit test the other class?

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldItemNormalizer.php
@@ -45,10 +45,10 @@ public function normalize($field_item, $format = NULL, array $context = array())
+      throw new \InvalidArgumentException('$context[\'target_instance\'] must be set to denormalize with the FieldItemNormalizer');
...
+      throw new \InvalidArgumentException('The field item passed in via $context[\'target_instance\'] must have a parent set.');

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FieldNormalizer.php
@@ -62,10 +62,10 @@ public function normalize($field, $format = NULL, array $context = array()) {
+      throw new \InvalidArgumentException('$context[\'target_instance\'] must be set to denormalize with the FieldNormalizer');
...
+      throw new \InvalidArgumentException('The field passed in via $context[\'target_instance\'] must have a parent set.');

You could also use Symfony\Component\Serializer\Exception\InvalidArgumentException.

10: 2225333-fielditemnormalizer-logicexception-10-tests-and-fix.patch queued for re-testing.

10: 2225333-fielditemnormalizer-logicexception-10-tests-and-fix.patch queued for re-testing.

Rerolled the pair of patches as @dawehner asked at #drupaldevdays: testing FieldItemNormalizer::denormalize() and FieldNormalizer::denormalize() in two different classes, but they have a common abstract ancestor as the dataProvider for the two is the same.

Additionally, changed from \InvalidArgumentException to \Symfony\Component\Serializer\Exception\InvalidArgumentException which is eventually the same at code level, but makes more sense semantically.

Awesome!

The "fail" patch failed in #17. The green patch passed.

Awesome!