With the latest dev version the 0 arg in relationships must be numeric. Want to ask why so? Identifiers do not have to be only numeric.

I was using a UUID which is a string distributed among different systems and now it does not work. So what is the value added for enforcing a numeric arg? If there is not really one can the check be removed?

Thanks

CommentFileSizeAuthor
#3 1815984-d7-services-numeric-argument-relationships-3.patch759 bytesmiqmago

Comments

blueminds’s picture

blueminds commented
Status: Active » Closed (fixed)

Looking at the latest repo version, this is not the case anymore and can confirm that relationships can be again addressed by non-numeric identifiers.

miqmago’s picture

miqmago commented

Still happening on 3.2

RESTServer.inc line 606

protected function resolveController($resource, $method, $path, $endpoint, $resource_name) { 

    // 'relationships'
    // First path element should be not empty,
    // second should be name of targeted action.
    if ($method == 'GET' && $pc >= 2 && isset($resource['relationships'][$path[1]])
-         && $this->checkNumberOfArguments($pc, $resource['relationships'][$path[1]], 1) && is_numeric($path[0])) {
+         && $this->checkNumberOfArguments($pc, $resource['relationships'][$path[1]], 1) && !empty($path[0])) {
      $class = 'relationships';
      $operation = $path[1];
    }
miqmago’s picture

miqmago commented
Version: 7.x-3.x-dev » 7.x-3.2
Category: support » bug
Status: Closed (fixed) » Active
StatusFileSize
new 1815984-d7-services-numeric-argument-relationships-3.patch759 bytes

Here it goes the patch

ygerasimov’s picture

ygerasimov commented
Status: Active » Reviewed & tested by the community

This change looks good to me.

kylebrowning’s picture

kylebrowning commented
Status: Reviewed & tested by the community » Needs review

lets double check the tests pass and maybe write a new test to validate this?

Status: Needs review » Needs work

The last submitted patch, 1815984-d7-services-numeric-argument-relationships-3.patch, failed testing.

marcingy’s picture

marcingy commented

The patch as is won't allow for the value 0 which the pervious patch did, so while fixing one issue it is causing a regression in another. Likely needs to be if (!numeric .... && !empty)

kylebrowning’s picture

kylebrowning commented
Version: 7.x-3.2 » 7.x-3.x-dev
Status: Needs work » Needs review
kylebrowning’s picture

kylebrowning commented

why not just isset()
?

marcingy’s picture

marcingy commented

Yes an isset will work

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Closed (fixed)

Fixed in dev.