arangodb / arangodb-php

PHP ODM for ArangoDB
https://www.arangodb.com
Apache License 2.0
182 stars 46 forks source link

[ASK] Edge->getById #243

Open lshaf opened 6 years ago

lshaf commented 6 years ago

when I use function getById, why Edge doesn't use documentClass just like document did? is there any reason? I want to know.

jsteemann commented 6 years ago

@lshaf : you mean like this?

index 3e1f0b5..a0e5eea 100644
--- a/lib/ArangoDBClient/EdgeHandler.php
+++ b/lib/ArangoDBClient/EdgeHandler.php
@@ -74,7 +74,9 @@ class EdgeHandler extends DocumentHandler
      */
     public function createFromArrayWithContext($data, $options)
     {
-        return Edge::createFromArray($data, $options);
+        $_documentClass = $this->_documentClass;
+
+        return $_documentClass::createFromArray($data, $options);
     }

That would make the getById method honor the documentClass specified. As far as I can see, the EdgeHandler class does not take _documentClass into account anywhere. So probably some of its other methods need adjustment too.

According to git log, the document class support was contributed by @diabl0 in 2017. @diabl0 : was there any specific reason to exclude the EdgeHandler from using document class?

lshaf commented 6 years ago

@diabl0 do you mind to answer?

jsteemann commented 6 years ago

Looking at this again, I think the main problem with using $_documentClass everywhere is that there may be different document classes for documents and edges. For example, the following code from Cursor.php makes a distinction between documents and edges at the moment:

        $entry = [
            'vertices'    => [],
            'edges'       => [],
            'source'      => $_documentClass::createFromArray($data['source'], $this->_options),
            'destination' => $_documentClass::createFromArray($data['destination'], $this->_options),
        ];
        foreach ($data['vertices'] as $v) {
            $entry['vertices'][] = $_documentClass::createFromArray($v, $this->_options);
        }
        foreach ($data['edges'] as $v) {
            $entry['edges'][] = Edge::createFromArray($v, $this->_options);
        }

I think using $_documentClass::createFromArray also for the edges here will not be ideal. But probably adding $_edgeClass, with a default to \ArangoDBClient\Edge and using that for creating edges would help. What do you think? @lshaf @diabl0

lshaf commented 6 years ago

it will be good and maybe you need to move setDocumentClass to Document and add setEdgeClass to Edge.

Just my idea.

jsteemann commented 6 years ago

Example implementation in https://github.com/arangodb/arangodb-php/pull/251

@lshaf @diabl0 : can you please comment on it? Thanks!

lshaf commented 6 years ago

Looks good but I found something look unnecessary. What do you think? @jsteemann