Problem/Motivation

In \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::getUuid

The logic in this method is obviously backwards:


  public function getUuid($data) {
    if (isset($data['uuid'])) {
      return NULL;
    }
    $uuid = $data['uuid'];
    // The value may be a nested array like $uuid[0]['value'].
    if (is_array($uuid) && isset($uuid[0]['value'])) {
      $uuid = $uuid[0]['value'];
    }
    return $uuid;
  }

If the uuid is set the function returns NULL

Proposed resolution

The isset() call should be changed to empty() or it should be changed to !isset()

Remaining tasks

patch and test

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Comments

pwolanin created an issue. See original summary.

ioana apetri’s picture

Assigned: Unassigned » ioana apetri
pwolanin’s picture

Assigned: ioana apetri » Unassigned
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new526 bytes

still needs tests

e0ipso’s picture

Status: Needs review » Needs work

After looking at it, I don't think this code is being used anywhere. How did you stumble upon this bug? Was it executed in your use case?

I don't think we can remove this code anyways because of BC reasons, but if it's not used we should fix it with your patch and mark it @deprecated.

wim leers’s picture

It definitely looks like dead code.

e0ipso’s picture

Yeah, this is probably coming from the HAL normalizer's initial copy.

wim leers’s picture

Yep, you're right: \Drupal\hal\Normalizer\EntityReferenceItemNormalizer::getUuid():

  /**
   * {@inheritdoc}
   */
  public function getUuid($data) {
    if (isset($data['uuid'])) {
      $uuid = $data['uuid'];
      // The value may be a nested array like $uuid[0]['value'].
      if (is_array($uuid) && isset($uuid[0]['value'])) {
        $uuid = $uuid[0]['value'];
      }
      return $uuid;
    }
  }

Interestingly, that code was last modified on Feb 3, 2015. But JSON API's code is from after that time. Since it's different, it looks like JSON API did use it at some point.

So definitely dead code today.

Given that it's indeed obviously broken, shouldn't we just delete it? How does fixing unused, broken code make sense? If we keep BC, we wouldn't be able to change it, but which code could use it since it's broken? Interesting dilemma 😀

wim leers’s picture

Priority: Normal » Minor
Status: Needs work » Needs review
StatusFileSize
new822 bytes

Patch for #7.

Status: Needs review » Needs work

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

wim leers’s picture

Aha, apparently there is a thing called UuidReferenceInterface… Never heard of that before! :O

Created #2930243: Remove unused "entity resolver" functionality from Serialization to fix that weirdness.

wim leers’s picture

Status: Needs work » Needs review
StatusFileSize
new1.11 KB
new1.23 KB
wim leers’s picture

Ignore #11's interdiff, it's the wrong one. The patch is correct though.

Fortunately, he difference between #8 and #11 is so tiny, that it's easy to spot/review even without an interdiff :)

  • e0ipso committed 033c159 on 8.x-1.x authored by pwolanin
    Issue #2925377 by Wim Leers, pwolanin, e0ipso: Incorrect logic in \...
e0ipso’s picture

Status: Needs review » Fixed

Nice little improvement. People will cease to be puzzled by this :-)

Status: Fixed » Closed (fixed)

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